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@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;
+
+ 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;
--
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:
> 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@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@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;
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2018-04-03 at 19:13 -0400, John Ferlan wrote:
[...]
> > 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 ;-).
I don't think you missed something, both version should work just
as fine. I happen to find my version easier to read than yours, so
I'd stick with that if you're okay with it - I guess it's just a
matter of preference at the end of the day...
--
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 05:04 AM, Andrea Bolognani wrote:
> On Tue, 2018-04-03 at 19:13 -0400, John Ferlan wrote:
> [...]
>>> 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 ;-).
>
> I don't think you missed something, both version should work just
> as fine. I happen to find my version easier to read than yours, so
> I'd stick with that if you're okay with it - I guess it's just a
> matter of preference at the end of the day...
>
I guess part of me is thinking of the "next" bridge that gets added
where someone just copies what you did and creates a needXXXToPCIBridge
boolean, adds another setting to true, another setting to false, and
then needs to decide which to prefer and thus has to muck with setting
the others based on the new one.
In the long run - it seems the setting of the bool is based on need,
then the second part is to decide what type of thing to add. That's why
I thought it would be easier to read with what I presented.
Again I'm fine with the way you had it, so it's your call.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.