[Qemu-devel] [PATCH] virtio-pci: make pci bar layout more flexible.

[Qemu-devel] [PATCH] virtio-pci: make pci bar layout more flexible.

[Gerd Hoffmann]

This patch makes the bar layout for virtio pci devices configurable.
This is used to
  (a) create different layouts for legacy/transitional vs. modern
      devices.
  (b) make sure we use unused pci bars, by checking
      PCIDevice->io_regions

VirtIOPCIProxy subclasses which need additional pci bars, such as
virtio-vga, just need to make sure they register the additinal bars
before initializing virtio-pci, so the superclass can see the registered
bars and shuffle around the virtio bars accordingly.

The patch also makes sure the modern bar is properly aligned so it can
be a 64bit bar, and the prefetchable + 64bit attributes are enabled.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/virtio/virtio-pci.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 6b1f422..a2cfb1f 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -965,8 +965,6 @@ static void virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
     PCIDevice *dev = &proxy->pci_dev;
     int offset;
 
-    cap->bar = 2;
-
     offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0, cap->cap_len);
     assert(offset > 0);
 
@@ -1232,9 +1230,12 @@ static void virtio_pci_device_write(void *opaque, hwaddr addr,
 static void virtio_pci_device_plugged(DeviceState *d)
 {
     VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
+    PCIDevice *pci = PCI_DEVICE(d);
     VirtioBusState *bus = &proxy->bus;
     bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY);
     bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
+    uint32_t modern_mem_bar;  /* 64bit bar */
+    uint32_t msix_bar;        /* 32bit bar */
     uint8_t *config;
     uint32_t size;
 
@@ -1248,6 +1249,12 @@ static void virtio_pci_device_plugged(DeviceState *d)
         pci_set_word(config + PCI_SUBSYSTEM_VENDOR_ID,
                      pci_get_word(config + PCI_VENDOR_ID));
         pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
+        msix_bar = 1;
+        modern_mem_bar = 2;
+        while (pci->io_regions[modern_mem_bar].size ||
+               pci->io_regions[modern_mem_bar+1].size) {
+            modern_mem_bar += 2;
+        }
     } else {
         /* pure virtio-1.0 */
         pci_set_word(config + PCI_VENDOR_ID,
@@ -1255,7 +1262,18 @@ static void virtio_pci_device_plugged(DeviceState *d)
         pci_set_word(config + PCI_DEVICE_ID,
                      0x1040 + virtio_bus_get_vdev_id(bus));
         pci_config_set_revision(config, 1);
+        modern_mem_bar = 0;
+        while (pci->io_regions[modern_mem_bar].size ||
+               pci->io_regions[modern_mem_bar+1].size) {
+            modern_mem_bar += 2;
+        }
+        msix_bar = modern_mem_bar + 2;
+        while (pci->io_regions[msix_bar].size) {
+            msix_bar++;
+        }
     }
+    assert(modern_mem_bar <= 4);
+    assert(msix_bar <= 5);
     config[PCI_INTERRUPT_PIN] = 1;
 
 
@@ -1263,29 +1281,34 @@ static void virtio_pci_device_plugged(DeviceState *d)
         struct virtio_pci_cap common = {
             .cfg_type = VIRTIO_PCI_CAP_COMMON_CFG,
             .cap_len = sizeof common,
+            .bar = modern_mem_bar,
             .offset = cpu_to_le32(0x0),
             .length = cpu_to_le32(0x1000),
         };
         struct virtio_pci_cap isr = {
             .cfg_type = VIRTIO_PCI_CAP_ISR_CFG,
             .cap_len = sizeof isr,
+            .bar = modern_mem_bar,
             .offset = cpu_to_le32(0x1000),
             .length = cpu_to_le32(0x1000),
         };
         struct virtio_pci_cap device = {
             .cfg_type = VIRTIO_PCI_CAP_DEVICE_CFG,
             .cap_len = sizeof device,
+            .bar = modern_mem_bar,
             .offset = cpu_to_le32(0x2000),
             .length = cpu_to_le32(0x1000),
         };
         struct virtio_pci_notify_cap notify = {
             .cap.cfg_type = VIRTIO_PCI_CAP_NOTIFY_CFG,
             .cap.cap_len = sizeof notify,
+            .cap.bar = modern_mem_bar,
             .cap.offset = cpu_to_le32(0x3000),
             .cap.length = cpu_to_le32(QEMU_VIRTIO_PCI_QUEUE_MEM_MULT *
                                       VIRTIO_PCI_QUEUE_MAX),
             .notify_off_multiplier = cpu_to_le32(QEMU_VIRTIO_PCI_QUEUE_MEM_MULT),
         };
+        uint32_t modern_mem_attr;
 
         static const MemoryRegionOps common_ops = {
             .read = virtio_pci_common_read,
@@ -1359,12 +1382,15 @@ static void virtio_pci_device_plugged(DeviceState *d)
                               QEMU_VIRTIO_PCI_QUEUE_MEM_MULT *
                               VIRTIO_PCI_QUEUE_MAX);
         memory_region_add_subregion(&proxy->modern_bar, 0x3000, &proxy->notify);
-        pci_register_bar(&proxy->pci_dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY,
-                         &proxy->modern_bar);
+        modern_mem_attr = (PCI_BASE_ADDRESS_SPACE_MEMORY |
+                           PCI_BASE_ADDRESS_MEM_PREFETCH |
+                           PCI_BASE_ADDRESS_MEM_TYPE_64);
+        pci_register_bar(&proxy->pci_dev, modern_mem_bar,
+                         modern_mem_attr, &proxy->modern_bar);
     }
 
     if (proxy->nvectors &&
-        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1)) {
+        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, msix_bar)) {
         error_report("unable to init msix vectors to %" PRIu32,
                      proxy->nvectors);
         proxy->nvectors = 0;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] virtio-pci: make pci bar layout more flexible.

