| Message ID | 1468953778-15295-1-git-send-email-marcel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 19, 2016 at 09:42:58PM +0300, Marcel Apfelbaum wrote: > Modern machines are expected to be used by newer setups with > modern guests aiming the use of the latest features. > > Enable modern and disable legacy for virtio devices > plugged into PCIe ports (Root ports or Downstream ports). > Using the Virtio 1 mode will remove the limitation > of the number of devices that can be attached to a machine > by removing the need for the IO BAR. > > Convert 'disable-modern' and 'disable-legacy' properties to OnOffAuto > with default Auto. > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > > Hi, > > v1 -> v2: > - Stick to existing defaults for old machine types (Michael S. Tsirkin) > > If everyone agrees, I am thinking about getting it into 2.7 > to avoid the ~15 virtio devices limitation per machine. > > Notes: > - The non PCIe machines behaviour should remain the same. > - I hope is OK to make the disable-* properties OnOffAuto. Previous setups > using them can be affected, why? could you explain pls? isn't onoffauto compatible with bit properties? > but libvirt is not using them yet (as far as I know) > - My tests were limited to checking all possible disable-* configurations (and make check for all archs) > > Thanks, > Marcel > > hw/virtio/virtio-pci.c | 31 ++++++++++++++++++++++++------- > hw/virtio/virtio-pci.h | 2 ++ > include/hw/compat.h | 8 ++++++++ > 3 files changed, 34 insertions(+), 7 deletions(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 2b34b43..ec9e84f 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1716,6 +1716,8 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) > { > VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev); > VirtioPCIClass *k = VIRTIO_PCI_GET_CLASS(pci_dev); > + bool pcie_port = (pci_bus_is_express(pci_dev->bus) && > + !pci_bus_is_root(pci_dev->bus)); drop the extra outside () please. > > /* > * virtio pci bar layout used by default. > @@ -1766,8 +1768,23 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) > > address_space_init(&proxy->modern_as, &proxy->modern_cfg, "virtio-pci-cfg-as"); > > - if (pci_is_express(pci_dev) && pci_bus_is_express(pci_dev->bus) && > - !pci_bus_is_root(pci_dev->bus)) { > + if ((pcie_port && (proxy->disable_modern == ON_OFF_AUTO_AUTO)) > + || (proxy->disable_modern == ON_OFF_AUTO_OFF)) { drop the () around == - logic and math mix naturally in C. also, pls put || at end of line, not at the beginning of continuation. this way you can see there is continuation as you read the code naturally. > + proxy->flags &= ~VIRTIO_PCI_FLAG_DISABLE_MODERN; > + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > + } else { > + proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_MODERN; > + pci_dev->cap_present &= ~QEMU_PCI_CAP_EXPRESS; > + } > + > + if ((pcie_port && (proxy->disable_legacy == ON_OFF_AUTO_AUTO)) > + || (proxy->disable_legacy == ON_OFF_AUTO_ON)) { > + proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_LEGACY; > + } else { > + proxy->flags &= ~VIRTIO_PCI_FLAG_DISABLE_LEGACY; > + } > + So this is a bit messy. Can we do: if (pcie_port) default_disable_legacy = false; else default_disable_legacy = true; now if (proxy->disable_legacy == ON_OFF_AUTO_ON || (proxy->disable_legacy == ON_OFF_AUTO_AUTO && default_disable_legacy)) proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_LEGACY; else proxy->flags &= ~VIRTIO_PCI_FLAG_DISABLE_LEGACY; ? Also, I wonder how does this interact with devices that play with these flags themselves, like virtio gpu? I guess we could just set VIRTIO_PCI_FLAG_DISABLE_LEGACY, avoid clearing it. Setting disable modern might be problematic for same reason, not sure what to do about that one. Did I miss anything? > + if (pcie_port && pci_is_express(pci_dev)) { > int pos; > > pos = pcie_endpoint_cap_init(pci_dev, 0); > @@ -1821,10 +1838,10 @@ static void virtio_pci_reset(DeviceState *qdev) > static Property virtio_pci_properties[] = { > DEFINE_PROP_BIT("virtio-pci-bus-master-bug-migration", VirtIOPCIProxy, flags, > VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT, false), > - DEFINE_PROP_BIT("disable-legacy", VirtIOPCIProxy, flags, > - VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, false), > - DEFINE_PROP_BIT("disable-modern", VirtIOPCIProxy, flags, > - VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, true), > + DEFINE_PROP_ON_OFF_AUTO("disable-legacy", VirtIOPCIProxy, disable_legacy, > + ON_OFF_AUTO_AUTO), > + DEFINE_PROP_ON_OFF_AUTO("disable-modern", VirtIOPCIProxy, disable_modern, > + ON_OFF_AUTO_AUTO), > DEFINE_PROP_BIT("migrate-extra", VirtIOPCIProxy, flags, > VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT, true), > DEFINE_PROP_BIT("modern-pio-notify", VirtIOPCIProxy, flags, > @@ -1841,7 +1858,7 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp) > PCIDevice *pci_dev = &proxy->pci_dev; > > if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) && > - !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)) { > + !(proxy->disable_modern == ON_OFF_AUTO_ON)) { > pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > } > > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h > index e4548c2..4f219d4 100644 > --- a/hw/virtio/virtio-pci.h > +++ b/hw/virtio/virtio-pci.h > @@ -144,6 +144,8 @@ struct VirtIOPCIProxy { > uint32_t modern_mem_bar; > int config_cap; > uint32_t flags; > + OnOffAuto disable_modern; > + OnOffAuto disable_legacy; > uint32_t class_code; > uint32_t nvectors; > uint32_t dfselect; > diff --git a/include/hw/compat.h b/include/hw/compat.h > index 9914e7a..1531399 100644 > --- a/include/hw/compat.h > +++ b/include/hw/compat.h > @@ -6,6 +6,14 @@ > .driver = "virtio-mmio",\ > .property = "format_transport_address",\ > .value = "off",\ > + },{\ > + .driver = "virtio-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > }, > > #define HW_COMPAT_2_5 \ > -- > 2.4.3
On 07/20/2016 01:56 AM, Michael S. Tsirkin wrote: > On Tue, Jul 19, 2016 at 09:42:58PM +0300, Marcel Apfelbaum wrote: >> Modern machines are expected to be used by newer setups with >> modern guests aiming the use of the latest features. >> >> Enable modern and disable legacy for virtio devices >> plugged into PCIe ports (Root ports or Downstream ports). >> Using the Virtio 1 mode will remove the limitation >> of the number of devices that can be attached to a machine >> by removing the need for the IO BAR. >> >> Convert 'disable-modern' and 'disable-legacy' properties to OnOffAuto >> with default Auto. >> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >> --- >> >> Hi, >> >> v1 -> v2: >> - Stick to existing defaults for old machine types (Michael S. Tsirkin) >> >> If everyone agrees, I am thinking about getting it into 2.7 >> to avoid the ~15 virtio devices limitation per machine. >> >> Notes: >> - The non PCIe machines behaviour should remain the same. >> - I hope is OK to make the disable-* properties OnOffAuto. Previous setups >> using them can be affected, > Hi Michael, > why? could you explain pls? isn't onoffauto compatible with bit > properties? > Yes, indeed. the value of 'Off' is replaced by 'Auto', but the semantics for the names "on/off" remain. > >> but libvirt is not using them yet (as far as I know) >> - My tests were limited to checking all possible disable-* configurations (and make check for all archs) >> >> Thanks, >> Marcel >> >> hw/virtio/virtio-pci.c | 31 ++++++++++++++++++++++++------- >> hw/virtio/virtio-pci.h | 2 ++ >> include/hw/compat.h | 8 ++++++++ >> 3 files changed, 34 insertions(+), 7 deletions(-) >> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c >> index 2b34b43..ec9e84f 100644 >> --- a/hw/virtio/virtio-pci.c >> +++ b/hw/virtio/virtio-pci.c >> @@ -1716,6 +1716,8 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) >> { >> VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev); >> VirtioPCIClass *k = VIRTIO_PCI_GET_CLASS(pci_dev); >> + bool pcie_port = (pci_bus_is_express(pci_dev->bus) && >> + !pci_bus_is_root(pci_dev->bus)); > > > drop the extra outside () please. > OK >> >> /* >> * virtio pci bar layout used by default. >> @@ -1766,8 +1768,23 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) >> >> address_space_init(&proxy->modern_as, &proxy->modern_cfg, "virtio-pci-cfg-as"); >> >> - if (pci_is_express(pci_dev) && pci_bus_is_express(pci_dev->bus) && >> - !pci_bus_is_root(pci_dev->bus)) { >> + if ((pcie_port && (proxy->disable_modern == ON_OFF_AUTO_AUTO)) >> + || (proxy->disable_modern == ON_OFF_AUTO_OFF)) { > > drop the () around == - logic and math mix naturally in C. > OK > also, pls put || at end of line, not at the beginning of > continuation. this way you can see there is > continuation as you read the code naturally. > The line length would be more than 80, so I needed to chose a way to "lose". I'll add || at the end of the line, sure. > > >> + proxy->flags &= ~VIRTIO_PCI_FLAG_DISABLE_MODERN; >> + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; >> + } else { >> + proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_MODERN; >> + pci_dev->cap_present &= ~QEMU_PCI_CAP_EXPRESS; >> + } >> + >> + if ((pcie_port && (proxy->disable_legacy == ON_OFF_AUTO_AUTO)) >> + || (proxy->disable_legacy == ON_OFF_AUTO_ON)) { >> + proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_LEGACY; >> + } else { >> + proxy->flags &= ~VIRTIO_PCI_FLAG_DISABLE_LEGACY; >> + } >> + > > So this is a bit messy. > > Can we do: > > if (pcie_port) > default_disable_legacy = false; > else > default_disable_legacy = true; > > now > > if (proxy->disable_legacy == ON_OFF_AUTO_ON || > (proxy->disable_legacy == ON_OFF_AUTO_AUTO && default_disable_legacy)) > > proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_LEGACY; > else > proxy->flags &= ~VIRTIO_PCI_FLAG_DISABLE_LEGACY; > > ? > We have 2 *different* logic: -disable-modern: make it *false* if plugged into a pcie_port or the user requested it. In this case update QEMU_PCI_CAP_EXPRESS flag accordingly. -disble-legacy: make it *true* if plugged into a pcie_port or the user requested it. If the code seems complicated I'll try to make it more readable. Maybe I'll add a switch for each flag. > > Also, I wonder how does this interact with devices that play with > these flags themselves, like virtio gpu? > I guess we could just set VIRTIO_PCI_FLAG_DISABLE_LEGACY, avoid clearing it. > I thought this method is called before any other init call sites. (like virtio gpu or serial) I'll double-check and update accordingly. > Setting disable modern might be problematic for same reason, not sure what > to do about that one. > > Did I miss anything? I'll check this code runs before the other specific devices init code. If it runs before, we have no problem here, otherwise I'll need to re-think this patch. Thank you for the review and I'll provide the answers in the next version. Marcel > >> + if (pcie_port && pci_is_express(pci_dev)) { >> int pos; >> >> pos = pcie_endpoint_cap_init(pci_dev, 0); >> @@ -1821,10 +1838,10 @@ static void virtio_pci_reset(DeviceState *qdev) >> static Property virtio_pci_properties[] = { >> DEFINE_PROP_BIT("virtio-pci-bus-master-bug-migration", VirtIOPCIProxy, flags, >> VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT, false), >> - DEFINE_PROP_BIT("disable-legacy", VirtIOPCIProxy, flags, >> - VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, false), >> - DEFINE_PROP_BIT("disable-modern", VirtIOPCIProxy, flags, >> - VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, true), >> + DEFINE_PROP_ON_OFF_AUTO("disable-legacy", VirtIOPCIProxy, disable_legacy, >> + ON_OFF_AUTO_AUTO), >> + DEFINE_PROP_ON_OFF_AUTO("disable-modern", VirtIOPCIProxy, disable_modern, >> + ON_OFF_AUTO_AUTO), >> DEFINE_PROP_BIT("migrate-extra", VirtIOPCIProxy, flags, >> VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT, true), >> DEFINE_PROP_BIT("modern-pio-notify", VirtIOPCIProxy, flags, >> @@ -1841,7 +1858,7 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp) >> PCIDevice *pci_dev = &proxy->pci_dev; >> >> if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) && >> - !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)) { >> + !(proxy->disable_modern == ON_OFF_AUTO_ON)) { >> pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; >> } >> >> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h >> index e4548c2..4f219d4 100644 >> --- a/hw/virtio/virtio-pci.h >> +++ b/hw/virtio/virtio-pci.h >> @@ -144,6 +144,8 @@ struct VirtIOPCIProxy { >> uint32_t modern_mem_bar; >> int config_cap; >> uint32_t flags; >> + OnOffAuto disable_modern; >> + OnOffAuto disable_legacy; >> uint32_t class_code; >> uint32_t nvectors; >> uint32_t dfselect; >> diff --git a/include/hw/compat.h b/include/hw/compat.h >> index 9914e7a..1531399 100644 >> --- a/include/hw/compat.h >> +++ b/include/hw/compat.h >> @@ -6,6 +6,14 @@ >> .driver = "virtio-mmio",\ >> .property = "format_transport_address",\ >> .value = "off",\ >> + },{\ >> + .driver = "virtio-pci",\ >> + .property = "disable-modern",\ >> + .value = "on",\ >> + },{\ >> + .driver = "virtio-pci",\ >> + .property = "disable-legacy",\ >> + .value = "off",\ >> }, >> >> #define HW_COMPAT_2_5 \ >> -- >> 2.4.3
> > Notes: > > - The non PCIe machines behaviour should remain the same. > > - I hope is OK to make the disable-* properties OnOffAuto. Previous setups > > using them can be affected, > > why? could you explain pls? isn't onoffauto compatible with bit > properties? I have the same question ;) > Also, I wonder how does this interact with devices that play with > these flags themselves, like virtio gpu? virtio input forces virtio 1.0 only too. I think they must be changed to set the new property variables instead. > I guess we could just set VIRTIO_PCI_FLAG_DISABLE_LEGACY, avoid clearing it. I think we should simply drop the flags and use bool variables instead. Also while being at it: Should we enable modern by default for all pci devices? cheers, Gerd
On 07/20/2016 11:41 AM, Gerd Hoffmann wrote: >>> Notes: >>> - The non PCIe machines behaviour should remain the same. >>> - I hope is OK to make the disable-* properties OnOffAuto. Previous setups >>> using them can be affected, >> >> why? could you explain pls? isn't onoffauto compatible with bit >> properties? > > I have the same question ;) > Hi Gerd, As I explained to Michael, I saw the 'off' value goes from 0 to 2, bu the name remains the same... :) I wasn't sure if is possible to use 0/1 instead of on/off, then we would have a problem. >> Also, I wonder how does this interact with devices that play with >> these flags themselves, like virtio gpu? > > virtio input forces virtio 1.0 only too. > I think they must be changed to set the new property variables instead. > I can do that. I preferred a minimalist approach since we are in "hard-freeze" and I wanted to make the change for 2.7 . >> I guess we could just set VIRTIO_PCI_FLAG_DISABLE_LEGACY, avoid clearing it. > > I think we should simply drop the flags and use bool variables instead. > You mean OnOffAuto new fields, right? We would need a wrapper since "auto" mode is not an actual value. But is doable, of course. > Also while being at it: Should we enable modern by default for all pci > devices? > I am not against it, if Michael approves I'll change that too, even for 2.7 Thanks, Marcel > cheers, > Gerd >
On Tue, 19 Jul 2016 21:42:58 +0300 Marcel Apfelbaum <marcel@redhat.com> wrote: > Modern machines are expected to be used by newer setups with > modern guests aiming the use of the latest features. > > Enable modern and disable legacy for virtio devices > plugged into PCIe ports (Root ports or Downstream ports). > Using the Virtio 1 mode will remove the limitation > of the number of devices that can be attached to a machine > by removing the need for the IO BAR. Stupid question: Does this limitation show up for legacy and transitional, but not for modern? Would it make sense then to default to modern for PCIe and transitional for non-PCIe? (The term "virtio-1" mode always confuses me a bit; this may be because ccw unlike pci does not have modern-only devices - and even if we had, basically the only difference would be that we'd disallow devices without the VERSION_1 feature.)
On 07/20/2016 12:16 PM, Cornelia Huck wrote: > On Tue, 19 Jul 2016 21:42:58 +0300 > Marcel Apfelbaum <marcel@redhat.com> wrote: > >> Modern machines are expected to be used by newer setups with >> modern guests aiming the use of the latest features. >> >> Enable modern and disable legacy for virtio devices >> plugged into PCIe ports (Root ports or Downstream ports). >> Using the Virtio 1 mode will remove the limitation >> of the number of devices that can be attached to a machine >> by removing the need for the IO BAR. > Hi Cornelia, > Stupid question: Does this limitation show up for legacy and > transitional, but not for modern? > Yes, with PCIe we need to disable the IO Bars. Here is a short explanation: The root cause it the PCIe architecture being "point to point" rather than 'shared bus'. Each PCIe port supports only one device (multiple functions though) but is exposed as a PCI bridge. The firmware will assign a 4k IO window for each bridge if a device with IO BARs is attached to it. So the firmware will allocate a 4K IO range for each PCIe port -> for each device. Since the IO space is pretty limited we can support around 15 devices with IO BARs attached to PCIe ports. There are other ways to deal with the limitation like tweaking the firmware to assign a smaller IO window (no PCI compliant, but it should work) Looking only at the virtio scope, disabling legacy and enabling modern should be enough. > Would it make sense then to default to modern for PCIe and transitional > for non-PCIe? > Yes. this patch only does the first part (deals only with the PCIe limitation), but the next version will also include 'transitional' virtio as default for non PCIe slots. Thanks, Marcel > (The term "virtio-1" mode always confuses me a bit; this may be because > ccw unlike pci does not have modern-only devices - and even if we had, > basically the only difference would be that we'd disallow devices > without the VERSION_1 feature.) >
Hi, > >> I guess we could just set VIRTIO_PCI_FLAG_DISABLE_LEGACY, avoid clearing it. > > > > I think we should simply drop the flags and use bool variables instead. > > > > You mean OnOffAuto new fields, right? > We would need a wrapper since "auto" mode is not an actual value. > But is doable, of course. I was thinking about new variables, set to true for "on", false for "off", and whatever we want do by default for "auto". But moving that evaluation to small wrapper functions and just call them when needed (which shouldn't be that often) instead of storing the evaluation result in variables is fine too. cheers, Gerd
On Wed, 20 Jul 2016 12:37:44 +0300 Marcel Apfelbaum <marcel@redhat.com> wrote: > On 07/20/2016 12:16 PM, Cornelia Huck wrote: > > On Tue, 19 Jul 2016 21:42:58 +0300 > > Marcel Apfelbaum <marcel@redhat.com> wrote: > > > >> Modern machines are expected to be used by newer setups with > >> modern guests aiming the use of the latest features. > >> > >> Enable modern and disable legacy for virtio devices > >> plugged into PCIe ports (Root ports or Downstream ports). > >> Using the Virtio 1 mode will remove the limitation > >> of the number of devices that can be attached to a machine > >> by removing the need for the IO BAR. > > > > Hi Cornelia, > > > Stupid question: Does this limitation show up for legacy and > > transitional, but not for modern? > > > > Yes, with PCIe we need to disable the IO Bars. > > Here is a short explanation: > The root cause it the PCIe architecture being "point to point" rather than 'shared bus'. > Each PCIe port supports only one device (multiple functions though) but is exposed > as a PCI bridge. The firmware will assign a 4k IO window for each bridge if > a device with IO BARs is attached to it. > > So the firmware will allocate a 4K IO range for each PCIe port -> for each device. > Since the IO space is pretty limited we can support around 15 devices with IO BARs > attached to PCIe ports. > > There are other ways to deal with the limitation like tweaking the firmware > to assign a smaller IO window (no PCI compliant, but it should work) Thanks for the explanation! > > Looking only at the virtio scope, disabling legacy and enabling modern > should be enough. > > > Would it make sense then to default to modern for PCIe and transitional > > for non-PCIe? > > > > Yes. this patch only does the first part (deals only with the PCIe limitation), > but the next version will also include 'transitional' virtio as default for non PCIe slots. OK, sounds sensible. The transport-agnostic virtio-1 code should be pretty sane at this point.
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 2b34b43..ec9e84f 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1716,6 +1716,8 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) { VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev); VirtioPCIClass *k = VIRTIO_PCI_GET_CLASS(pci_dev); + bool pcie_port = (pci_bus_is_express(pci_dev->bus) && + !pci_bus_is_root(pci_dev->bus)); /* * virtio pci bar layout used by default. @@ -1766,8 +1768,23 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) address_space_init(&proxy->modern_as, &proxy->modern_cfg, "virtio-pci-cfg-as"); - if (pci_is_express(pci_dev) && pci_bus_is_express(pci_dev->bus) && - !pci_bus_is_root(pci_dev->bus)) { + if ((pcie_port && (proxy->disable_modern == ON_OFF_AUTO_AUTO)) + || (proxy->disable_modern == ON_OFF_AUTO_OFF)) { + proxy->flags &= ~VIRTIO_PCI_FLAG_DISABLE_MODERN; + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; + } else { + proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_MODERN; + pci_dev->cap_present &= ~QEMU_PCI_CAP_EXPRESS; + } + + if ((pcie_port && (proxy->disable_legacy == ON_OFF_AUTO_AUTO)) + || (proxy->disable_legacy == ON_OFF_AUTO_ON)) { + proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_LEGACY; + } else { + proxy->flags &= ~VIRTIO_PCI_FLAG_DISABLE_LEGACY; + } + + if (pcie_port && pci_is_express(pci_dev)) { int pos; pos = pcie_endpoint_cap_init(pci_dev, 0); @@ -1821,10 +1838,10 @@ static void virtio_pci_reset(DeviceState *qdev) static Property virtio_pci_properties[] = { DEFINE_PROP_BIT("virtio-pci-bus-master-bug-migration", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT, false), - DEFINE_PROP_BIT("disable-legacy", VirtIOPCIProxy, flags, - VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, false), - DEFINE_PROP_BIT("disable-modern", VirtIOPCIProxy, flags, - VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, true), + DEFINE_PROP_ON_OFF_AUTO("disable-legacy", VirtIOPCIProxy, disable_legacy, + ON_OFF_AUTO_AUTO), + DEFINE_PROP_ON_OFF_AUTO("disable-modern", VirtIOPCIProxy, disable_modern, + ON_OFF_AUTO_AUTO), DEFINE_PROP_BIT("migrate-extra", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT, true), DEFINE_PROP_BIT("modern-pio-notify", VirtIOPCIProxy, flags, @@ -1841,7 +1858,7 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp) PCIDevice *pci_dev = &proxy->pci_dev; if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) && - !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)) { + !(proxy->disable_modern == ON_OFF_AUTO_ON)) { pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; } diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h index e4548c2..4f219d4 100644 --- a/hw/virtio/virtio-pci.h +++ b/hw/virtio/virtio-pci.h @@ -144,6 +144,8 @@ struct VirtIOPCIProxy { uint32_t modern_mem_bar; int config_cap; uint32_t flags; + OnOffAuto disable_modern; + OnOffAuto disable_legacy; uint32_t class_code; uint32_t nvectors; uint32_t dfselect; diff --git a/include/hw/compat.h b/include/hw/compat.h index 9914e7a..1531399 100644 --- a/include/hw/compat.h +++ b/include/hw/compat.h @@ -6,6 +6,14 @@ .driver = "virtio-mmio",\ .property = "format_transport_address",\ .value = "off",\ + },{\ + .driver = "virtio-pci",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "virtio-pci",\ + .property = "disable-legacy",\ + .value = "off",\ }, #define HW_COMPAT_2_5 \
Modern machines are expected to be used by newer setups with modern guests aiming the use of the latest features. Enable modern and disable legacy for virtio devices plugged into PCIe ports (Root ports or Downstream ports). Using the Virtio 1 mode will remove the limitation of the number of devices that can be attached to a machine by removing the need for the IO BAR. Convert 'disable-modern' and 'disable-legacy' properties to OnOffAuto with default Auto. Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> --- Hi, v1 -> v2: - Stick to existing defaults for old machine types (Michael S. Tsirkin) If everyone agrees, I am thinking about getting it into 2.7 to avoid the ~15 virtio devices limitation per machine. Notes: - The non PCIe machines behaviour should remain the same. - I hope is OK to make the disable-* properties OnOffAuto. Previous setups using them can be affected, but libvirt is not using them yet (as far as I know) - My tests were limited to checking all possible disable-* configurations (and make check for all archs) Thanks, Marcel hw/virtio/virtio-pci.c | 31 ++++++++++++++++++++++++------- hw/virtio/virtio-pci.h | 2 ++ include/hw/compat.h | 8 ++++++++ 3 files changed, 34 insertions(+), 7 deletions(-)