[libvirt] [PATCH 08/11] conf: Prefer pcie-to-pci-bridge to dmi-to-pci-bridge

[John Ferlan]

On 03/28/2018 10:06 AM, Andrea Bolognani wrote:
> Both pcie-to-pci-bridge and dmi-to-pci-bridge can be used to
> create a traditional PCI topology in a pure PCIe guest such as
> those using the x86_64/q35 or aarch64/virt machine type;
> however, the former should be preferred, as it doesn't need to
> obey limitation of real hardware and is completely
> architecture-agnostic.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1520821
> 
> Signed-off-by: Andrea Bolognani <abologna at redhat.com>
> ---
>  src/conf/domain_addr.c | 59 +++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 49 insertions(+), 10 deletions(-)
> 
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index b0709f8295..8964973e03 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -416,6 +416,7 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs,
>      size_t i;
>      int model;
>      bool needDMIToPCIBridge = false;
> +    bool needPCIeToPCIBridge = false;
>  
>      add = addr->bus - addrs->nbuses + 1;
>      if (add <= 0)
> @@ -436,27 +437,41 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs,
>              model = VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE;
>  
>              /* if there aren't yet any buses that will accept a
> -             * pci-bridge, and the caller is asking for one, we'll need to
> -             * add a dmi-to-pci-bridge first.
> +             * pci-bridge, but we need one for the device's PCI address
> +             * to make sense, it means the guest only has a PCIe topology
> +             * configured so far, and we need to create a traditional PCI
> +             * topology to accomodate the new device.
>               */
>              needDMIToPCIBridge = true;
> +            needPCIeToPCIBridge = true;
>              for (i = 0; i < addrs->nbuses; i++) {
>                  if (addrs->buses[i].flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE) {
>                      needDMIToPCIBridge = false;
> +                    needPCIeToPCIBridge = false;
>                      break;
>                  }
>              }
> -            if (needDMIToPCIBridge && add == 1) {
> +
> +            /* Prefer pcie-to-pci-bridge, fall back to dmi-to-pci-bridge */
> +            if (addrs->isPCIeToPCIBridgeSupported)
> +                needDMIToPCIBridge = false;
> +            else
> +                needPCIeToPCIBridge = false;

The above seems a bit extra work and is a bit hard to read...  Could the
previous for loop change to use a new bool "needXToPCIBridge"...

Then, I think it would just be:

    if (addrs->isPCIeToPCIBridgeSupported)
        needPCIeToPCIBridge = needXToPCIBridge;
    else
        needDMIToPCIBridge = needXToPCIBridge;

with the following just being if (needXToPCIBridge && add == 1)

What you have works, just seems to be overkill or maybe I'm missing
something subtle ;-).  You have my

Reviewed-by: John Ferlan <jferlan at redhat.com>

John


> +
> +            if ((needDMIToPCIBridge || needPCIeToPCIBridge) && add == 1) {
>                  /* We need to add a single pci-bridge to provide the bus
>                   * our legacy PCI device will be plugged into; however, we
>                   * have also determined that there isn't yet any proper
> -                 * place to connect that pci-bridge we're about to add (on
> -                 * a system with pcie-root, that "proper place" would be a
> -                 * dmi-to-pci-bridge". So, to give the pci-bridge a place
> -                 * to connect, we increase the count of buses to add,
> -                 * while also incrementing the bus number in the address
> -                 * for the device (since the pci-bridge will now be at an
> -                 * index 1 higher than the caller had anticipated).
> +                 * place to connect that pci-bridge we're about to add,
> +                 * which means we're dealing with a pure PCIe guest. We
> +                 * need to create a traditional PCI topology, and for that
> +                 * we have two options: dmi-to-pci-bridge + pci-bridge or
> +                 * pcie-root-port + pcie-to-pci-bridge (the latter of which
> +                 * is pretty much a pci-bridge as far as devices attached
> +                 * to it are concerned and as such makes the pci-bridge
> +                 * unnecessary). Either way, there's going to be one more
> +                 * controller than initially expected, and the 'bus' part
> +                 * of the device's address will need to be bumped.
>                   */
>                  add++;
>                  addr->bus++;
> @@ -525,6 +540,30 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs,
>          }
>      }
>  
> +    if (needPCIeToPCIBridge) {
> +        /* We need a pcie-root-port to plug pcie-to-pci-bridge into; however,
> +         * qemuDomainAssignPCIAddresses() will, in some cases, create a dummy
> +         * PCIe device and reserve an address for it in order to leave the
> +         * user with an empty pcie-root-port ready for hotplugging, and if
> +         * we didn't do anything other than adding the pcie-root-port here
> +         * it would be used for that, which we don't want. So we change the
> +         * connect flags to make sure only the pcie-to-pci-bridge will be
> +         * connected to the pcie-root-port we just added, and another one
> +         * will be allocated for the dummy PCIe device later on.
> +         */
> +        if (virDomainPCIAddressBusSetModel(&addrs->buses[i],
> +                                           VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT) < 0) {
> +            return -1;
> +        }
> +        addrs->buses[i].flags = VIR_PCI_CONNECT_TYPE_PCIE_TO_PCI_BRIDGE;
> +        i++;
> +
> +        if (virDomainPCIAddressBusSetModel(&addrs->buses[i++],
> +                                           VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE) < 0) {
> +            return -1;
> +        }
> +    }
> +
>      for (; i < addrs->nbuses; i++) {
>          if (virDomainPCIAddressBusSetModel(&addrs->buses[i], model) < 0)
>              return -1;
>