[Michael S. Tsirkin]

On Tue, Mar 03, 2015 at 02:55:13PM +0100, Gerd Hoffmann wrote:
> This patch makes the bar layout for virtio pci devices configurable.
> This is used to
>   (a) create different layouts for legacy/transitional vs. modern
>       devices.
>   (b) make sure we use unused pci bars, by checking
>       PCIDevice->io_regions
> 
> VirtIOPCIProxy subclasses which need additional pci bars, such as
> virtio-vga, just need to make sure they register the additinal bars
> before initializing virtio-pci, so the superclass can see the registered
> bars and shuffle around the virtio bars accordingly.

I think I prefer we just DTRT and keep same layouts for everyone by
default: isn't there a layout that is good for everybody?
Maybe you could give two examples that require two
incompatible layouts?
I know this means we'll leave BAR0 unused for modern devices but that
does not seem too bad.


> The patch also makes sure the modern bar is properly aligned so it can
> be a 64bit bar, and the prefetchable + 64bit attributes are enabled.

This can be a separate patch?

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/virtio/virtio-pci.c | 36 +++++++++++++++++++++++++++++++-----
>  1 file changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 6b1f422..a2cfb1f 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -965,8 +965,6 @@ static void virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
>      PCIDevice *dev = &proxy->pci_dev;
>      int offset;
>  
> -    cap->bar = 2;
> -
>      offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0, cap->cap_len);
>      assert(offset > 0);
>  
> @@ -1232,9 +1230,12 @@ static void virtio_pci_device_write(void *opaque, hwaddr addr,
>  static void virtio_pci_device_plugged(DeviceState *d)
>  {
>      VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
> +    PCIDevice *pci = PCI_DEVICE(d);
>      VirtioBusState *bus = &proxy->bus;
>      bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY);
>      bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
> +    uint32_t modern_mem_bar;  /* 64bit bar */
> +    uint32_t msix_bar;        /* 32bit bar */
>      uint8_t *config;
>      uint32_t size;
>  
> @@ -1248,6 +1249,12 @@ static void virtio_pci_device_plugged(DeviceState *d)
>          pci_set_word(config + PCI_SUBSYSTEM_VENDOR_ID,
>                       pci_get_word(config + PCI_VENDOR_ID));
>          pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
> +        msix_bar = 1;
> +        modern_mem_bar = 2;
> +        while (pci->io_regions[modern_mem_bar].size ||
> +               pci->io_regions[modern_mem_bar+1].size) {
> +            modern_mem_bar += 2;
> +        }
>      } else {
>          /* pure virtio-1.0 */
>          pci_set_word(config + PCI_VENDOR_ID,
> @@ -1255,7 +1262,18 @@ static void virtio_pci_device_plugged(DeviceState *d)
>          pci_set_word(config + PCI_DEVICE_ID,
>                       0x1040 + virtio_bus_get_vdev_id(bus));
>          pci_config_set_revision(config, 1);
> +        modern_mem_bar = 0;
> +        while (pci->io_regions[modern_mem_bar].size ||
> +               pci->io_regions[modern_mem_bar+1].size) {
> +            modern_mem_bar += 2;
> +        }
> +        msix_bar = modern_mem_bar + 2;
> +        while (pci->io_regions[msix_bar].size) {
> +            msix_bar++;
> +        }
>      }
> +    assert(modern_mem_bar <= 4);
> +    assert(msix_bar <= 5);
>      config[PCI_INTERRUPT_PIN] = 1;
>  
>  
> @@ -1263,29 +1281,34 @@ static void virtio_pci_device_plugged(DeviceState *d)
>          struct virtio_pci_cap common = {
>              .cfg_type = VIRTIO_PCI_CAP_COMMON_CFG,
>              .cap_len = sizeof common,
> +            .bar = modern_mem_bar,
>              .offset = cpu_to_le32(0x0),
>              .length = cpu_to_le32(0x1000),
>          };
>          struct virtio_pci_cap isr = {
>              .cfg_type = VIRTIO_PCI_CAP_ISR_CFG,
>              .cap_len = sizeof isr,
> +            .bar = modern_mem_bar,
>              .offset = cpu_to_le32(0x1000),
>              .length = cpu_to_le32(0x1000),
>          };
>          struct virtio_pci_cap device = {
>              .cfg_type = VIRTIO_PCI_CAP_DEVICE_CFG,
>              .cap_len = sizeof device,
> +            .bar = modern_mem_bar,
>              .offset = cpu_to_le32(0x2000),
>              .length = cpu_to_le32(0x1000),
>          };
>          struct virtio_pci_notify_cap notify = {
>              .cap.cfg_type = VIRTIO_PCI_CAP_NOTIFY_CFG,
>              .cap.cap_len = sizeof notify,
> +            .cap.bar = modern_mem_bar,
>              .cap.offset = cpu_to_le32(0x3000),
>              .cap.length = cpu_to_le32(QEMU_VIRTIO_PCI_QUEUE_MEM_MULT *
>                                        VIRTIO_PCI_QUEUE_MAX),
>              .notify_off_multiplier = cpu_to_le32(QEMU_VIRTIO_PCI_QUEUE_MEM_MULT),
>          };
> +        uint32_t modern_mem_attr;
>  
>          static const MemoryRegionOps common_ops = {
>              .read = virtio_pci_common_read,
> @@ -1359,12 +1382,15 @@ static void virtio_pci_device_plugged(DeviceState *d)
>                                QEMU_VIRTIO_PCI_QUEUE_MEM_MULT *
>                                VIRTIO_PCI_QUEUE_MAX);
>          memory_region_add_subregion(&proxy->modern_bar, 0x3000, &proxy->notify);
> -        pci_register_bar(&proxy->pci_dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY,
> -                         &proxy->modern_bar);
> +        modern_mem_attr = (PCI_BASE_ADDRESS_SPACE_MEMORY |
> +                           PCI_BASE_ADDRESS_MEM_PREFETCH |
> +                           PCI_BASE_ADDRESS_MEM_TYPE_64);
> +        pci_register_bar(&proxy->pci_dev, modern_mem_bar,
> +                         modern_mem_attr, &proxy->modern_bar);
>      }
>  
>      if (proxy->nvectors &&
> -        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1)) {
> +        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, msix_bar)) {
>          error_report("unable to init msix vectors to %" PRIu32,
>                       proxy->nvectors);
>          proxy->nvectors = 0;
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH] virtio-pci: make pci bar layout more flexible.

