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@redhat.com>
---
src/conf/domain_addr.c | 9 ---------
1 file changed, 9 deletions(-)
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;
} 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;
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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)...
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, 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@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;
> } 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@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;
>>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2018-04-03 at 22:44 -0400, Laine Stump wrote:
> On 04/03/2018 07:12 PM, John Ferlan wrote:
> > Since Laine added this code - perhaps calling his name out on the CC
> > list will allow him to appear and answer the question.
Fair point. I kinda just assumed Laine would be the only one
crazy^Wbrave enough to attempt a review for this series :P
> > > - } 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.).
I'm not sure I agree here. There are just way too many corner cases
for us to hope we can deal with all of them reasonably, let alone
without introducing heaps of fragile and difficult to understand
code, and supporting some of them can give users the impression that
*all* of them are going to work, no matter how insane.
I'd rather back the reasonable, supportable use cases with rock-solid
code and error out when the user is asking too much of libvirt.
> 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.
This matches my understanding, and my experience after playing with
it for a fairly long time.
> 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.
Looking forward to that :)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 04/04/2018 04:54 AM, Andrea Bolognani wrote: > On Tue, 2018-04-03 at 22:44 -0400, Laine Stump wrote: > >> 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. > Looking forward to that :) > So it turns out that once all of your patches are applied, the error is the same before or after this code is removed, so I think it may not be executed at all. Since it doesn't solve the issue it originally was intended to solve, and removing it makes no functional difference, I have no problem with you removing it (it would be nice if we could figure out a way to log a more informative error than: unsupported configuration: PCI controller at index 4 (0x04) has bus='0x04', but index must be larger than bus but that's a separate issue.) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, 2018-04-06 at 14:23 -0400, Laine Stump wrote: > So it turns out that once all of your patches are applied, the error is > the same before or after this code is removed, so I think it may not be > executed at all. > > > Since it doesn't solve the issue it originally was intended to solve, > and removing it makes no functional difference, I have no problem with > you removing it (it would be nice if we could figure out a way to log a > more informative error than: > > > unsupported configuration: PCI controller at index 4 (0x04) has > bus='0x04', > > but index must be larger than bus > > > but that's a separate issue.) Yeah, that would be nice indeed. I've pushed the patch in the meantime, thanks for looking at it :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.