| Message ID | 1426671309-13645-20-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. > @@ -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
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? 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. > > >> --- >> 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
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(). > > >> @@ -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
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 <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? > > 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
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. > > > > > >> @@ -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
On Thu, Mar 19, 2015 at 6:01 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > 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 >> <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? >> >> 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? I mean migration from 2.4 to 2.3 with e.g vectors=129. > > qemu 2.3 can not run when > 128 vectors are requested. Unfortunately not, qemu can run with just a warning like: qemu-system-x86_64: -device virtio-net-pci,vectors=129: unable to init msix vectors to 129 vectors will reset to zero in this case. > > 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. This is what I want to do. And since vectors=129 won't crash for 2.3. Still need to keep the 4k for compat machine types. > > > >> >> > >> > >> >> --- >> >> 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
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
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; @@ -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",\
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> --- 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(-)