[Gerd Hoffmann]

  Hi,

> > VirtIOPCIProxy subclasses which need additional pci bars, such as
> > virtio-vga, just need to make sure they register the additinal bars
> > before initializing virtio-pci, so the superclass can see the registered
> > bars and shuffle around the virtio bars accordingly.
> 
> I think I prefer we just DTRT and keep same layouts for everyone by
> default: isn't there a layout that is good for everybody?

I want bar #2 for the vga framebuffer for virtio-vga.  Which conflicts
with bar #2 being used for the modern bar in todays code.

We can move the modern bar to #4 for everybody, then we'll have:

  #0 -- legacy i/o
  #1 -- msix
  #2 -- unused (by virtio)
  #3 -- unused (by virtio)
  #4 -- modern mem
  #5 -- modern mem too (because it's 64bit).

That'll leave bars #2 + #3 free, for additional bars (1x 64bit or 2x
32bit) such as vga framebuffer if needed.

> I know this means we'll leave BAR0 unused for modern devices but that
> does not seem too bad.

Yep, no technical reason against it, although it IMHO looks nicer.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] virtio-pci: make pci bar layout more flexible.

[Michael S. Tsirkin]

On Wed, Mar 04, 2015 at 11:55:50AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > VirtIOPCIProxy subclasses which need additional pci bars, such as
> > > virtio-vga, just need to make sure they register the additinal bars
> > > before initializing virtio-pci, so the superclass can see the registered
> > > bars and shuffle around the virtio bars accordingly.
> > 
> > I think I prefer we just DTRT and keep same layouts for everyone by
> > default: isn't there a layout that is good for everybody?
> 
> I want bar #2 for the vga framebuffer for virtio-vga.  Which conflicts
> with bar #2 being used for the modern bar in todays code.

