[libvirt] [PATCH 0/5] Revert and fix PCI bridge auto-add to extend default bus
Erik Skultety (5): qemu: Remove dead code in qemuDomainAssignPCIAddresses revert patch conf: virDomainDefMaybeAddController tweak return code qemu: Fix auto-adding PCI bridge when all slots are reserved qemu: move PCI slot assignment for PIIX3, Q35 into a separate function qemu: reorder PCI slot assignment functions src/conf/domain_conf.c | 2 +- src/qemu/qemu_command.c | 264 ++++++++++++++++++++++++++++-------------------- 2 files changed, 154 insertions(+), 112 deletions(-) -- 1.9.3
As it turned out, fix of dead code 419a22 changed the affected condition from "never true" to "always true", so better fix would be to change the return code of virDomainMaybeAddController from 0 to 1 if a new bridge has been added, thus distinguishing case when we didn't need to add any controller and case we successfully added one. The return code is changed in the next commit --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c041ee7..cdf02a6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1510,7 +1510,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, i, bus->model)) < 0) goto cleanup; /* If we added a new bridge, we will need one more address */ - if (rv == 0 && + if (rv > 0 && virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0) goto cleanup; } -- 1.9.3
On 01/21/2015 05:49 PM, Erik Skultety wrote:
As it turned out, fix of dead code 419a22 changed the affected condition from "never true" to "always true", so better fix would be to change the return code of virDomainMaybeAddController from 0 to 1 if a new bridge has been added, thus distinguishing case when we didn't need to add any controller and case we successfully added one.
The return code is changed in the next commit --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK and pushed. Jan
Previously the function returned either -1 in case of an error or 0 on success. However, we should also distinguish between a case we successfully added a controller and a case there wasn't a need to add any controller --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8792f5e..9ff3819 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12416,7 +12416,7 @@ virDomainDefMaybeAddController(virDomainDefPtr def, return -1; } - return 0; + return 1; } -- 1.9.3
On 01/21/2015 05:49 PM, Erik Skultety wrote:
Previously the function returned either -1 in case of an error or 0 on success. However, we should also distinguish between a case we successfully added a controller and a case there wasn't a need to add any controller --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK and pushed. Jan
Commit 93c8ca tried to fix the issue with auto-adding of a PCI bridge controller, it worked well when the slots were reserved by devices with user defined addresses without any other devices with unspecified PCI addresses present in the XML. However, if another device with unspecified PCI addresses appeared in the domain's XML, the same problem occurred as the BZ below references. This patch fixes the issue by not creating any additional bridges when not necessary. Now let's say all the buses corresponding to the number of PCI controllers present in the domain's XML got fully reserved by devices with user defined addresses. If there are no other devices present in the XML, no need to create both an additional PCI bus and a bridge controller. If however there are still some PCI devices waiting to get a slot assigned by qemuAssignDevicePCISlots, this means an additional bus needs to be created along with a corresponding bridge controller. This scenario now results in an reasonable error instead of generating wrong qemu command line. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1132900 --- src/qemu/qemu_command.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cdf02a6..99b06ee 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1472,6 +1472,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, int nbuses = 0; size_t i; int rv; + int resflags = 0; + bool buses_reserved = false; + virDomainPCIConnectFlags flags = VIR_PCI_CONNECT_TYPE_PCI; for (i = 0; i < def->ncontrollers; i++) { @@ -1490,17 +1493,22 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, /* 1st pass to figure out how many PCI bridges we need */ if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true))) goto cleanup; + + for (i = 0; i < nbuses; i++) { + if (qemuDomainPCIBusFullyReserved(&addrs->buses[i])) + resflags |= 1 << i; + } + buses_reserved = (resflags && ~((~0) << nbuses)); + if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup; - for (i = 0; i < addrs->nbuses; i++) { - if (!qemuDomainPCIBusFullyReserved(&addrs->buses[i])) { - - /* Reserve 1 extra slot for a (potential) bridge */ - if (virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0) - goto cleanup; - } - } + /* Reserve 1 extra slot for a (potential) bridge only if buses + * are not fully reserved yet + */ + if (!buses_reserved && + virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0) + goto cleanup; for (i = 1; i < addrs->nbuses; i++) { virDomainPCIAddressBusPtr bus = &addrs->buses[i]; @@ -1532,6 +1540,24 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup; } + + for (i = 0; i < def->ncontrollers; i++) { + /* check if every PCI bridge controller's ID is greater than + * the bus it is placed onto + */ + virDomainControllerDefPtr cont = def->controllers[i]; + int idx = cont->idx; + int bus = cont->info.addr.pci.bus; + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE && + idx <= bus) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("failed to create PCI bridge " + "on bus %d: bus is fully reserved"), + bus); + goto cleanup; + } + } } if (obj && obj->privateData) { -- 1.9.3
On 01/21/2015 05:50 PM, Erik Skultety wrote:
Commit 93c8ca tried to fix the issue with auto-adding of a PCI bridge controller, it worked well when the slots were reserved by devices with user defined addresses without any other devices with unspecified PCI addresses present in the XML. However, if another device with unspecified PCI addresses appeared in the domain's XML, the same problem occurred as the BZ below references.
This patch fixes the issue by not creating any additional bridges when not necessary. Now let's say all the buses corresponding to the number of PCI controllers present in the domain's XML got fully reserved by devices with user defined addresses. If there are no other devices present in the XML, no need to create both an additional PCI bus and a bridge controller. If however there are still some PCI devices waiting to get a slot assigned by qemuAssignDevicePCISlots, this means an additional bus needs to be created along with a corresponding bridge controller. This scenario now results in an reasonable error instead of generating wrong qemu command line.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1132900 --- src/qemu/qemu_command.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-)
This breaks the pci-many test case you added before.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cdf02a6..99b06ee 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1472,6 +1472,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, int nbuses = 0; size_t i; int rv; + int resflags = 0; + bool buses_reserved = false; +
If you set this to true by default...
virDomainPCIConnectFlags flags = VIR_PCI_CONNECT_TYPE_PCI;
for (i = 0; i < def->ncontrollers; i++) { @@ -1490,17 +1493,22 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, /* 1st pass to figure out how many PCI bridges we need */ if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true))) goto cleanup; + + for (i = 0; i < nbuses; i++) {
+ if (qemuDomainPCIBusFullyReserved(&addrs->buses[i])) + resflags |= 1 << i;
you can do: if (!fullyReserved(buses[i])) buses_reserved = false; Or just rename the bool to something like 'addExtraEmptySlot' and invert the condition below. (To stay more positive :))
+ } + buses_reserved = (resflags && ~((~0) << nbuses)); + if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup;
- for (i = 0; i < addrs->nbuses; i++) { - if (!qemuDomainPCIBusFullyReserved(&addrs->buses[i])) { - - /* Reserve 1 extra slot for a (potential) bridge */ - if (virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0) - goto cleanup; - } - } + /* Reserve 1 extra slot for a (potential) bridge only if buses + * are not fully reserved yet + */ + if (!buses_reserved && + virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0) + goto cleanup;
for (i = 1; i < addrs->nbuses; i++) { virDomainPCIAddressBusPtr bus = &addrs->buses[i]; @@ -1532,6 +1540,24 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup; } + + for (i = 0; i < def->ncontrollers; i++) { + /* check if every PCI bridge controller's ID is greater than + * the bus it is placed onto + */ + virDomainControllerDefPtr cont = def->controllers[i]; + int idx = cont->idx; + int bus = cont->info.addr.pci.bus; + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE && + idx <= bus) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("failed to create PCI bridge " + "on bus %d: bus is fully reserved"), + bus); + goto cleanup; + } + }
This should IMHO be a separate commit, as it does not fix the case where the devices exactly fill the buses, but when there are too many. Also, maybe the error message could say something about 'too many devices with fixed addressess'? Jan
On 01/22/2015 05:00 PM, Ján Tomko wrote:
On 01/21/2015 05:50 PM, Erik Skultety wrote:
Commit 93c8ca tried to fix the issue with auto-adding of a PCI bridge controller, it worked well when the slots were reserved by devices with user defined addresses without any other devices with unspecified PCI addresses present in the XML. However, if another device with unspecified PCI addresses appeared in the domain's XML, the same problem occurred as the BZ below references.
This patch fixes the issue by not creating any additional bridges when not necessary. Now let's say all the buses corresponding to the number of PCI controllers present in the domain's XML got fully reserved by devices with user defined addresses. If there are no other devices present in the XML, no need to create both an additional PCI bus and a bridge controller. If however there are still some PCI devices waiting to get a slot assigned by qemuAssignDevicePCISlots, this means an additional bus needs to be created along with a corresponding bridge controller. This scenario now results in an reasonable error instead of generating wrong qemu command line.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1132900 --- src/qemu/qemu_command.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-)
This breaks the pci-many test case you added before.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cdf02a6..99b06ee 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1472,6 +1472,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, int nbuses = 0; size_t i; int rv; + int resflags = 0; + bool buses_reserved = false; +
If you set this to true by default...
virDomainPCIConnectFlags flags = VIR_PCI_CONNECT_TYPE_PCI;
for (i = 0; i < def->ncontrollers; i++) { @@ -1490,17 +1493,22 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, /* 1st pass to figure out how many PCI bridges we need */ if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true))) goto cleanup; + + for (i = 0; i < nbuses; i++) {
+ if (qemuDomainPCIBusFullyReserved(&addrs->buses[i])) + resflags |= 1 << i;
you can do: if (!fullyReserved(buses[i])) buses_reserved = false;
Yeah, that one looks better, thanks.
Or just rename the bool to something like 'addExtraEmptySlot' and invert the condition below. (To stay more positive :))
+ } + buses_reserved = (resflags && ~((~0) << nbuses)); +
Not to mention this one should have been bitwise AND instead of logical one anyway.
if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup;
- for (i = 0; i < addrs->nbuses; i++) { - if (!qemuDomainPCIBusFullyReserved(&addrs->buses[i])) { - - /* Reserve 1 extra slot for a (potential) bridge */ - if (virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0) - goto cleanup; - } - } + /* Reserve 1 extra slot for a (potential) bridge only if buses + * are not fully reserved yet + */ + if (!buses_reserved && + virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0) + goto cleanup;
for (i = 1; i < addrs->nbuses; i++) { virDomainPCIAddressBusPtr bus = &addrs->buses[i]; @@ -1532,6 +1540,24 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup; } + + for (i = 0; i < def->ncontrollers; i++) { + /* check if every PCI bridge controller's ID is greater than + * the bus it is placed onto + */ + virDomainControllerDefPtr cont = def->controllers[i]; + int idx = cont->idx; + int bus = cont->info.addr.pci.bus; + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE && + idx <= bus) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("failed to create PCI bridge " + "on bus %d: bus is fully reserved"), + bus); + goto cleanup; + } + }
This should IMHO be a separate commit, as it does not fix the case where the devices exactly fill the buses, but when there are too many.
You're probably right, coming in v2 series.
Also, maybe the error message could say something about 'too many devices with fixed addressess'?
Jan
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Erik
In order to be able to test for fully reserved PCI buses, assignment of PCI slots for integrated devices needs to be moved to a separate function. This also might be a good preparation if we decide to add support for other chipsets as well. --- src/qemu/qemu_command.c | 45 ++++++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 99b06ee..dbcc854 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1494,6 +1494,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true))) goto cleanup; + if (qemuValidateDevicePCISlotsChipsets(def, qemuCaps, addrs) < 0) + goto cleanup; + for (i = 0; i < nbuses; i++) { if (qemuDomainPCIBusFullyReserved(&addrs->buses[i])) resflags |= 1 << i; @@ -1537,6 +1540,8 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, goto cleanup; if (qemuDomainSupportsPCI(def)) { + if (qemuValidateDevicePCISlotsChipsets(def, qemuCaps, addrs) < 0) + goto cleanup; if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup; } @@ -1996,6 +2001,27 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, return ret; } +static int +qemuValidateDevicePCISlotsChipsets(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + virDomainPCIAddressSetPtr addrs) +{ + if ((STRPREFIX(def->os.machine, "pc-0.") || + STRPREFIX(def->os.machine, "pc-1.") || + STRPREFIX(def->os.machine, "pc-i440") || + STREQ(def->os.machine, "pc") || + STRPREFIX(def->os.machine, "rhel")) && + qemuValidateDevicePCISlotsPIIX3(def, qemuCaps, addrs) < 0) { + return -1; + } + + if (qemuDomainMachineIsQ35(def) && + qemuDomainValidateDevicePCISlotsQ35(def, qemuCaps, addrs) < 0) { + return -1; + } + + return 0; +} /* * This assigns static PCI slots to all configured devices. @@ -2014,6 +2040,9 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, * - PIIX3 ISA bridge, IDE controller, something else unknown, USB controller (slot 1) * - Video (slot 2) * + * - These integrated devices were already added by + * qemuValidateDevicePCISlotsChipsets invoked right before this function + * * Incrementally assign slots from 3 onwards: * * - Net @@ -2031,27 +2060,13 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, */ int qemuAssignDevicePCISlots(virDomainDefPtr def, - virQEMUCapsPtr qemuCaps, + virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED, virDomainPCIAddressSetPtr addrs) { size_t i, j; virDomainPCIConnectFlags flags; virDevicePCIAddress tmp_addr; - if ((STRPREFIX(def->os.machine, "pc-0.") || - STRPREFIX(def->os.machine, "pc-1.") || - STRPREFIX(def->os.machine, "pc-i440") || - STREQ(def->os.machine, "pc") || - STRPREFIX(def->os.machine, "rhel")) && - qemuValidateDevicePCISlotsPIIX3(def, qemuCaps, addrs) < 0) { - goto error; - } - - if (qemuDomainMachineIsQ35(def) && - qemuDomainValidateDevicePCISlotsQ35(def, qemuCaps, addrs) < 0) { - goto error; - } - /* PCI controllers */ for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { -- 1.9.3
On 01/21/2015 05:50 PM, Erik Skultety wrote:
In order to be able to test for fully reserved PCI buses, assignment of PCI slots for integrated devices needs to be moved to a separate function. This also might be a good preparation if we decide to add support for other chipsets as well. --- src/qemu/qemu_command.c | 45 ++++++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 15 deletions(-)
This should be done before fixing the bug in 3/5, and it also doesn't compile. Jan
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 99b06ee..dbcc854 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1494,6 +1494,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true))) goto cleanup;
+ if (qemuValidateDevicePCISlotsChipsets(def, qemuCaps, addrs) < 0) + goto cleanup; + for (i = 0; i < nbuses; i++) { if (qemuDomainPCIBusFullyReserved(&addrs->buses[i])) resflags |= 1 << i; @@ -1537,6 +1540,8 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, goto cleanup;
if (qemuDomainSupportsPCI(def)) { + if (qemuValidateDevicePCISlotsChipsets(def, qemuCaps, addrs) < 0) + goto cleanup; if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup; } @@ -1996,6 +2001,27 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, return ret; }
+static int +qemuValidateDevicePCISlotsChipsets(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + virDomainPCIAddressSetPtr addrs) +{ + if ((STRPREFIX(def->os.machine, "pc-0.") || + STRPREFIX(def->os.machine, "pc-1.") || + STRPREFIX(def->os.machine, "pc-i440") || + STREQ(def->os.machine, "pc") || + STRPREFIX(def->os.machine, "rhel")) && + qemuValidateDevicePCISlotsPIIX3(def, qemuCaps, addrs) < 0) { + return -1; + } + + if (qemuDomainMachineIsQ35(def) && + qemuDomainValidateDevicePCISlotsQ35(def, qemuCaps, addrs) < 0) { + return -1; + } + + return 0; +}
/* * This assigns static PCI slots to all configured devices. @@ -2014,6 +2040,9 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, * - PIIX3 ISA bridge, IDE controller, something else unknown, USB controller (slot 1) * - Video (slot 2) * + * - These integrated devices were already added by + * qemuValidateDevicePCISlotsChipsets invoked right before this function + * * Incrementally assign slots from 3 onwards: * * - Net @@ -2031,27 +2060,13 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, */ int qemuAssignDevicePCISlots(virDomainDefPtr def, - virQEMUCapsPtr qemuCaps, + virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED, virDomainPCIAddressSetPtr addrs) { size_t i, j; virDomainPCIConnectFlags flags; virDevicePCIAddress tmp_addr;
- if ((STRPREFIX(def->os.machine, "pc-0.") || - STRPREFIX(def->os.machine, "pc-1.") || - STRPREFIX(def->os.machine, "pc-i440") || - STREQ(def->os.machine, "pc") || - STRPREFIX(def->os.machine, "rhel")) && - qemuValidateDevicePCISlotsPIIX3(def, qemuCaps, addrs) < 0) { - goto error; - } - - if (qemuDomainMachineIsQ35(def) && - qemuDomainValidateDevicePCISlotsQ35(def, qemuCaps, addrs) < 0) { - goto error; - } - /* PCI controllers */ for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
On 01/22/2015 05:00 PM, Ján Tomko wrote:
On 01/21/2015 05:50 PM, Erik Skultety wrote:
In order to be able to test for fully reserved PCI buses, assignment of PCI slots for integrated devices needs to be moved to a separate function. This also might be a good preparation if we decide to add support for other chipsets as well. --- src/qemu/qemu_command.c | 45 ++++++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 15 deletions(-)
This should be done before fixing the bug in 3/5, and it also doesn't compile.
Jan
I thought about this series as a collection, which did compile just fine, but now I see how silly that was, lesson learned, thank you. Erik
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 99b06ee..dbcc854 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1494,6 +1494,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true))) goto cleanup;
+ if (qemuValidateDevicePCISlotsChipsets(def, qemuCaps, addrs) < 0) + goto cleanup; + for (i = 0; i < nbuses; i++) { if (qemuDomainPCIBusFullyReserved(&addrs->buses[i])) resflags |= 1 << i; @@ -1537,6 +1540,8 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, goto cleanup;
if (qemuDomainSupportsPCI(def)) { + if (qemuValidateDevicePCISlotsChipsets(def, qemuCaps, addrs) < 0) + goto cleanup; if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup; } @@ -1996,6 +2001,27 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, return ret; }
+static int +qemuValidateDevicePCISlotsChipsets(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + virDomainPCIAddressSetPtr addrs) +{ + if ((STRPREFIX(def->os.machine, "pc-0.") || + STRPREFIX(def->os.machine, "pc-1.") || + STRPREFIX(def->os.machine, "pc-i440") || + STREQ(def->os.machine, "pc") || + STRPREFIX(def->os.machine, "rhel")) && + qemuValidateDevicePCISlotsPIIX3(def, qemuCaps, addrs) < 0) { + return -1; + } + + if (qemuDomainMachineIsQ35(def) && + qemuDomainValidateDevicePCISlotsQ35(def, qemuCaps, addrs) < 0) { + return -1; + } + + return 0; +}
/* * This assigns static PCI slots to all configured devices. @@ -2014,6 +2040,9 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, * - PIIX3 ISA bridge, IDE controller, something else unknown, USB controller (slot 1) * - Video (slot 2) * + * - These integrated devices were already added by + * qemuValidateDevicePCISlotsChipsets invoked right before this function + * * Incrementally assign slots from 3 onwards: * * - Net @@ -2031,27 +2060,13 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, */ int qemuAssignDevicePCISlots(virDomainDefPtr def, - virQEMUCapsPtr qemuCaps, + virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED, virDomainPCIAddressSetPtr addrs) { size_t i, j; virDomainPCIConnectFlags flags; virDevicePCIAddress tmp_addr;
- if ((STRPREFIX(def->os.machine, "pc-0.") || - STRPREFIX(def->os.machine, "pc-1.") || - STRPREFIX(def->os.machine, "pc-i440") || - STREQ(def->os.machine, "pc") || - STRPREFIX(def->os.machine, "rhel")) && - qemuValidateDevicePCISlotsPIIX3(def, qemuCaps, addrs) < 0) { - goto error; - } - - if (qemuDomainMachineIsQ35(def) && - qemuDomainValidateDevicePCISlotsQ35(def, qemuCaps, addrs) < 0) { - goto error; - } - /* PCI controllers */ for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
In previous commit a chunk of code got moved in to a separate static function qemuValidateDevicePCISlotsChipsets. This function then invokes chipset specific functions which are defined as static as well. For these reasons it is necessary to either have a forward declaration or slightly reorder definitions. --- src/qemu/qemu_command.c | 255 ++++++++++++++++++++++++------------------------ 1 file changed, 128 insertions(+), 127 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index dbcc854..280a18a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1458,133 +1458,6 @@ qemuDomainPCIBusFullyReserved(virDomainPCIAddressBusPtr bus) return true; } -int -qemuDomainAssignPCIAddresses(virDomainDefPtr def, - virQEMUCapsPtr qemuCaps, - virDomainObjPtr obj) -{ - int ret = -1; - virDomainPCIAddressSetPtr addrs = NULL; - qemuDomainObjPrivatePtr priv = NULL; - - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - int max_idx = -1; - int nbuses = 0; - size_t i; - int rv; - int resflags = 0; - bool buses_reserved = false; - - virDomainPCIConnectFlags flags = VIR_PCI_CONNECT_TYPE_PCI; - - for (i = 0; i < def->ncontrollers; i++) { - if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { - if ((int) def->controllers[i]->idx > max_idx) - max_idx = def->controllers[i]->idx; - } - } - - nbuses = max_idx + 1; - - if (nbuses > 0 && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { - virDomainDeviceInfo info; - - /* 1st pass to figure out how many PCI bridges we need */ - if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true))) - goto cleanup; - - if (qemuValidateDevicePCISlotsChipsets(def, qemuCaps, addrs) < 0) - goto cleanup; - - for (i = 0; i < nbuses; i++) { - if (qemuDomainPCIBusFullyReserved(&addrs->buses[i])) - resflags |= 1 << i; - } - buses_reserved = (resflags && ~((~0) << nbuses)); - - if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) - goto cleanup; - - /* Reserve 1 extra slot for a (potential) bridge only if buses - * are not fully reserved yet - */ - if (!buses_reserved && - virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0) - goto cleanup; - - for (i = 1; i < addrs->nbuses; i++) { - virDomainPCIAddressBusPtr bus = &addrs->buses[i]; - - if ((rv = virDomainDefMaybeAddController( - def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, - i, bus->model)) < 0) - goto cleanup; - /* If we added a new bridge, we will need one more address */ - if (rv > 0 && - virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0) - goto cleanup; - } - nbuses = addrs->nbuses; - virDomainPCIAddressSetFree(addrs); - addrs = NULL; - - } else if (max_idx > 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("PCI bridges are not supported " - "by this QEMU binary")); - goto cleanup; - } - - if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, false))) - goto cleanup; - - if (qemuDomainSupportsPCI(def)) { - if (qemuValidateDevicePCISlotsChipsets(def, qemuCaps, addrs) < 0) - goto cleanup; - if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) - goto cleanup; - } - - for (i = 0; i < def->ncontrollers; i++) { - /* check if every PCI bridge controller's ID is greater than - * the bus it is placed onto - */ - virDomainControllerDefPtr cont = def->controllers[i]; - int idx = cont->idx; - int bus = cont->info.addr.pci.bus; - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && - cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE && - idx <= bus) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("failed to create PCI bridge " - "on bus %d: bus is fully reserved"), - bus); - goto cleanup; - } - } - } - - if (obj && obj->privateData) { - priv = obj->privateData; - if (addrs) { - /* if this is the live domain object, we persist the PCI addresses*/ - virDomainPCIAddressSetFree(priv->pciaddrs); - priv->persistentAddrs = 1; - priv->pciaddrs = addrs; - addrs = NULL; - } else { - priv->persistentAddrs = 0; - } - } - - ret = 0; - - cleanup: - virDomainPCIAddressSetFree(addrs); - - return ret; -} int qemuDomainAssignAddresses(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, @@ -2023,6 +1896,134 @@ qemuValidateDevicePCISlotsChipsets(virDomainDefPtr def, return 0; } +int +qemuDomainAssignPCIAddresses(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + virDomainObjPtr obj) +{ + int ret = -1; + virDomainPCIAddressSetPtr addrs = NULL; + qemuDomainObjPrivatePtr priv = NULL; + + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { + int max_idx = -1; + int nbuses = 0; + size_t i; + int rv; + int resflags = 0; + bool buses_reserved = false; + + virDomainPCIConnectFlags flags = VIR_PCI_CONNECT_TYPE_PCI; + + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { + if ((int) def->controllers[i]->idx > max_idx) + max_idx = def->controllers[i]->idx; + } + } + + nbuses = max_idx + 1; + + if (nbuses > 0 && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { + virDomainDeviceInfo info; + + /* 1st pass to figure out how many PCI bridges we need */ + if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true))) + goto cleanup; + + if (qemuValidateDevicePCISlotsChipsets(def, qemuCaps, addrs) < 0) + goto cleanup; + + for (i = 0; i < nbuses; i++) { + if (qemuDomainPCIBusFullyReserved(&addrs->buses[i])) + resflags |= 1 << i; + } + buses_reserved = (resflags && ~((~0) << nbuses)); + + if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) + goto cleanup; + + /* Reserve 1 extra slot for a (potential) bridge only if buses + * are not fully reserved yet + */ + if (!buses_reserved && + virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0) + goto cleanup; + + for (i = 1; i < addrs->nbuses; i++) { + virDomainPCIAddressBusPtr bus = &addrs->buses[i]; + + if ((rv = virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, + i, bus->model)) < 0) + goto cleanup; + /* If we added a new bridge, we will need one more address */ + if (rv > 0 && + virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0) + goto cleanup; + } + nbuses = addrs->nbuses; + virDomainPCIAddressSetFree(addrs); + addrs = NULL; + + } else if (max_idx > 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("PCI bridges are not supported " + "by this QEMU binary")); + goto cleanup; + } + + if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, false))) + goto cleanup; + + if (qemuDomainSupportsPCI(def)) { + if (qemuValidateDevicePCISlotsChipsets(def, qemuCaps, addrs) < 0) + goto cleanup; + if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) + goto cleanup; + } + + for (i = 0; i < def->ncontrollers; i++) { + /* check if every PCI bridge controller's ID is greater than + * the bus it is placed onto + */ + virDomainControllerDefPtr cont = def->controllers[i]; + int idx = cont->idx; + int bus = cont->info.addr.pci.bus; + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE && + idx <= bus) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("failed to create PCI bridge " + "on bus %d: bus is fully reserved"), + bus); + goto cleanup; + } + } + } + + if (obj && obj->privateData) { + priv = obj->privateData; + if (addrs) { + /* if this is the live domain object, we persist the PCI addresses*/ + virDomainPCIAddressSetFree(priv->pciaddrs); + priv->persistentAddrs = 1; + priv->pciaddrs = addrs; + addrs = NULL; + } else { + priv->persistentAddrs = 0; + } + } + + ret = 0; + + cleanup: + virDomainPCIAddressSetFree(addrs); + + return ret; +} + /* * This assigns static PCI slots to all configured devices. * The ordering here is chosen to match the ordering used -- 1.9.3
participants (2)
-
Erik Skultety -
Ján Tomko