10:48 a.m.
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
10:49 a.m.
New subject: [libvirt] [PATCH 1/5] qemu: Remove dead code in qemuDomainAssignPCIAddresses revert patch
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
4:06 a.m.
New subject: [libvirt] [PATCH 1/5] qemu: Remove dead code in qemuDomainAssignPCIAddresses revert patch
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
10:49 a.m.
New subject: [libvirt] [PATCH 2/5] conf: virDomainDefMaybeAddController tweak return code
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
4:06 a.m.
New subject: [libvirt] [PATCH 2/5] conf: virDomainDefMaybeAddController tweak return code
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
10:50 a.m.
New subject: [libvirt] [PATCH 3/5] qemu: Fix auto-adding PCI bridge when all slots are reserved
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
10 a.m.
New subject: [libvirt] [PATCH 3/5] qemu: Fix auto-adding PCI bridge when all slots are reserved
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
6:04 a.m.
New subject: [libvirt] [PATCH 3/5] qemu: Fix auto-adding PCI bridge when all slots are reserved
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Erik
10:50 a.m.
New subject: [libvirt] [PATCH 4/5] qemu: move PCI slot assignment for PIIX3, Q35 into a separate function
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
10 a.m.
New subject: [libvirt] [PATCH 4/5] qemu: move PCI slot assignment for PIIX3, Q35 into a separate function
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) {
6:05 a.m.
New subject: [libvirt] [PATCH 4/5] qemu: move PCI slot assignment for PIIX3, Q35 into a separate function
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
10:50 a.m.
New subject: [libvirt] [PATCH 5/5] qemu: reorder PCI slot assignment functions
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
3621
days inactive
3623
days old
11 comments
2 participants
participants (2)
-
Erik Skultety -
Ján Tomko