OK, and IIUC you are saying that using bar #2 for that is a requirement for
vga to work?

> We can move the modern bar to #4 for everybody, then we'll have:
> 
>   #0 -- legacy i/o
>   #1 -- msix
>   #2 -- unused (by virtio)
>   #3 -- unused (by virtio)
>   #4 -- modern mem
>   #5 -- modern mem too (because it's 64bit).
> 
> That'll leave bars #2 + #3 free, for additional bars (1x 64bit or 2x
> 32bit) such as vga framebuffer if needed.

Yes, that's fine with me.

> > I know this means we'll leave BAR0 unused for modern devices but that
> > does not seem too bad.
> 
> Yep, no technical reason against it, although it IMHO looks nicer.
> 
> cheers,
>   Gerd

If we run into a problem, we'll be able to reshuffle it all easily.

-- 
MST

Re: [Qemu-devel] [PATCH] virtio-pci: make pci bar layout more flexible.

[Gerd Hoffmann]

On Mi, 2015-03-04 at 12:04 +0100, Michael S. Tsirkin wrote:
> On Wed, Mar 04, 2015 at 11:55:50AM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > > VirtIOPCIProxy subclasses which need additional pci bars, such as
> > > > virtio-vga, just need to make sure they register the additinal bars
> > > > before initializing virtio-pci, so the superclass can see the registered
> > > > bars and shuffle around the virtio bars accordingly.
> > > 
> > > I think I prefer we just DTRT and keep same layouts for everyone by
> > > default: isn't there a layout that is good for everybody?
> > 
> > I want bar #2 for the vga framebuffer for virtio-vga.  Which conflicts
> > with bar #2 being used for the modern bar in todays code.
> 
> OK, and IIUC you are saying that using bar #2 for that is a requirement for
> vga to work?

