[PATCH] pci: don't map virtio 1.0 storage devices above 4G

[Marcel Apfelbaum]

Otherwise SeaBIOS can't access virtio's modern BAR.

Signed-off-by: Marcel Apfelbaum 
---

Hi,

If there is no room to map all MMIO BARs into the 32-bit PCI window,
SeaBIOS will re-allocate all 64-bit MMIO BARs into over-4G space.

Virtio 1.0 block devices (virtio-blk/virtio-scsi) use a 64-bit BAR
unusable by SeaBIOS if mapped over 4G space, preventing the system
to boot.

The simplest solution is to follow the xhci model and simply
skip migrating the virtio 1.0 modern bar into over-4G space.

In order to reproduce the problem use:
   -device virtio-blk-pci, ...\
   -object memory-backend-file,id=mem,size=4G,mem-path=/dev/hugepages,share=on \
   -device ivshmem-plain,memdev=mem \

Thanks,
Marcel

src/fw/pciinit.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index 35d9902..3b76e66 100644
--- a/src/fw/pciinit.c
+++ b/src/fw/pciinit.c
@@ -655,6 +655,13 @@ static void pci_region_migrate_64bit_entries(struct pci_region *from,
             continue;
         if (entry->dev->class == PCI_CLASS_SERIAL_USB)
             continue;
+        if (entry->dev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET &&
+            (entry->dev->device == PCI_DEVICE_ID_VIRTIO_BLK_09  ||
+             entry->dev->device == PCI_DEVICE_ID_VIRTIO_BLK_10  ||
+             entry->dev->device == PCI_DEVICE_ID_VIRTIO_SCSI_09 ||
+             entry->dev->device == PCI_DEVICE_ID_VIRTIO_SCSI_10))
+            continue;
+
         // Move from source list to destination list.
         hlist_del(&entry->node);
         hlist_add(&entry->node, last);

[Michael S. Tsirkin]

On Sun, Sep 11, 2016 at 08:23:34PM +0300, Marcel Apfelbaum wrote:
> Otherwise SeaBIOS can't access virtio's modern BAR.
> Signed-off-by: Marcel Apfelbaum
>
> Hi,
>
> If there is no room to map all MMIO BARs into the 32-bit PCI window,
> SeaBIOS will re-allocate all 64-bit MMIO BARs into over-4G space.
>
> Virtio 1.0 block devices (virtio-blk/virtio-scsi) use a 64-bit BAR
> unusable by SeaBIOS if mapped over 4G space, preventing the system
> to boot.
>
> The simplest solution is to follow the xhci model and simply
> skip migrating the virtio 1.0 modern bar into over-4G space.
>
> In order to reproduce the problem use:
>    -device virtio-blk-pci, ...\
>    -object memory-backend-file,id=mem,size=4G,mem-path=/dev/hugepages,share=on \
>    -device ivshmem-plain,memdev=mem \
>
> Thanks,
> Marcel
> src/fw/pciinit.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index 35d9902..3b76e66 100644
> --- a/src/fw/pciinit.c
> +++ b/src/fw/pciinit.c
> @@ -655,6 +655,13 @@ static void pci_region_migrate_64bit_entries(struct pci_region *from,
>              continue;
>          if (entry->dev->class == PCI_CLASS_SERIAL_USB)
>              continue;
>    if (entry->dev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET &&
>        (entry->dev->device == PCI_DEVICE_ID_VIRTIO_BLK_09  ||
>         entry->dev->device == PCI_DEVICE_ID_VIRTIO_BLK_10  ||
>         entry->dev->device == PCI_DEVICE_ID_VIRTIO_SCSI_09 ||
>         entry->dev->device == PCI_DEVICE_ID_VIRTIO_SCSI_10))
>        continue;
>    // Move from source list to destination list.
>    hlist_del(&entry->node);
>    hlist_add(&entry->node, last);

What if guest is booting from the network?

Rather than special-case virtio storage, it seems to me that the right
thing to do is to only allocate resources for the boot devices, and rely
on the OS to allocate resources for the rest.  IIUC this is typically
controlled by the plug and play OS flag in smbios.

[Marcel Apfelbaum]

On 09/12/2016 06:46 AM, Michael S. Tsirkin wrote:
> On Sun, Sep 11, 2016 at 08:23:34PM +0300, Marcel Apfelbaum wrote:
> > Otherwise SeaBIOS can't access virtio's modern BAR.
> > Signed-off-by: Marcel Apfelbaum
> >
> > Hi,
> >
> > If there is no room to map all MMIO BARs into the 32-bit PCI window,
> > SeaBIOS will re-allocate all 64-bit MMIO BARs into over-4G space.
> >
> > Virtio 1.0 block devices (virtio-blk/virtio-scsi) use a 64-bit BAR
> > unusable by SeaBIOS if mapped over 4G space, preventing the system
> > to boot.
> >
> > The simplest solution is to follow the xhci model and simply
> > skip migrating the virtio 1.0 modern bar into over-4G space.
> >
> > In order to reproduce the problem use:
> >    -device virtio-blk-pci, ...\
> >    -object memory-backend-file,id=mem,size=4G,mem-path=/dev/hugepages,share=on \
> >    -device ivshmem-plain,memdev=mem \
> >
> > Thanks,
> > Marcel
> > src/fw/pciinit.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> > index 35d9902..3b76e66 100644
> > --- a/src/fw/pciinit.c
> > +++ b/src/fw/pciinit.c
> > @@ -655,6 +655,13 @@ static void pci_region_migrate_64bit_entries(struct pci_region *from,
> >              continue;
> >          if (entry->dev->class == PCI_CLASS_SERIAL_USB)
> >              continue;
> >    if (entry->dev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET &&
> >        (entry->dev->device == PCI_DEVICE_ID_VIRTIO_BLK_09  ||
> >         entry->dev->device == PCI_DEVICE_ID_VIRTIO_BLK_10  ||
> >         entry->dev->device == PCI_DEVICE_ID_VIRTIO_SCSI_09 ||
> >         entry->dev->device == PCI_DEVICE_ID_VIRTIO_SCSI_10))
> >        continue;
> >    // Move from source list to destination list.
> >    hlist_del(&entry->node);
> >    hlist_add(&entry->node, last);
>
> What if guest is booting from the network?

No harm done. And the Guest OS can re-map the hardware registers to 64-bit if desired.
> Rather than special-case virtio storage,

It is not a special case. The system can boot from usb, storage or network. (I might forget something here :))
Each hardware vendor needs to ensure their hardware can be used
by the firmware and this is what we are doing here.

For example commit (a247e678) pci: don't map usb host adapters above 4G
does that for all USB controllers.

it seems to me that the right
> thing to do is to only allocate resources for the boot devices, and rely
> on the OS to allocate resources for the rest.  IIUC this is typically
> controlled by the plug and play OS flag in smbios.

What about not PNP Operating Systems? The firmware should address them all.
Also, what about the first time we load the OS, this smbios OS flag would not be there
and we can't install the system. (if I got this flag right)

This seems to me as to much changes to the ecosystem
(this firmware, all pxe drivers out there, specific issues with different OSes...)
for the specific case, I am not saying we shouldn't do that for the long run.

Thanks,
Marcel

[Gerd Hoffmann]

Hi,
> Virtio 1.0 block devices (virtio-blk/virtio-scsi) use a 64-bit BAR
> unusable by SeaBIOS if mapped over 4G space, preventing the system
> to boot.

Not needed, works fine in master (see commit
0e21548b15e25e010c362ea13d170c61a3fcc899).

Is not a clean cherry-pick for 1.9-stable though, so this isn't yet in
any seabios release.

So, maybe it is time to plan a 1.10 release?  There are also a number of
other improvements and cleanups in the master branch.  Kevin?

cheers,
  Gerd

Next release

[Kevin O'Connor]

On Mon, Sep 12, 2016 at 10:01:18AM +0200, Gerd Hoffmann wrote:
> Hi,
> > Virtio 1.0 block devices (virtio-blk/virtio-scsi) use a 64-bit BAR
> > unusable by SeaBIOS if mapped over 4G space, preventing the system
> > to boot.
>
> Not needed, works fine in master (see commit
> 0e21548b15e25e010c362ea13d170c61a3fcc899).
>
> Is not a clean cherry-pick for 1.9-stable though, so this isn't yet in
> any seabios release.
>
> So, maybe it is time to plan a 1.10 release?  There are also a number of
> other improvements and cleanups in the master branch.  Kevin?

Makes sense.  Freeze at end of month and release mid-October?

-Kevin

Next release

[Gerd Hoffmann]

> > So, maybe it is time to plan a 1.10 release?  There are also a number of
> > other improvements and cleanups in the master branch.  Kevin?
>
> Makes sense.  Freeze at end of month and release mid-October?

Looks good to me.

cheers,
  Gerd