From 4d100c7a41befc21a5c09ad8385c711772714827 Mon Sep 17 00:00:00 2001 From: Laine Stump Date: Tue, 10 May 2016 13:14:32 -0400 Subject: [PATCH] conf: permit auto-assignment of controller indexes Hand-entering indexes for 20 PCI controllers is not as tedious as manually determining and entering their PCI addresses, but it's still annoying, and the algorithm for determining the proper index is incredibly simple (in all cases except one) - just pick the lowest unused index. The one exception is USB2 controllers because multiple controllers in the same group have the same index. For these we look to see if 1) the most recently added USB controller is also a USB2 controller, and 2) the group *that* controller belongs to doesn't yet have a controller of the exact model we're just now adding - if both are true, the new controller gets the same index, but in all other cases we just assign the lowest unused index. With this patch in place and combined with the automatic PCI address assignment, we can define a PCIe switch with several ports like this: ... These will each get a unique index, and PCI addresses that connect them together appropriately with no pesky numbers required. --- docs/formatdomain.html.in | 7 +- docs/schemas/domaincommon.rng | 8 +- src/conf/domain_conf.c | 124 ++++++++++++-- src/conf/domain_conf.h | 2 +- src/qemu/qemu_driver.c | 5 +- src/qemu/qemu_hotplug.c | 8 + .../qemuxml2argv-autoindex.args | 53 ++++++ .../qemuxml2argv-autoindex.xml | 54 +++++++ tests/qemuxml2argvtest.c | 9 ++ .../qemuxml2xmlout-autoindex.xml | 153 ++++++++++++++++++ tests/qemuxml2xmltest.c | 10 +- 11 files changed, 410 insertions(+), 23 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-autoindex.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-autoindex.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-autoindex.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9b3f9ee269..e737e39d38 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3041,8 +3041,11 @@ attribute index which is the decimal integer describing in which order the bus controller is encountered (for use in controller attributes of - <address> elements). Some controller types - have additional attributes that control specific features, such as: + <address> elements). + Since 1.3.5 the index is optional; if + not specified, it will be auto-assigned to be the lowest unused + index for the given controller type. Some controller types have + additional attributes that control specific features, such as:

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index bbe676147e..d14c1c5ecf 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1704,9 +1704,11 @@ - - - + + + + + diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 50d1d94935..568c699888 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1647,9 +1647,11 @@ virDomainControllerDefNew(virDomainControllerType type) return NULL; def->type = type; - def->model = -1; /* initialize anything that has a non-0 default */ + def->model = -1; + def->idx = -1; + switch ((virDomainControllerType) def->type) { case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: def->opts.vioserial.ports = -1; @@ -3585,7 +3587,7 @@ virDomainDefRejectDuplicateControllers(virDomainDefPtr def) for (i = 0; i < def->ncontrollers; i++) { cont = def->controllers[i]; - if ((int) cont->idx > max_idx[cont->type]) + if (cont->idx > max_idx[cont->type]) max_idx[cont->type] = cont->idx; } @@ -4254,6 +4256,84 @@ virDomainDefRemoveOfflineVcpuPin(virDomainDefPtr def) } +static void +virDomainAssignControllerIndexes(virDomainDefPtr def) +{ + /* the index attribute of a controller is optional in the XML, but + * is required to be valid at any time after parse. When no index + * is provided for a controller, assign one automatically by + * looking at what indexes are already used for that controller + * type in the domain - the unindexed controller gets the lowest + * unused index. + */ + size_t outer; + + for (outer = 0; outer < def->ncontrollers; outer++) { + virDomainControllerDefPtr cont = def->controllers[outer]; + virDomainControllerDefPtr prev = NULL; + size_t inner; + + if (cont->idx != -1) + continue; + + if (outer > 0 && IS_USB2_CONTROLLER(cont)) { + /* USB2 controllers are the only exception to the simple + * "assign the lowest unused index". A group of USB2 + * "companions" should all be at the same index as other + * USB2 controllers in the group, but only do this + * automatically if it appears to be the intent. To prove + * intent: the USB controller on the list just prior to + * this one must also be a USB2 controller, and there must + * not yet be a controller with the exact same model of + * this one and the same index as the previously added + * controller (e.g., if this controller is a UHCI1, then + * the previous controller must be an EHCI1 or a UHCI[23], + * and there must not already be a UHCI1 controller with + * the same index as the previous controller). If all of + * these are satisfied, set this controller to the same + * index as the previous controller. + */ + int prevIdx; + + prevIdx = outer - 1; + while (prevIdx >= 0 && + def->controllers[prevIdx]->type != VIR_DOMAIN_CONTROLLER_TYPE_USB) + prevIdx--; + if (prevIdx >= 0) + prev = def->controllers[prevIdx]; + /* if the last USB controller isn't USB2, that breaks + * the chain, so we need a new index for this new + * controller + */ + if (prev && !IS_USB2_CONTROLLER(prev)) + prev = NULL; + + /* if prev != NULL, we've found a potential index to + * use. Make sure this index isn't already used by an + * existing USB2 controller of the same model as the new + * one. + */ + for (inner = 0; prev && inner < def->ncontrollers; inner++) { + if (def->controllers[inner]->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + def->controllers[inner]->idx == prev->idx && + def->controllers[inner]->model == cont->model) { + /* we already have a controller of this model with + * the proposed index, so we need to move to a new + * index for this controller + */ + prev = NULL; + } + } + if (prev) + cont->idx = prev->idx; + } + /* if none of the above applied, prev will be NULL */ + if (!prev) + cont->idx = virDomainControllerFindUnusedIndex(def, cont->type); + } +} + + #define UNSUPPORTED(FEATURE) (!((FEATURE) & xmlopt->config.features)) /** * virDomainDefPostParseCheckFeatures: @@ -4422,6 +4502,11 @@ virDomainDefPostParse(virDomainDefPtr def, .parseFlags = parseFlags, }; + /* this must be done before the hypervisor-specific callback, + * in case presence of a controller at a specific index is checked + */ + virDomainAssignControllerIndexes(def); + /* call the domain config callback */ if (xmlopt->config.domainPostParseCallback) { ret = xmlopt->config.domainPostParseCallback(def, caps, parseFlags, @@ -7908,16 +7993,6 @@ virDomainControllerDefParseXML(xmlNodePtr node, if (!(def = virDomainControllerDefNew(type))) goto error; - idx = virXMLPropString(node, "index"); - if (idx) { - if (virStrToLong_ui(idx, NULL, 10, &def->idx) < 0 || - def->idx > INT_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot parse controller index %s"), idx); - goto error; - } - } - model = virXMLPropString(node, "model"); if (model) { if ((def->model = virDomainControllerModelTypeFromString(def, model)) < 0) { @@ -7927,6 +8002,18 @@ virDomainControllerDefParseXML(xmlNodePtr node, } } + idx = virXMLPropString(node, "index"); + if (idx) { + unsigned int idxVal; + if (virStrToLong_ui(idx, NULL, 10, &idxVal) < 0 || + idxVal > INT_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse controller index %s"), idx); + goto error; + } + def->idx = idxVal; + } + cur = node->children; while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE) { @@ -8075,7 +8162,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, "have an address")); goto error; } - if (def->idx != 0) { + if (def->idx > 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("pci-root and pcie-root controllers " "should have index 0")); @@ -13672,6 +13759,14 @@ void virDomainControllerInsertPreAlloced(virDomainDefPtr def, for (idx = (def->ncontrollers - 1); idx >= 0; idx--) { current = def->controllers[idx]; if (current->type == controller->type) { + if (controller->idx == -1) { + /* If the new controller doesn't have an index set + * yet, put it just past this controller, which until + * now was the last controller of this type. + */ + insertAt = idx + 1; + break; + } if (current->idx > controller->idx) { /* If bus matches and current controller is after * new controller, then new controller should go here @@ -16238,6 +16333,7 @@ virDomainDefParseXML(xmlDocPtr xml, virDomainControllerDefPtr controller = virDomainControllerDefParseXML(nodes[i], ctxt, flags); + if (!controller) goto error; @@ -19552,7 +19648,7 @@ virDomainControllerDefFormat(virBufferPtr buf, } virBufferAsprintf(buf, - "idx); if (model) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0d97003a91..b1953b3143 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -733,7 +733,7 @@ struct _virDomainUSBControllerOpts { /* Stores the virtual disk controller configuration */ struct _virDomainControllerDef { int type; - unsigned int idx; + int idx; int model; /* -1 == undef */ unsigned int queues; unsigned int cmd_per_lun; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 40ef046a93..10d3e3dfe7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1,7 +1,7 @@ /* * qemu_driver.c: core driver methods for managing qemu guests * - * Copyright (C) 2006-2015 Red Hat, Inc. + * Copyright (C) 2006-2016 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -7827,7 +7827,8 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, case VIR_DOMAIN_DEVICE_CONTROLLER: controller = dev->data.controller; - if (virDomainControllerFind(vmdef, controller->type, + if (controller->idx != -1 && + virDomainControllerFind(vmdef, controller->type, controller->idx) >= 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Target already exists")); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 41a0e4f9b8..6ce0a84cfa 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -425,6 +425,14 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, return -1; } + /* default idx would normally be set by virDomainDefPostParse(), + * which isn't called in the case of live attach of a single + * device. + */ + if (controller->idx == -1) + controller->idx = virDomainControllerFindUnusedIndex(vm->def, + controller->type); + if (virDomainControllerFind(vm->def, controller->type, controller->idx) >= 0) { virReportError(VIR_ERR_OPERATION_FAILED, _("target %s:%d already exists"), diff --git a/tests/qemuxml2argvdata/qemuxml2argv-autoindex.args b/tests/qemuxml2argvdata/qemuxml2argv-autoindex.args new file mode 100644 index 0000000000..bbf8f474fd --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-autoindex.args @@ -0,0 +1,53 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/libexec/qemu-kvm \ +-name q35-test \ +-S \ +-M q35 \ +-m 2048 \ +-smp 2 \ +-uuid 11dbdcdd-4c3b-482b-8903-9bdb8c0a2774 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-q35-test/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \ +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \ +-device x3130-upstream,id=pci.4,bus=pci.3,addr=0x0 \ +-device xio3130-downstream,port=0x0,chassis=5,id=pci.5,bus=pci.4,addr=0x0 \ +-device xio3130-downstream,port=0x1,chassis=6,id=pci.6,bus=pci.4,addr=0x1 \ +-device xio3130-downstream,port=0x2,chassis=7,id=pci.7,bus=pci.4,addr=0x2 \ +-device xio3130-downstream,port=0x3,chassis=8,id=pci.8,bus=pci.4,addr=0x3 \ +-device xio3130-downstream,port=0x4,chassis=9,id=pci.9,bus=pci.4,addr=0x4 \ +-device xio3130-downstream,port=0x5,chassis=10,id=pci.10,bus=pci.4,addr=0x5 \ +-device xio3130-downstream,port=0x6,chassis=11,id=pci.11,bus=pci.4,addr=0x6 \ +-device xio3130-downstream,port=0x7,chassis=12,id=pci.12,bus=pci.4,addr=0x7 \ +-device xio3130-downstream,port=0x8,chassis=13,id=pci.13,bus=pci.4,addr=0x8 \ +-device xio3130-downstream,port=0x9,chassis=14,id=pci.14,bus=pci.4,addr=0x9 \ +-device ich9-usb-ehci1,id=usb,bus=pcie.0,addr=0x1d.0x7 \ +-device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pcie.0,multifunction=on,\ +addr=0x1d \ +-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pcie.0,addr=0x1d.0x1 \ +-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pcie.0,addr=0x1d.0x2 \ +-device ich9-usb-ehci1,id=usb1,bus=pcie.0,addr=0x1a.0x7 \ +-device ich9-usb-uhci1,masterbus=usb1.0,firstport=0,bus=pcie.0,multifunction=on,\ +addr=0x1a \ +-device ich9-usb-uhci1,masterbus=usb2.0,firstport=0,bus=pci.2,multifunction=on,\ +addr=0x1 \ +-device ich9-usb-uhci2,masterbus=usb2.0,firstport=2,bus=pci.2,addr=0x1.0x1 \ +-device ich9-usb-uhci3,masterbus=usb2.0,firstport=4,bus=pci.2,addr=0x1.0x2 \ +-device ich9-usb-ehci1,id=usb2,bus=pci.2,addr=0x1.0x7 \ +-device nec-usb-xhci,id=usb3,bus=pci.2,addr=0x2 \ +-device ich9-usb-uhci1,masterbus=usb4.0,firstport=0,bus=pci.2,multifunction=on,\ +addr=0x3 \ +-device ich9-usb-uhci2,masterbus=usb4.0,firstport=2,bus=pci.2,addr=0x3.0x1 \ +-device ich9-usb-uhci3,masterbus=usb4.0,firstport=4,bus=pci.2,addr=0x3.0x2 \ +-device ich9-usb-ehci1,id=usb4,bus=pci.2,addr=0x3.0x7 \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-sata0-0-0 \ +-device ide-drive,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-autoindex.xml b/tests/qemuxml2argvdata/qemuxml2argv-autoindex.xml new file mode 100644 index 0000000000..1fb8c372c0 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-autoindex.xml @@ -0,0 +1,54 @@ + + q35-test + 11dbdcdd-4c3b-482b-8903-9bdb8c0a2774 + 2097152 + 2097152 + 2 + + hvm + + + + destroy + restart + destroy + + /usr/libexec/qemu-kvm + + + +
+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0b26026b5c..db42b7bd71 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1574,6 +1574,15 @@ mymain(void) QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); + DO_TEST("autoindex", + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_DEVICE_X3130_UPSTREAM, + QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1, + QEMU_CAPS_NEC_USB_XHCI); DO_TEST_PARSE_ERROR("q35-wrong-root", QEMU_CAPS_DEVICE_PCI_BRIDGE, diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-autoindex.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-autoindex.xml new file mode 100644 index 0000000000..086a1cf5a4 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-autoindex.xml @@ -0,0 +1,153 @@ + + q35-test + 11dbdcdd-4c3b-482b-8903-9bdb8c0a2774 + 2097152 + 2097152 + 2 + + hvm + + + + destroy + restart + destroy + + /usr/libexec/qemu-kvm + + + +
+ + + + +
+ + + + +
+ + + + +
+ + + +
+ + + + +
+ + + + +
+ + + + +
+ + + + +
+ + + + +
+ + + + +
+ + + + +
+ + + + +
+ + + + +
+ + + + +
+ + +
+ + + +
+ + + +
+ + + +
+ + +
+ + + +
+ + + +
+ + + +
+ + + +
+ + +
+ + +
+ + + +
+ + + +
+ + + +
+ + +
+ + +
+ + + + + + diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index e23831b3b4..e9d1f93836 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -664,7 +664,15 @@ mymain(void) QEMU_CAPS_DEVICE_X3130_UPSTREAM, QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM, QEMU_CAPS_DEVICE_PXB_PCIE); - + DO_TEST_FULL("autoindex", WHEN_ACTIVE, GIC_NONE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_DEVICE_X3130_UPSTREAM, + QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1, + QEMU_CAPS_NEC_USB_XHCI); DO_TEST_FULL("hostdev-scsi-lsi", WHEN_ACTIVE, GIC_NONE, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_LSI,