Re: [Qemu-devel] [PATCH 5/6] virtio-pci: introduce pio notification capability for modern device

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

On Fri, Aug 21, 2015 at 05:05:49PM +0800, Jason Wang wrote:
> We used to use mmio for notification. This could be slow on some arch
> (e.g on x86 without EPT). So this patch introduces pio bar and a pio
> notification cap for modern device. This ability is enabled through
> property "modern-pio-notify" for virtio pci devices and was disabled
> by default. Management can enable when it thinks it was needed.
> 
> Benchmarks shows almost no obvious difference with legacy device.
> Thanks Wenli Quan <wquan@redhat.com> for the benchmarking.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

I don't really care much about non-EPT hosts, but if you propose
a patch to optimize them, it should be accompanied by numbers
showing the performance difference.

> ---
>  hw/virtio/virtio-pci.c | 122 ++++++++++++++++++++++++++++++++++++++++++-------
>  hw/virtio/virtio-pci.h |  10 ++++
>  2 files changed, 115 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index fbd1f1f..e399565 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -210,7 +210,9 @@ static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy,
>      EventNotifier *notifier = virtio_queue_get_host_notifier(vq);
>      bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY);
>      bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
> +    bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY;
>      MemoryRegion *modern_mr = &proxy->notify.mr;
> +    MemoryRegion *modern_notify_mr = &proxy->notify_pio.mr;
>      MemoryRegion *legacy_mr = &proxy->bar;
>      hwaddr modern_addr = QEMU_VIRTIO_PCI_QUEUE_MEM_MULT *
>                           virtio_get_queue_index(vq);
> @@ -228,6 +230,10 @@ static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy,
>          if (modern) {
>              memory_region_add_eventfd(modern_mr, modern_addr, 0,
>                                        false, n, notifier);
> +            if (modern_pio) {
> +                memory_region_add_eventfd(modern_notify_mr, 0, 2,
> +                                              true, n, notifier);
> +            }
>          }
>          if (legacy) {
>              memory_region_add_eventfd(legacy_mr, legacy_addr, 2,
> @@ -237,6 +243,10 @@ static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy,
>          if (modern) {
>              memory_region_del_eventfd(modern_mr, modern_addr, 0,
>                                        false, n, notifier);
> +            if (modern_pio) {
> +                memory_region_del_eventfd(modern_notify_mr, 0, 2,
> +                                          true, n, notifier);
> +            }
>          }
>          if (legacy) {
>              memory_region_del_eventfd(legacy_mr, legacy_addr, 2,
> @@ -1344,6 +1354,17 @@ static void virtio_pci_notify_write(void *opaque, hwaddr addr,
>      }
>  }
>  
> +static void virtio_pci_notify_write_pio(void *opaque, hwaddr addr,
> +                                        uint64_t val, unsigned size)
> +{
> +    VirtIODevice *vdev = opaque;
> +    unsigned queue = val;
> +
> +    if (queue < VIRTIO_QUEUE_MAX) {
> +        virtio_queue_notify(vdev, queue);
> +    }
> +}
> +
>  static uint64_t virtio_pci_isr_read(void *opaque, hwaddr addr,
>                                      unsigned size)
>  {
> @@ -1437,6 +1458,16 @@ static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
>          },
>          .endianness = DEVICE_LITTLE_ENDIAN,
>      };
> +    static const MemoryRegionOps notify_pio_ops = {
> +        .read = virtio_pci_notify_read,
> +        .write = virtio_pci_notify_write_pio,
> +        .impl = {
> +            .min_access_size = 1,
> +            .max_access_size = 4,
> +        },
> +        .endianness = DEVICE_LITTLE_ENDIAN,
> +    };
> +
>  
>      memory_region_init_io(&proxy->common.mr, OBJECT(proxy),
>                            &common_ops,
> @@ -1461,30 +1492,60 @@ static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
>                            virtio_bus_get_device(&proxy->bus),
>                            "virtio-pci-notify",
>                            proxy->notify.size);
> +
> +    memory_region_init_io(&proxy->notify_pio.mr, OBJECT(proxy),
> +                          &notify_pio_ops,
> +                          virtio_bus_get_device(&proxy->bus),
> +                          "virtio-pci-notify-pio",
> +                          proxy->notify.size);
>  }
>  
>  static void virtio_pci_modern_region_map(VirtIOPCIProxy *proxy,
>                                           VirtIOPCIRegion *region,
> -                                         struct virtio_pci_cap *cap)
> +                                         struct virtio_pci_cap *cap,
> +                                         MemoryRegion *mr,
> +                                         uint8_t bar)
>  {
> -    memory_region_add_subregion(&proxy->modern_bar,
> -                                region->offset,
> -                                &region->mr);
> +    memory_region_add_subregion(mr, region->offset, &region->mr);
>  
>      cap->cfg_type = region->type;
> -    cap->bar = proxy->modern_mem_bar;
> +    cap->bar = bar;
>      cap->offset = cpu_to_le32(region->offset);
>      cap->length = cpu_to_le32(region->size);
>      virtio_pci_add_mem_cap(proxy, cap);
> +
> +}
> +
> +static void virtio_pci_modern_mem_region_map(VirtIOPCIProxy *proxy,
> +                                             VirtIOPCIRegion *region,
> +                                             struct virtio_pci_cap *cap)
> +{
> +    virtio_pci_modern_region_map(proxy, region, cap,
> +                                 &proxy->modern_bar, proxy->modern_mem_bar);
>  }
>  
> -static void virtio_pci_modern_region_unmap(VirtIOPCIProxy *proxy,
> -                                           VirtIOPCIRegion *region)
> +static void virtio_pci_modern_io_region_map(VirtIOPCIProxy *proxy,
> +                                            VirtIOPCIRegion *region,
> +                                            struct virtio_pci_cap *cap)
> +{
> +    virtio_pci_modern_region_map(proxy, region, cap,
> +                                 &proxy->io_bar, proxy->modern_io_bar);
> +}
> +
> +static void virtio_pci_modern_mem_region_unmap(VirtIOPCIProxy *proxy,
> +                                               VirtIOPCIRegion *region)
>  {
>      memory_region_del_subregion(&proxy->modern_bar,
>                                  &region->mr);
>  }
>  
> +static void virtio_pci_modern_io_region_unmap(VirtIOPCIProxy *proxy,
> +                                              VirtIOPCIRegion *region)
> +{
> +    memory_region_del_subregion(&proxy->io_bar,
> +                                &region->mr);
> +}
> +
>  /* This is called by virtio-bus just after the device is plugged. */
>  static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>  {
> @@ -1492,6 +1553,7 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>      VirtioBusState *bus = &proxy->bus;
>      bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY);
>      bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
> +    bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY;
>      uint8_t *config;
>      uint32_t size;
>      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> @@ -1530,16 +1592,31 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>              .cap.cap_len = sizeof cfg,
>              .cap.cfg_type = VIRTIO_PCI_CAP_PCI_CFG,
>          };
> -        struct virtio_pci_cfg_cap *cfg_mask;
> +        struct virtio_pci_notify_cap notify_pio = {
> +            .cap.cap_len = sizeof notify,
> +            .notify_off_multiplier = cpu_to_le32(0x0),
> +        };
>  
> -        /* TODO: add io access for speed */
> +        struct virtio_pci_cfg_cap *cfg_mask;
>  
>          virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
>          virtio_pci_modern_regions_init(proxy);
> -        virtio_pci_modern_region_map(proxy, &proxy->common, &cap);
> -        virtio_pci_modern_region_map(proxy, &proxy->isr, &cap);
> -        virtio_pci_modern_region_map(proxy, &proxy->device, &cap);
> -        virtio_pci_modern_region_map(proxy, &proxy->notify, &notify.cap);
> +
> +        virtio_pci_modern_mem_region_map(proxy, &proxy->common, &cap);
> +        virtio_pci_modern_mem_region_map(proxy, &proxy->isr, &cap);
> +        virtio_pci_modern_mem_region_map(proxy, &proxy->device, &cap);
> +        virtio_pci_modern_mem_region_map(proxy, &proxy->notify, &notify.cap);
> +
> +        if (modern_pio) {
> +            memory_region_init(&proxy->io_bar, OBJECT(proxy),
> +                               "virtio-pci-io", 0x4);
> +
> +            pci_register_bar(&proxy->pci_dev, proxy->modern_io_bar,
> +                             PCI_BASE_ADDRESS_SPACE_IO, &proxy->io_bar);
> +
> +            virtio_pci_modern_io_region_map(proxy, &proxy->notify_pio,
> +                                            &notify_pio.cap);
> +        }
>  
>          pci_register_bar(&proxy->pci_dev, proxy->modern_mem_bar,
>                           PCI_BASE_ADDRESS_SPACE_MEMORY |
> @@ -1592,14 +1669,18 @@ static void virtio_pci_device_unplugged(DeviceState *d)
>  {
>      VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
>      bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
> +    bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY;
>  
>      virtio_pci_stop_ioeventfd(proxy);
>  
>      if (modern) {
> -        virtio_pci_modern_region_unmap(proxy, &proxy->common);
> -        virtio_pci_modern_region_unmap(proxy, &proxy->isr);
> -        virtio_pci_modern_region_unmap(proxy, &proxy->device);
> -        virtio_pci_modern_region_unmap(proxy, &proxy->notify);
> +        virtio_pci_modern_mem_region_unmap(proxy, &proxy->common);
> +        virtio_pci_modern_mem_region_unmap(proxy, &proxy->isr);
> +        virtio_pci_modern_mem_region_unmap(proxy, &proxy->device);
> +        virtio_pci_modern_mem_region_unmap(proxy, &proxy->notify);
> +        if (modern_pio) {
> +            virtio_pci_modern_io_region_unmap(proxy, &proxy->notify_pio);
> +        }
>      }
>  }
>  
> @@ -1619,6 +1700,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>       */
>      proxy->legacy_io_bar  = 0;
>      proxy->msix_bar       = 1;
> +    proxy->modern_io_bar  = 2;
>      proxy->modern_mem_bar = 4;
>  
>      proxy->common.offset = 0x0;
> @@ -1638,6 +1720,10 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>          QEMU_VIRTIO_PCI_QUEUE_MEM_MULT * VIRTIO_QUEUE_MAX;
>      proxy->notify.type = VIRTIO_PCI_CAP_NOTIFY_CFG;
>  
> +    proxy->notify_pio.offset = 0x0;
> +    proxy->notify_pio.size = 0x4;
> +    proxy->notify_pio.type = VIRTIO_PCI_CAP_NOTIFY_CFG;
> +
>      /* subclasses can enforce modern, so do this unconditionally */
>      memory_region_init(&proxy->modern_bar, OBJECT(proxy), "virtio-pci",
>                         2 * QEMU_VIRTIO_PCI_QUEUE_MEM_MULT *
> @@ -1684,6 +1770,8 @@ static Property virtio_pci_properties[] = {
>                      VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, true),
>      DEFINE_PROP_BIT("migrate-modern", VirtIOPCIProxy, flags,
>                      VIRTIO_PCI_FLAG_MIGRATE_MODERN_BIT, true),
> +    DEFINE_PROP_BIT("modern-pio-notify", VirtIOPCIProxy, flags,
> +                    VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index fd2e889..491edfd 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -79,6 +79,13 @@ typedef struct VirtioBusClass VirtioPCIBusClass;
>  #define VIRTIO_PCI_FLAG_MIGRATE_MODERN_BIT 4
>  #define VIRTIO_PCI_FLAG_MIGRATE_MODERN (1 << VIRTIO_PCI_FLAG_MIGRATE_MODERN_BIT)
>  
> +/* have pio notification for modern device ? */
> +#define VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT 5
> +#define VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY \
> +    (1 << VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT)
> +
> +
> +
>  typedef struct {
>      MSIMessage msg;
>      int virq;
> @@ -123,11 +130,14 @@ struct VirtIOPCIProxy {
>      VirtIOPCIRegion isr;
>      VirtIOPCIRegion device;
>      VirtIOPCIRegion notify;
> +    VirtIOPCIRegion notify_pio;
>      MemoryRegion modern_bar;
> +    MemoryRegion io_bar;
>      MemoryRegion modern_cfg;
>      AddressSpace modern_as;
>      uint32_t legacy_io_bar;
>      uint32_t msix_bar;
> +    uint32_t modern_io_bar;
>      uint32_t modern_mem_bar;
>      int config_cap;
>      uint32_t flags;
> -- 
> 2.1.4