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

["Michael S. Tsirkin" <mst@redhat.com>]

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