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

[Laine Stump]

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
>