From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44160) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YYTAL-0008GH-U4 for qemu-devel@nongnu.org; Thu, 19 Mar 2015 01:38:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YYTAJ-0006KL-Ov for qemu-devel@nongnu.org; Thu, 19 Mar 2015 01:38:45 -0400 Date: Thu, 19 Mar 2015 13:23:56 +0800 From: Jason Wang Message-Id: <1426742636.5002.4@smtp.corp.redhat.com> In-Reply-To: <20150318135321-mutt-send-email-mst@redhat.com> References: <1426671309-13645-1-git-send-email-jasowang@redhat.com> <1426671309-13645-20-git-send-email-jasowang@redhat.com> <20150318135321-mutt-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Subject: Re: [Qemu-devel] [PATCH V4 19/19] virtio-pci: introduce auto_msix_bar_size property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Alexander Graf , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, cornelia.huck@de.ibm.com, Paolo Bonzini , Richard Henderson On Wed, Mar 18, 2015 at 8:57 PM, Michael S. Tsirkin 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 >> Cc: Richard Henderson >> Cc: Michael S. Tsirkin >> Cc: Alexander Graf >> Cc: qemu-ppc@nongnu.org >> Signed-off-by: Jason Wang > > 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(). > > >> @@ -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