Makes things alot easier as seavgabios has support for virtio-vga (in
vga compat mode) already and expects the framebuffer being in bar #2.

It's not a _really_ hard requirement though.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] virtio-pci: make pci bar layout more flexible.

[Michael S. Tsirkin]

On Wed, Mar 04, 2015 at 12:46:50PM +0100, Gerd Hoffmann wrote:
> On Mi, 2015-03-04 at 12:04 +0100, Michael S. Tsirkin wrote:
> > On Wed, Mar 04, 2015 at 11:55:50AM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > > VirtIOPCIProxy subclasses which need additional pci bars, such as
> > > > > virtio-vga, just need to make sure they register the additinal bars
> > > > > before initializing virtio-pci, so the superclass can see the registered
> > > > > bars and shuffle around the virtio bars accordingly.
> > > > 
> > > > I think I prefer we just DTRT and keep same layouts for everyone by
> > > > default: isn't there a layout that is good for everybody?
> > > 
> > > I want bar #2 for the vga framebuffer for virtio-vga.  Which conflicts
> > > with bar #2 being used for the modern bar in todays code.
> > 
> > OK, and IIUC you are saying that using bar #2 for that is a requirement for
> > vga to work?
> 
> Makes things alot easier as seavgabios has support for virtio-vga (in
> vga compat mode) already and expects the framebuffer being in bar #2.
> 
> It's not a _really_ hard requirement though.
> 
> cheers,
>   Gerd
> 

Easy is good, I think we don't really care where the modern bar is,
let's just move it for everyone, and add a comment explaining the
motivation.

-- 
MST

Re: [Qemu-devel] [PATCH] virtio-pci: make pci bar layout more flexible.

[Michael S. Tsirkin]

On Wed, Mar 04, 2015 at 12:51:35PM +0100, Michael S. Tsirkin wrote:
> On Wed, Mar 04, 2015 at 12:46:50PM +0100, Gerd Hoffmann wrote:
> > On Mi, 2015-03-04 at 12:04 +0100, Michael S. Tsirkin wrote:
> > > On Wed, Mar 04, 2015 at 11:55:50AM +0100, Gerd Hoffmann wrote:
> > > >   Hi,
> > > > 
> > > > > > VirtIOPCIProxy subclasses which need additional pci bars, such as
> > > > > > virtio-vga, just need to make sure they register the additinal bars
> > > > > > before initializing virtio-pci, so the superclass can see the registered
> > > > > > bars and shuffle around the virtio bars accordingly.
> > > > > 
> > > > > I think I prefer we just DTRT and keep same layouts for everyone by
> > > > > default: isn't there a layout that is good for everybody?
> > > > 
> > > > I want bar #2 for the vga framebuffer for virtio-vga.  Which conflicts
> > > > with bar #2 being used for the modern bar in todays code.
> > > 
> > > OK, and IIUC you are saying that using bar #2 for that is a requirement for
> > > vga to work?
> > 
> > Makes things alot easier as seavgabios has support for virtio-vga (in
> > vga compat mode) already and expects the framebuffer being in bar #2.
> > 
> > It's not a _really_ hard requirement though.
> > 
> > cheers,
> >   Gerd
> > 
> 
> Easy is good, I think we don't really care where the modern bar is,
> let's just move it for everyone, and add a comment explaining the
> motivation.

OTOH if we really want to make it flexible, we can add a new
virtio pci capability, with a new cfg_type value, say 0x6,
and use that to describe BAR and offset for vga use.
I'm not sure I understand all the implications though.

> -- 
> MST