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

[Greg Kurz <gkurz@linux.vnet.ibm.com>]

On Mon, 24 Aug 2015 16:52:45 +0200
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> On Fri, 21 Aug 2015 17:05:49 +0800
> Jason Wang <jasowang@redhat.com> 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>
> > ---
> >  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);
> > +            }
> 
> 
> This calls for the following change in memory.c:
> 
>  static void adjust_endianness(MemoryRegion *mr, uint64_t *data, unsigned size)
>  {
> -    if (memory_region_wrong_endianness(mr)) {
> +    if (size && memory_region_wrong_endianness(mr)) {
> 

Oops... this comment was for patch 4/6... Please ignore.

> 
> otherwise we abort on PPC64.
> 
> >          }
> >          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;
> 
>