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
> ---
> 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
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
