From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47024) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YYpeV-0007Da-0x for qemu-devel@nongnu.org; Fri, 20 Mar 2015 01:39:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YYpeR-0007dt-QF for qemu-devel@nongnu.org; Fri, 20 Mar 2015 01:39:22 -0400 Date: Fri, 20 Mar 2015 13:38:51 +0800 From: Jason Wang Message-Id: <1426829931.5879.2@smtp.corp.redhat.com> In-Reply-To: <20150319110158-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> <1426742636.5002.4@smtp.corp.redhat.com> <20150319110158-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 Thu, Mar 19, 2015 at 6:02 PM, Michael S. Tsirkin 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 >> 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(). > > 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