Re: [Qemu-devel] [PATCH V4 19/19] virtio-pci: introduce auto_msix_bar_size property

[Jason Wang <jasowang@redhat.com>]

On Thu, Mar 19, 2015 at 6:02 PM, Michael S. Tsirkin <mst@redhat.com> 
wrote:
> On Thu, Mar 19, 2015 at 01:23:56PM +0800, Jason Wang wrote:
>>  
>>  
>>  On Wed, Mar 18, 2015 at 8:57 PM, Michael S. Tsirkin 
>> <mst@redhat.com> wrote:
>>  >On Wed, Mar 18, 2015 at 05:35:09PM +0800, Jason Wang wrote:
>>  >> Currently we don't support more than 128 MSI-X vectors for a pci
>>  >> devices, trying to use vector=129 for a virtio-net-pci device 
>> may get:
>>  >> qemu-system-x86_64: -device 
>> virtio-net-pci,netdev=hn0,vectors=129:
>>  >> unable to init msix vectors to 129
>>  >>   This this because the MSI-X bar size were hard-coded as 4096. 
>> So this
>>  >> patch introduces boolean auto_msix_bar_size property for
>>  >> virito-pci devices. Enable this will let the device calculate 
>> the msix
>>  >> bar size based on the number of MSI-X entries instead of 
>> previous 4096
>>  >> hard-coded limit.
>>  >> This is a must to let virtio-net can up to 256 queues and each 
>> queue
>>  >> were associated with a specific MSI-X entry.
>>  >> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>  >> Cc: Richard Henderson <rth@twiddle.net>
>>  >> Cc: Michael S. Tsirkin <mst@redhat.com>
>>  >> Cc: Alexander Graf <agraf@suse.de>
>>  >> Cc: qemu-ppc@nongnu.org
>>  >> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>  >
>>  >I don't understand what this property does.
>>  >What if I *don't* set auto_msix_bar_size?
>>  >vectors<=128 works exactly like it dif previously, vectors=129 
>> fails?
>>  >Does not seem like useful behaviour, to me.
>>  >
>>  >
>>  >> ---
>>  >>  hw/i386/pc_piix.c      |  8 ++++++++
>>  >>  hw/i386/pc_q35.c       |  8 ++++++++
>>  >>  hw/ppc/spapr.c         | 11 ++++++++++-
>>  >>  hw/virtio/virtio-pci.c | 17 +++++++++++++++--
>>  >>  hw/virtio/virtio-pci.h |  3 +++
>>  >>  include/hw/compat.h    | 11 +++++++++++
>>  >>  6 files changed, 55 insertions(+), 3 deletions(-)
>>  >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>  >> index 0796719..8808500 100644
>>  >> --- a/hw/i386/pc_piix.c
>>  >> +++ b/hw/i386/pc_piix.c
>>  >> @@ -552,6 +552,10 @@ static QEMUMachine pc_i440fx_machine_v2_3 = 
>> {
>>  >>      PC_I440FX_2_3_MACHINE_OPTIONS,
>>  >>      .name = "pc-i440fx-2.3",
>>  >>      .init = pc_init_pci_2_3,
>>  >> +    .compat_props = (GlobalProperty[]) {
>>  >> +        HW_COMPAT_2_3,
>>  >> +        { /* end of list */ }
>>  >> +    },
>>  >>  };
>>  >>  #define PC_I440FX_2_2_MACHINE_OPTIONS 
>> PC_I440FX_2_3_MACHINE_OPTIONS
>>  >> @@ -560,6 +564,10 @@ static QEMUMachine pc_i440fx_machine_v2_2 = 
>> {
>>  >>      PC_I440FX_2_2_MACHINE_OPTIONS,
>>  >>      .name = "pc-i440fx-2.2",
>>  >>      .init = pc_init_pci_2_2,
>>  >> +    .compat_props = (GlobalProperty[]) {
>>  >> +        HW_COMPAT_2_2,
>>  >> +        { /* end of list */ }
>>  >> +    },
>>  >>  };
>>  >>  #define PC_I440FX_2_1_MACHINE_OPTIONS                           
>> \
>>  >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>>  >> index a8a34a4..4a34349 100644
>>  >> --- a/hw/i386/pc_q35.c
>>  >> +++ b/hw/i386/pc_q35.c
>>  >> @@ -448,6 +448,10 @@ static QEMUMachine pc_q35_machine_v2_3 = {
>>  >>      PC_Q35_2_3_MACHINE_OPTIONS,
>>  >>      .name = "pc-q35-2.3",
>>  >>      .init = pc_q35_init_2_3,
>>  >> +    .compat_props = (GlobalProperty[]) {
>>  >> +        HW_COMPAT_2_3,
>>  >> +        { /* end of list */ }
>>  >> +    },
>>  >>  };
>>  >>  #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
>>  >> @@ -456,6 +460,10 @@ static QEMUMachine pc_q35_machine_v2_2 = {
>>  >>      PC_Q35_2_2_MACHINE_OPTIONS,
>>  >>      .name = "pc-q35-2.2",
>>  >>      .init = pc_q35_init_2_2,
>>  >> +    .compat_props = (GlobalProperty[]) {
>>  >> +        HW_COMPAT_2_2,
>>  >> +        { /* end of list */ }
>>  >> +    },
>>  >>  };
>>  >>  #define PC_Q35_2_1_MACHINE_OPTIONS                      \
>>  >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>  >> index 5f25dd3..853a5cc 100644
>>  >> --- a/hw/ppc/spapr.c
>>  >> +++ b/hw/ppc/spapr.c
>>  >> @@ -1794,12 +1794,16 @@ static const TypeInfo spapr_machine_info 
>> = {
>>  >>      },
>>  >>  };
>>  >> +#define SPAPR_COMPAT_2_3 \
>>  >> +        HW_COMPAT_2_3
>>  >> +
>>  >>  #define SPAPR_COMPAT_2_2 \
>>  >> +        SPAPR_COMPAT_2_3, \
>>  >>          {\
>>  >>              .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
>>  >>              .property = "mem_win_size",\
>>  >>              .value    = "0x20000000",\
>>  >> -        }
>>  >> +        } \
>>  >>  #define SPAPR_COMPAT_2_1 \
>>  >>          SPAPR_COMPAT_2_2
>>  >> @@ -1883,10 +1887,15 @@ static const TypeInfo 
>> spapr_machine_2_2_info =
>>  >>{
>>  >>     static void spapr_machine_2_3_class_init(ObjectClass *oc, 
>> void
>>  >>*data)
>>  >>  {
>>  >> +    static GlobalProperty compat_props[] = {
>>  >> +        SPAPR_COMPAT_2_3,
>>  >> +        { /* end of list */ }
>>  >> +    };
>>  >>      MachineClass *mc = MACHINE_CLASS(oc);
>>  >>      mc->name = "pseries-2.3";
>>  >>      mc->desc = "pSeries Logical Partition (PAPR compliant) 
>> v2.3";
>>  >> +    mc->compat_props = compat_props;
>>  >>  }
>>  >>  static const TypeInfo spapr_machine_2_3_info = {
>>  >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>  >> index 4a5febb..f4cd405 100644
>>  >> --- a/hw/virtio/virtio-pci.c
>>  >> +++ b/hw/virtio/virtio-pci.c
>>  >> @@ -925,7 +925,7 @@ static void 
>> virtio_pci_device_plugged(DeviceState
>>  >>*d)
>>  >>      VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
>>  >>      VirtioBusState *bus = &proxy->bus;
>>  >>      uint8_t *config;
>>  >> -    uint32_t size;
>>  >> +    uint32_t size, bar_size;
>>  >>      config = proxy->pci_dev.config;
>>  >>      if (proxy->class_code) {
>>  >> @@ -936,8 +936,19 @@ static void 
>> virtio_pci_device_plugged(DeviceState
>>  >>*d)
>>  >>      pci_set_word(config + PCI_SUBSYSTEM_ID,
>>  >>virtio_bus_get_vdev_id(bus));
>>  >>      config[PCI_INTERRUPT_PIN] = 1;
>>  >> +    if (proxy->flags & VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE) {
>>  >> +        bar_size = proxy->nvectors * PCI_MSIX_ENTRY_SIZE * 2;
>>  >> +        if (bar_size & (bar_size - 1)) {
>>  >> +            bar_size = 1 << qemu_fls(bar_size);
>>  >> +        }
>>  >> +    } else {
>>  >> +        /* For migration compatibility */
>>  >> +        bar_size = 4096;
>>  >> +    }
>>  >> +
>>  >>      if (proxy->nvectors &&
>>  >> -        msix_init_exclusive_bar(&proxy->pci_dev, 
>> proxy->nvectors, 1,
>>  >>4096)) {
>>  >> +        msix_init_exclusive_bar(&proxy->pci_dev, 
>> proxy->nvectors, 1,
>>  >> +                                bar_size)) {
>>  >>          error_report("unable to init msix vectors to %" PRIu32,
>>  >>                       proxy->nvectors);
>>  >>          proxy->nvectors = 0;
>>  >
>>  >
>>  >As I expected, msix format stuff spreads out to virtio.
>>  >Consider "vectors * PCI_MSIX_ENTRY_SIZE * 2"
>>  >That's because you use half the BAR for BIR in msix.c
>>  >So any change will have to be done in two places,
>>  >that's bad.
>>  
>>  Yes, will move to msix.c by passing the compat flags to
>>  msix_init_exclusive_bar().
> 
> I really wonder why isn't just specifying # of vectors enough.
> 

As I replied, since vectors=129 won't get crash in 2.3. We need to keep 
the layout for legacy machine type.

> 
>>  >
>>  >
>>  >> @@ -1370,6 +1381,8 @@ static const TypeInfo 
>> virtio_serial_pci_info = {
>>  >>  static Property virtio_net_properties[] = {
>>  >>      DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
>>  >>                      VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, false),
>>  >> +    DEFINE_PROP_BIT("auto_msix_bar_size", VirtIOPCIProxy, flags,
>>  >> +                    VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT, true),
>>  >>      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
>>  >>      DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
>>  >>      DEFINE_PROP_END_OF_LIST(),
>>  >> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
>>  >> index 3bac016..82a6782 100644
>>  >> --- a/hw/virtio/virtio-pci.h
>>  >> +++ b/hw/virtio/virtio-pci.h
>>  >> @@ -62,6 +62,9 @@ typedef struct VirtioBusClass 
>> VirtioPCIBusClass;
>>  >>   * vcpu thread using ioeventfd for some devices. */
>>  >>  #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1
>>  >>  #define VIRTIO_PCI_FLAG_USE_IOEVENTFD   (1 <<
>>  >>VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)
>>  >> +#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT 2
>>  >> +#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE \
>>  >> +    (1 << VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT)
>>  >>  typedef struct {
>>  >>      MSIMessage msg;
>>  >> diff --git a/include/hw/compat.h b/include/hw/compat.h
>>  >> index 313682a..3186275 100644
>>  >> --- a/include/hw/compat.h
>>  >> +++ b/include/hw/compat.h
>>  >> @@ -1,7 +1,18 @@
>>  >>  #ifndef HW_COMPAT_H
>>  >>  #define HW_COMPAT_H
>>  >> +#define HW_COMPAT_2_3 \
>>  >> +        {\
>>  >> +            .driver   = "virtio-net-pci",\
>>  >> +            .property = "auto_msix_bar_size",\
>>  >> +            .value    = "off",\
>>  >> +        }
>>  >> +
>>  >> +#define HW_COMPAT_2_2 \
>>  >> +        HW_COMPAT_2_3
>>  >> +
>>  >>  #define HW_COMPAT_2_1 \
>>  >> +        HW_COMPAT_2_2, \
>>  >>          {\
>>  >>              .driver   = "intel-hda",\
>>  >>              .property = "old_msi_addr",\
>>  >> --  2.1.0