From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46587) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YYXGq-0006EO-I8 for qemu-devel@nongnu.org; Thu, 19 Mar 2015 06:01:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YYXGm-000832-80 for qemu-devel@nongnu.org; Thu, 19 Mar 2015 06:01:44 -0400 Date: Thu, 19 Mar 2015 11:01:31 +0100 From: "Michael S. Tsirkin" Message-ID: <20150319105643-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> <1426742592.5002.3@smtp.corp.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1426742592.5002.3@smtp.corp.redhat.com> 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: Jason Wang 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 01:23:12PM +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? > > Yes, but there looks like a bug in the code which can lead a bar size less > than 4K. > > > > >Does not seem like useful behaviour, to me. > > This property allows to have more than 128 vectors to be used. We disable > this for legacy machine types to stick bar size to 4K to keep the migration > compatibility. Compatibility between which two configurations? qemu 2.3 can not run when > 128 vectors are requested. So there is no need to worry about what happens when you request > 128 vectors and an old machine type. qemu exiting is not guest visible behaviour. I can imagine two reasonable solutions: 1. increase bar size for everyone. make it 8k for compat machine types simple but wastes memory 2. make bar size depend on # of vectors as # of vectors is user-specified, there's no problem with compatibility, so no need to tweak machine types, but layout is dynamic so more complex. > > > > > > >> --- > >> 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. > > > > > >> @@ -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