[libvirt] [PATCH 03/11] conf: Remove dubious code from virDomainPCIAddressSetGrow()

[Laine Stump]

On 04/03/2018 07:12 PM, John Ferlan wrote:
>
> On 03/28/2018 10:06 AM, Andrea Bolognani wrote:
>> I haven't been able to come up with a single scenario in which
>> the code in question would be executed; even if there was one,
>> it would be due to the user specifying a *partial* PCI topology
>> in the guest XML, which is of course entirely unsupportable and
>> thus providing even the slightest hint that doing so is in any
>> way a good idea is actively harmful.
>>
>> Signed-off-by: Andrea Bolognani <abologna at redhat.com>
>> ---
>>  src/conf/domain_addr.c | 9 ---------
>>  1 file changed, 9 deletions(-)
>>
> Since Laine added this code - perhaps calling his name out on the CC
> list will allow him to appear and answer the question. Although it could
> take 3 such utterances (e.g. beetlejuice)...

I would hope that such a reference wouldn't need to be explained :-)


>
> Personally, it seems reasonable and you didn't have to change any test
> output, but PCI connection requirements are black boxes to me. I know I
> cannot plug my US plug into the CZ outlets,

Sure you can - you just need properly sized large bare wires.

> but when it comes to PCI
> connection rules - I'm glad someone else knows them!
>
> A soft and mostly unqualified,
>
> Reviewed-by: John Ferlan <jferlan at redhat.com>
>
> John
>
>> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
>> index 0c914fe25c..18b6f8d588 100644
>> --- a/src/conf/domain_addr.c
>> +++ b/src/conf/domain_addr.c
>> @@ -447,15 +447,6 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs,
>>                  addr->bus++;
>>              }
>>          }
>> -    } else if (flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE &&
>> -               addrs->buses[0].model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
>> -        /* NB: if the root bus is pci-root, and we couldn't find an
>> -         * open place to connect a pci-bridge, then there is nothing
>> -         * we can do (since the only way to gain a new slot that
>> -         * accepts a pci-bridge is to add *a pci-bridge* (which is the
>> -         * reason we're here in the first place!)
>> -         */
>> -        model = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;

This is saying that if the device you need a slot for is a pci-bridge
(or has exactly the same connection requirements as a pci-bridge) and if
the root bus is pcie-root then you need to add a dmi-to-pci-bridge (you
wouldn't be in this function in the first place if you already *had* a
dmi-to-pci-bridge, so there's no need to check if you have one). This
would happen if someone manually added a pci-bridge device to a pure
pcie topology.

Normally I would say that this should stay, as I believe we *should* try
to support partially-specified topologies as much as possible (since
you've dealt with libvirt-qe bugzilla reports, you too know of some of
the odd scenarios they come up with - e.g. removing one controller from
a working config.).

However, currently when an un-addressed pci-bridge is encountered (which
is the only time we should get to this code), the search for a slot that
accepts a pci-bridge will find an empty slot on the *that same
pci-bridge* and we end up logging an error indicating that (I forget the
exact error) - I could *swear* that at some point in the past it wasn't
dead code, and that it actually helped to resolve a bug filed by
libvirt-qe, but experimentation shows that certainly isn't the case now.

In the meantime, if I remove that code (and don't apply any of your
patches) then a pure pcie domain *can* be successfully edited to add a
single pci-bridge (as long as you specify an index, *and* there is an
unused index of a smaller value), but what ends up in the
"proper/validated" config is a pci-bridge that is plugged directly into
a pcie-root-port (which is also wrong, but silently so).

So I guess I reserve my judgement until I see what happens with your
entire series applied, which I'll do tomorrow morning.


>>      } else if (flags & (VIR_PCI_CONNECT_TYPE_PCIE_DEVICE |
>>                          VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT)) {
>>          model = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
>>