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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx>
>
> 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;
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
