1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH v2 0/4] qemu: Add support for generic PCIe Root Ports
  4. View patch

[libvirt] [PATCH v2 2/4] qemu: Use generic PCIe Root Ports by default when available

Andrea Bolognani posted 4 patches 8 years, 10 months ago
[libvirt] [PATCH v2 2/4] qemu: Use generic PCIe Root Ports by default when available
Posted by Andrea Bolognani 8 years, 10 months ago 
ioh3420 is emulated Intel hardware, so it always looked
quite out of place in aarch64/virt guests. Even for x86/q35
guests, the recently-introduced pcie-root-port is a better
choice because, unlike ioh3420, it doesn't require IO space
(a farily constrained resource) to work.

If pcie-root-port is available in QEMU, use it; ioh3420 is
still used as fallback for when pcie-root-port is not
available.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1408808
---
Changes from v1:

  * Always use pcie-root-port if available, regardless of
    machine type.

 src/qemu/qemu_domain_address.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 64aa4ef..6d3a627 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1846,13 +1846,15 @@ qemuDomainSupportsPCI(virDomainDefPtr def,
 
 
 static void
-qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont)
+qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont,
+                                           virQEMUCapsPtr qemuCaps)
 {
     int *modelName = &cont->opts.pciopts.modelName;
 
     /* make sure it's not already set */
     if (*modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE)
         return;
+
     switch ((virDomainControllerModelPCI)cont->model) {
     case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
         *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE;
@@ -1861,7 +1863,12 @@ qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont)
         *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE;
         break;
     case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
-        *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420;
+        /* Use generic PCIe Root Ports if available, falling back to
+         * ioh3420 otherwise */
+        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT))
+            *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT;
+        else
+            *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420;
         break;
     case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
         *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM;
@@ -2143,7 +2150,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
              * device in qemu) for any controller that doesn't yet
              * have it set.
              */
-            qemuDomainPCIControllerSetDefaultModelName(cont);
+            qemuDomainPCIControllerSetDefaultModelName(cont, qemuCaps);
 
             /* set defaults for any other auto-generated config
              * options for this controller that haven't been
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/4] qemu: Use generic PCIe Root Ports by default when available
Posted by Laine Stump 8 years, 10 months ago 
On 03/16/2017 01:14 PM, Andrea Bolognani wrote:
> ioh3420 is emulated Intel hardware, so it always looked
> quite out of place in aarch64/virt guests. Even for x86/q35
> guests, the recently-introduced pcie-root-port is a better
> choice because, unlike ioh3420, it doesn't require IO space
> (a farily constrained resource) to work.

s/farily/fairly/

(my understanding is that there is still an issue with the "disable IO
space bit - apparently both Linux and Windows will still reserve and
even *use* IO space even if the controller says that it doesn't support
it! But at least *in theory* it won't use it :-P)


> 
> If pcie-root-port is available in QEMU, use it; ioh3420 is
> still used as fallback for when pcie-root-port is not
> available.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1408808
> ---
> Changes from v1:
> 
>   * Always use pcie-root-port if available, regardless of
>     machine type.
> 
>  src/qemu/qemu_domain_address.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)


ACK, although..... (see below)

> 
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index 64aa4ef..6d3a627 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -1846,13 +1846,15 @@ qemuDomainSupportsPCI(virDomainDefPtr def,
>  
>  
>  static void
> -qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont)
> +qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont,
> +                                           virQEMUCapsPtr qemuCaps)
>  {
>      int *modelName = &cont->opts.pciopts.modelName;
>  
>      /* make sure it's not already set */
>      if (*modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE)
>          return;
> +
>      switch ((virDomainControllerModelPCI)cont->model) {
>      case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
>          *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE;
> @@ -1861,7 +1863,12 @@ qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont)
>          *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE;
>          break;
>      case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
> -        *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420;
> +        /* Use generic PCIe Root Ports if available, falling back to
> +         * ioh3420 otherwise */
> +        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT))
> +            *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT;
> +        else
> +            *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420;

I wonder if we should check caps for IOH3420 here just to be consistent
(and log an error if neither is available). I realize that's not the way
it worked before (existing code only checks the caps for a particular
device at the time we generate the commandline), but I'll be the first
to admit my original code was, err, "less than ideal".

It's up to you though, add it or not.


>          break;
>      case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
>          *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM;
> @@ -2143,7 +2150,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>               * device in qemu) for any controller that doesn't yet
>               * have it set.
>               */
> -            qemuDomainPCIControllerSetDefaultModelName(cont);
> +            qemuDomainPCIControllerSetDefaultModelName(cont, qemuCaps);
>  
>              /* set defaults for any other auto-generated config
>               * options for this controller that haven't been
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/4] qemu: Use generic PCIe Root Ports by default when available
Posted by Andrea Bolognani 8 years, 10 months ago 
On Thu, 2017-03-16 at 18:30 -0400, Laine Stump wrote:
> > @@ -1861,7 +1863,12 @@ qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont)
> >          *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE;
> >          break;
> >      case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
> > -        *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420;
> > +        /* Use generic PCIe Root Ports if available, falling back to
> > +         * ioh3420 otherwise */
> > +        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT))
> > +            *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT;
> > +        else
> > +            *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420;
> 
> I wonder if we should check caps for IOH3420 here just to be consistent
> (and log an error if neither is available). I realize that's not the way
> it worked before (existing code only checks the caps for a particular
> device at the time we generate the commandline), but I'll be the first
> to admit my original code was, err, "less than ideal".
> 
> It's up to you though, add it or not.

We already check when building the QEMU command line, which
is the appropriate place IMHO. I'd rather not duplicate the
check here as well.


I've pushed the patches now, thanks for reviewing!

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/4] qemu: Use generic PCIe Root Ports by default when available
Posted by Laine Stump 8 years, 10 months ago 
On 03/17/2017 05:06 AM, Andrea Bolognani wrote:
> On Thu, 2017-03-16 at 18:30 -0400, Laine Stump wrote:
>>> @@ -1861,7 +1863,12 @@ qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont)
>>>           *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE;
>>>           break;
>>>       case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
>>> -        *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420;
>>> +        /* Use generic PCIe Root Ports if available, falling back to
>>> +         * ioh3420 otherwise */
>>> +        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT))
>>> +            *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT;
>>> +        else
>>> +            *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420;
>>  
>> I wonder if we should check caps for IOH3420 here just to be consistent
>> (and log an error if neither is available). I realize that's not the way
>> it worked before (existing code only checks the caps for a particular
>> device at the time we generate the commandline), but I'll be the first
>> to admit my original code was, err, "less than ideal".
>>  
>> It's up to you though, add it or not.
> 
> We already check when building the QEMU command line, which
> is the appropriate place IMHO. I'd rather not duplicate the
> check here as well.

It all depends on whether you want to get an error when you define the
domain, or not until you try to start it. I prefer the latter, but in
this case it's all just academic, since we know that all qemu binaries
we recognize as having a PCIe root bus also support the ioh3420.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.