Re: [libvirt] [PATCH v3 5/5] qemu: auto-add bridges and allow using them
On 22.04.2013 20:43, Ján Tomko wrote:
Add a dry run address allocation to figure out how many bridges
will be needed for all the devices without explicit addresses.
Auto-add just enough bridges to put all the devices on, or up to the
bridge with the largest specified index.
---
src/conf/domain_conf.c | 26 ++-
src/qemu/qemu_command.c| 211
+
src/qemu/qemu_command.h| 4 +-
tests/qemuxml2argvdata/qemuxml2argv-pci-bridge.xml | 210
tests/qemuxml2xmltest.c| 1 +
5 files changed, 411 insertions(+), 41 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-bridge.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 3787ff1..ec7d0e6 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1519,41 +1609,58 @@ void
qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs)
VIR_FREE(addrs);
}
-
static int
qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs,
virDevicePCIAddressPtr next_addr)
{
-virDevicePCIAddress tmp_addr = addrs-lastaddr;
-int i;
-char *addr;
+virDevicePCIAddress a = addrs-lastaddr;
-tmp_addr.slot++;
-for (i = 0; i QEMU_PCI_ADDRESS_SLOT_LAST; i++, tmp_addr.slot++) {
-if (QEMU_PCI_ADDRESS_SLOT_LAST = tmp_addr.slot) {
-tmp_addr.slot = 0;
-}
+if (addrs-nbuses == 0) {
+virReportError(VIR_ERR_XML_ERROR, %s, _(No PCI buses available));
+return -1;
+}
-if (!(addr = qemuPCIAddressAsString(tmp_addr)))
-return -1;
+/* Start the search at the last used bus and slot */
+for (a.slot++; a.bus addrs-nbuses; a.bus++) {
+for ( ; a.slot QEMU_PCI_ADDRESS_SLOT_LAST; a.slot++) {
I know I am too late, but this causes 'make syntax-check' fail. I've
just pushed trivial patch for that.
+if (!qemuDomainPCIAddressSlotInUse(addrs, a))
+goto success;
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 5/5] qemu: auto-add bridges and allow using them
On 04/23/2013 12:24 AM, Eric Blake wrote:
On 04/22/2013 12:43 PM, Ján Tomko wrote:
qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs,
virDevicePCIAddressPtr next_addr)
{
-virDevicePCIAddress tmp_addr = addrs-lastaddr;
-int i;
-char *addr;
+virDevicePCIAddress a = addrs-lastaddr;
-tmp_addr.slot++;
-for (i = 0; i QEMU_PCI_ADDRESS_SLOT_LAST; i++, tmp_addr.slot++) {
-if (QEMU_PCI_ADDRESS_SLOT_LAST = tmp_addr.slot) {
-tmp_addr.slot = 0;
-}
+if (addrs-nbuses == 0) {
+virReportError(VIR_ERR_XML_ERROR, %s, _(No PCI buses
available));
+return -1;
+}
addrs-nbuses should always be = 1, now that we allocate it, right? Is
it possible to hit this error?
It will be 0 when there is no PCI controller present and we'll hit this
error when someone tries to use a PCI device on a machine with no PCI bus.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 5/5] qemu: auto-add bridges and allow using them
Add a dry run address allocation to figure out how many bridges
will be needed for all the devices without explicit addresses.
Auto-add just enough bridges to put all the devices on, or up to the
bridge with the largest specified index.
---
src/conf/domain_conf.c | 26 ++-
src/qemu/qemu_command.c| 211 +
src/qemu/qemu_command.h| 4 +-
tests/qemuxml2argvdata/qemuxml2argv-pci-bridge.xml | 210
tests/qemuxml2xmltest.c| 1 +
5 files changed, 411 insertions(+), 41 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-bridge.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5740009..800c0a7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2578,6 +2578,9 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
virCapsPtr caps ATTRIBUTE_UNUSED)
{
int i;
+bool b;
+int ret = -1;
+virBitmapPtr bitmap = NULL;
/* verify init path for container based domains */
if (STREQ(def-os.type, exe) !def-os.init) {
@@ -2653,11 +2656,30 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
}
}
-return 0;
+/* Verify that PCI controller indexes are unique */
+bitmap = virBitmapNew(def-ncontrollers);
+for (i = 0; i def-ncontrollers; i++) {
+if (def-controllers[i]-type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
+ignore_value(virBitmapGetBit(bitmap, def-controllers[i]-idx,
b));
+if (b) {
+virReportError(VIR_ERR_XML_ERROR,
+ _(Multiple PCI controllers with index %d),
+ def-controllers[i]-idx);
+goto cleanup;
+}
+ignore_value(virBitmapSetBit(bitmap, def-controllers[i]-idx));
+}
+}
+ret = 0;
+
+cleanup:
+virBitmapFree(bitmap);
+
+return ret;
no_memory:
virReportOOMError();
-return -1;
+goto cleanup;
}
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 3787ff1..ec7d0e6 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1197,6 +1197,8 @@ struct _qemuDomainPCIAddressSet {
qemuDomainPCIAddressBus *used;
virDevicePCIAddress lastaddr;
size_t nbuses;/* allocation of 'used' */
+bool dryRun; /* on a dry run, new buses are auto-added
+ and addresses aren't saved in device infos */
};
@@ -1216,9 +1218,10 @@ static bool
qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UN
_(Only PCI domain 0 is available));
return false;
}
-if (addr-bus != 0) {
-virReportError(VIR_ERR_XML_ERROR, %s,
- _(Only PCI bus 0 is available));
+if (addr-bus = addrs-nbuses) {
+virReportError(VIR_ERR_XML_ERROR,
+ _(Only PCI buses up to %zu are available),
+ addrs-nbuses - 1);
return false;
}
if (addr-function = QEMU_PCI_ADDRESS_FUNCTION_LAST) {
@@ -1233,9 +1236,46 @@ static bool
qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UN
QEMU_PCI_ADDRESS_SLOT_LAST);
return false;
}
+if (addr-slot == 0) {
+if (addr-bus) {
+virReportError(VIR_ERR_XML_ERROR, %s,
+ _(Slot 0 is unusable on PCI bridges));
+return false;
+} else {
+virReportError(VIR_ERR_XML_ERROR, %s,
+ _(Slot 0 on bus 0 is reserved for the host
bridge));
+return false;
+}
+}
return true;
}
+/* Ensure addr fits in the address set, by expanding it if needed.
+ * Return value:
+ * -1 = OOM
+ * 0 = no action performed
+ * 0 = number of buses added
+ */
+static int
+qemuDomainPCIAddressSetGrow(qemuDomainPCIAddressSetPtr addrs,
+ virDevicePCIAddressPtr addr)
+{
+int add, i;
+
+add = addr-bus - addrs-nbuses + 1;
+i = addrs-nbuses;
+if (add = 0)
+return 0;
+if (VIR_EXPAND_N(addrs-used, addrs-nbuses, add) 0) {
+virReportOOMError();
+return -1;
+}
+/* reserve slot 0 on the new buses */
+for (; i addrs-nbuses; i++)
+addrs-used[i][0] = 0xFF;
+return add;
+}
+
static char *qemuPCIAddressAsString(virDevicePCIAddressPtr addr)
{
@@ -1273,6 +1313,9 @@ static int qemuCollectPCIAddress(virDomainDefPtr def
ATTRIBUTE_UNUSED,
return 0;
}
+if (addrs-dryRun qemuDomainPCIAddressSetGrow(addrs, addr) 0)
+return -1;
+
if (!qemuPCIAddressValidate(addrs, addr))
return -1;
@@ -1326,15 +1369,54 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
qemuDomainObjPrivatePtr priv = NULL;
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
+int max_idx =
Re: [libvirt] [PATCH v3 5/5] qemu: auto-add bridges and allow using them
(before anything else - you committed an unresolved merge conflict in
qemu_command.h. You'll need to remove the extra blah text.)
Hopefully Eric can once again review the logic of the code that
determines what bridges need to be auto-added, and assign PCI addresses
to devices, since he was kind enough to review it last time :-)
On 04/22/2013 02:43 PM, Ján Tomko wrote:
Add a dry run address allocation to figure out how many bridges
will be needed for all the devices without explicit addresses.
Auto-add just enough bridges to put all the devices on, or up to the
bridge with the largest specified index.
Ah, so then we don't need to warn about gaps in the index sequence.
---
src/conf/domain_conf.c | 26 ++-
src/qemu/qemu_command.c| 211
+
src/qemu/qemu_command.h| 4 +-
tests/qemuxml2argvdata/qemuxml2argv-pci-bridge.xml | 210
tests/qemuxml2xmltest.c| 1 +
5 files changed, 411 insertions(+), 41 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-bridge.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5740009..800c0a7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2578,6 +2578,9 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
virCapsPtr caps ATTRIBUTE_UNUSED)
{
int i;
+bool b;
+int ret = -1;
+virBitmapPtr bitmap = NULL;
/* verify init path for container based domains */
if (STREQ(def-os.type, exe) !def-os.init) {
@@ -2653,11 +2656,30 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
}
}
-return 0;
+/* Verify that PCI controller indexes are unique */
+bitmap = virBitmapNew(def-ncontrollers);
+for (i = 0; i def-ncontrollers; i++) {
+if (def-controllers[i]-type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
+ignore_value(virBitmapGetBit(bitmap, def-controllers[i]-idx,
b));
+if (b) {
+virReportError(VIR_ERR_XML_ERROR,
+ _(Multiple PCI controllers with index %d),
+ def-controllers[i]-idx);
+goto cleanup;
+}
+ignore_value(virBitmapSetBit(bitmap, def-controllers[i]-idx));
+}
+}
+ret = 0;
+
+cleanup:
+virBitmapFree(bitmap);
+
+return ret;
I don't see where we do something like this for the other controller
types. We should (separate patch though, of course :-)
no_memory:
virReportOOMError();
-return -1;
+goto cleanup;
}
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 3787ff1..ec7d0e6 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1197,6 +1197,8 @@ struct _qemuDomainPCIAddressSet {
qemuDomainPCIAddressBus *used;
virDevicePCIAddress lastaddr;
size_t nbuses;/* allocation of 'used' */
+bool dryRun; /* on a dry run, new buses are auto-added
+ and addresses aren't saved in device infos */
};
@@ -1216,9 +1218,10 @@ static bool
qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UN
_(Only PCI domain 0 is available));
return false;
}
-if (addr-bus != 0) {
-virReportError(VIR_ERR_XML_ERROR, %s,
- _(Only PCI bus 0 is available));
+if (addr-bus = addrs-nbuses) {
+virReportError(VIR_ERR_XML_ERROR,
+ _(Only PCI buses up to %zu are available),
+ addrs-nbuses - 1);
return false;
}
if (addr-function = QEMU_PCI_ADDRESS_FUNCTION_LAST) {
@@ -1233,9 +1236,46 @@ static bool
qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UN
QEMU_PCI_ADDRESS_SLOT_LAST);
return false;
}
+if (addr-slot == 0) {
+if (addr-bus) {
+virReportError(VIR_ERR_XML_ERROR, %s,
+ _(Slot 0 is unusable on PCI bridges));
+return false;
+} else {
+virReportError(VIR_ERR_XML_ERROR, %s,
+ _(Slot 0 on bus 0 is reserved for the host
bridge));
+return false;
+}
You can move the duplicated return false; out of the if and else, to a
single occurrence.
+}
return true;
}
+/* Ensure addr fits in the address set, by expanding it if needed.
+ * Return value:
+ * -1 = OOM
+ * 0 = no action performed
+ * 0 = number of buses added
+ */
+static int
+qemuDomainPCIAddressSetGrow(qemuDomainPCIAddressSetPtr addrs,
+ virDevicePCIAddressPtr addr)
+{
+int add, i;
+
+add = addr-bus - addrs-nbuses + 1;
+i = addrs-nbuses;
+if (add = 0)
+return 0;
+
Re: [libvirt] [PATCH v3 5/5] qemu: auto-add bridges and allow using them
On 04/22/2013 12:43 PM, Ján Tomko wrote:
Add a dry run address allocation to figure out how many bridges
will be needed for all the devices without explicit addresses.
Auto-add just enough bridges to put all the devices on, or up to the
bridge with the largest specified index.
---
In addition to Laine's comments,
+virBitmapPtr bitmap = NULL;
/* verify init path for container based domains */
if (STREQ(def-os.type, exe) !def-os.init) {
@@ -2653,11 +2656,30 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
}
}
-return 0;
+/* Verify that PCI controller indexes are unique */
+bitmap = virBitmapNew(def-ncontrollers);
This limits the bitmap to the number of controllers that I passed in,
but your commit message makes it sound like I can pass in a controller
for index 1 and index 2 while letting the code auto-insert the
controller for index 0. If that's true, then...
+for (i = 0; i def-ncontrollers; i++) {
+if (def-controllers[i]-type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
+ignore_value(virBitmapGetBit(bitmap, def-controllers[i]-idx,
b));
...attempting to get bit 2 (based on the explicit 2 in
def-controllers[i]-idx) will fail as out-of-range,...
+if (b) {
+virReportError(VIR_ERR_XML_ERROR,
+ _(Multiple PCI controllers with index %d),
+ def-controllers[i]-idx);
+goto cleanup;
+}
+ignore_value(virBitmapSetBit(bitmap, def-controllers[i]-idx));
+}
+}
...and the attempt to set will also fail. Which means that a similar
example of passing in two controllers that both try to use index 2, and
let 0 and 1 be auto-populated, won't detect the collision. Do we know
what the maximum index will be? Is it time to add a growable bitmap?
Should we separate this duplicate detection code into a separate patch?
+/* Ensure addr fits in the address set, by expanding it if needed.
+ * Return value:
+ * -1 = OOM
+ * 0 = no action performed
+ * 0 = number of buses added
+ */
+static int
+qemuDomainPCIAddressSetGrow(qemuDomainPCIAddressSetPtr addrs,
+ virDevicePCIAddressPtr addr)
Indentation is off.
@@ -1326,15 +1369,54 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
qemuDomainObjPrivatePtr priv = NULL;
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
+int max_idx = 0;
int nbuses = 0;
int i;
+int rv;
for (i = 0; i def-ncontrollers; i++) {
-if (def-controllers[i]-type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
+if (def-controllers[i]-type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
{
+if (def-controllers[i]-idx max_idx)
+max_idx = def-controllers[i]-idx;
nbuses++;
+}
+}
This looks like a more accurate determination of the max bus number;
should you move the duplicate detection here instead?
+
+if (nbuses 0 max_idx = nbuses)
+nbuses = max_idx + 1;
You change nbuses, but only if it is already 1, but then...
+
+if (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 (qemuAssignDevicePCISlots(def, qemuCaps, addrs) 0)
+goto cleanup;
+/* Reserve 1 extra slot for a (potential) bridge */
+if (qemuDomainPCIAddressSetNextAddr(addrs, info) 0)
+goto cleanup;
+
+for (i = 1; i addrs-nbuses; i++) {
+if ((rv = virDomainDefMaybeAddController(
+def, VIR_DOMAIN_CONTROLLER_TYPE_PCI,
+i, VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE)) 0)
+goto cleanup;
+/* If we added a new bridge, we will need one more address */
+if (rv 0 qemuDomainPCIAddressSetNextAddr(addrs, info)
0)
+goto cleanup;
+}
+nbuses = addrs-nbuses;
+qemuDomainPCIAddressSetFree(addrs);
+addrs = NULL;
+
+} else if (nbuses 1) {
...read nbuses if the capability is not present. Would it be any
simpler to just change this to '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)))
+if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, false)))
goto cleanup;
if (qemuAssignDevicePCISlots(def, qemuCaps,
