The instruction "See Makefile.am" in libvirt.private_syms always makes me think that this file is autogenerated and should not be touched manually. This patch spares every reader of libvirt.private_syms the hassle of reading Makefile.am before augmenting libvirt.private_syms. Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer(a)siemens.com&gt; Signed-off-by: Jan Kiszka <jan.kiszka(a)siemens.com&gt; --- src/libvirt_private.syms | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 867678f..f724493 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1,5 +1,6 @@ # -# General private symbols. See Makefile.am. +# General private symbols. Add symbols here, and see Makefile.am for +# more details. # -- 1.6.4

This allows us to connect a disk with a specific controller, which is required for disk hotadd/remove. A new XML child element is added to the <disk> container: <disk> ... <controller pci_addr="<addr>" id="<identifier>" bus="<number>" unit="<number>"/> </disk> Either id _or_ pci_addr can be specified. When the controller has been brought into the system via tghe hotplug mechanism also included in this patch series, it will typically have an ID. In other cases, the controller can also be specified with the PCI address. Wrt. the data structures touched in this commit, notice that while "bus" as subelement of target specifies the type of bus (e.g., scsi, ide,...), it is used to enumerate the buses on the controller here. Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer(a)siemens.com&gt; Signed-off-by: Jan Kiszka <jan.kiszka(a)siemens.com&gt; --- src/domain_conf.c | 29 +++++++++++++++++++++++++++++ src/domain_conf.h | 5 ++++- 2 files changed, 33 insertions(+), 1 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index 5ae0775..a6120c8 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -650,7 +650,12 @@ virDomainDiskDefParseXML(virConnectPtr conn, char *driverType = NULL; char *source = NULL; char *target = NULL; + char *controller = NULL; + char *bus_id = NULL; + char *unit_id = NULL; + char *controller_id = NULL; char *bus = NULL; + char *unit = NULL; char *cachetag = NULL; char *devaddr = NULL; virStorageEncryptionPtr encryption = NULL; @@ -694,12 +699,18 @@ virDomainDiskDefParseXML(virConnectPtr conn, (xmlStrEqual(cur->name, BAD_CAST "target"))) { target = virXMLPropString(cur, "dev"); bus = virXMLPropString(cur, "bus"); + unit = virXMLPropString(cur, "unit"); /* HACK: Work around for compat with Xen * driver in previous libvirt releases */ if (target && STRPREFIX(target, "ioemu:")) memmove(target, target+6, strlen(target)-5); + } else if ((controller == NULL) && + (xmlStrEqual(cur->name, BAD_CAST "controller"))) { + controller_id = virXMLPropString(cur, "id"); + bus_id = virXMLPropString(cur, "bus"); + unit_id = virXMLPropString(cur, "unit"); } else if ((driverName == NULL) && (xmlStrEqual(cur->name, BAD_CAST "driver"))) { driverName = virXMLPropString(cur, "name"); @@ -800,6 +811,24 @@ virDomainDiskDefParseXML(virConnectPtr conn, } } + if (controller) { + def->controller_id = controller_id; + + def->bus_id = -1; + if (bus_id && virStrToLong_i(bus_id, NULL, 10, &def->bus_id) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <controller> 'bus' attribute")); + goto error; + } + + def->unit_id = -1; + if (unit_id && virStrToLong_i(unit_id, NULL, 10, &def->unit_id) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <controller> 'unit' attribute")); + goto error; + } + } + if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && def->bus != VIR_DOMAIN_DISK_BUS_FDC) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, diff --git a/src/domain_conf.h b/src/domain_conf.h index 09368d9..898f6c9 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -105,9 +105,12 @@ typedef virDomainDiskDef *virDomainDiskDefPtr; struct _virDomainDiskDef { int type; int device; - int bus; + int bus; /* Bus type, e.g. scsi or ide */ + int bus_id; /* Bus number on the controller */ + int unit_id; /* Unit on the controller */ char *src; char *dst; + char *controller_id; char *driverName; char *driverType; char *serial; -- 1.6.4

This augments virDomainDevice with a <controller> element that is used to represent disk controllers (e.g., scsi controllers). The XML format is given by <controller type="scsi" id="<my_id>"> <bus addr="<Domain>:<Bus>:<Slot>"> </controller> where type denotes the disk interface (scsi, ide,...), id is an arbitrary string that identifies the controller for disk hotadd/remove, and bus addr denotes the controller address on the PCI bus. The bus element can be omitted; in this case, an address will be assigned automatically. Currently, only hotplugging scsi controllers on a PCI bus is supported, and this only for qemu guests Routines for parsing this definition and the associated data structures are included in this commit. Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer(a)siemens.com&gt; Signed-off-by: Jan Kiszka <jan.kiszka(a)siemens.com&gt; --- docs/schemas/domain.rng | 145 ++++++++++++++++++++++++++++++++++------------- src/domain_conf.c | 125 ++++++++++++++++++++++++++++++++++++++++ src/domain_conf.h | 24 ++++++++ 3 files changed, 255 insertions(+), 39 deletions(-) diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 70e98a7..dc9b849 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -364,49 +364,116 @@ <define name="disk"> <element name="disk"> <optional> - <attribute name="device"> + <attribute name="device"> + <choice> + <value>floppy</value> + <value>disk</value> + <value>cdrom</value> + </choice> + </attribute> + </optional> + <optional> + <element name="controller"> + <optional> + <attribute name="bus"> + <ref name="unsignedInt"/> + </attribute> + </optional> + <optional> + <attribute name="unit"> + <ref name="unsignedInt"/> + </attribute> + </optional> + <choice> + <attribute name="id"> + <ref name="genericName"/> + </attribute> + <attribute name="pciaddr"> + <!-- Just for testing --> + <ref name="deviceName"/> + </attribute> + </choice> + </element> + </optional> + <choice> + <group> + <attribute name="type"> + <value>file</value> + </attribute> + <interleave> + <optional> + <element name="source"> + <attribute name="file"> + <ref name="absFilePath"/> + </attribute> + <empty/> + </element> + </optional> + <ref name="diskspec"/> + </interleave> + </group> + <group> + <attribute name="type"> + <value>block</value> + </attribute> + <interleave> + <optional> + <element name="source"> + <attribute name="dev"> + <ref name="deviceName"/> + </attribute> + <empty/> + </element> + </optional> + <ref name="diskspec"/> + </interleave> + </group> + <ref name="diskspec"/> + </choice> + </element> + </define> + <define name="target"> + <element name="target"> + <attribute name="dev"> + <ref name="deviceName"/> + </attribute> + <optional> + <attribute name="bus"> <choice> - <value>floppy</value> - <value>disk</value> - <value>cdrom</value> + <value>ide</value> + <value>fdc</value> + <value>scsi</value> + <value>virtio</value> + <value>xen</value> + <value>usb</value> + <value>uml</value> </choice> </attribute> </optional> - <choice> - <group> - <attribute name="type"> - <value>file</value> - </attribute> - <interleave> - <optional> - <element name="source"> - <attribute name="file"> - <ref name="absFilePath"/> - </attribute> - <empty/> - </element> - </optional> - <ref name="diskspec"/> - </interleave> - </group> - <group> - <attribute name="type"> - <value>block</value> - </attribute> - <interleave> - <optional> - <element name="source"> - <attribute name="dev"> - <ref name="deviceName"/> - </attribute> - <empty/> - </element> - </optional> - <ref name="diskspec"/> - </interleave> - </group> - <ref name="diskspec"/> - </choice> + </element> + </define> + + <define name="controller"> + <element name="controller"> + <interleave> + <attribute name="type"> + <choice> + <!-- For now, only SCSI is supported --> + <value>scsi</value> + </choice> + </attribute> + <attribute name="id"> + <ref name="genericName"/> + </attribute> + </interleave> + <optional> + <element name="bus"> + <attribute name="addr"> + <!-- Just for testing, should be pciaddress --> + <ref name="deviceName"/> + </attribute> + </element> + </optional> </element> </define> <define name="target"> diff --git a/src/domain_conf.c b/src/domain_conf.c index a6120c8..bbaf944 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -79,6 +79,7 @@ VIR_ENUM_IMPL(virDomainLifecycle, VIR_DOMAIN_LIFECYCLE_LAST, VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST, "disk", + "controller", "filesystem", "interface", "input", @@ -294,6 +295,18 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def); } +void virDomainControllerDefFree(virDomainControllerDefPtr def) +{ + if (!def) + return; + + VIR_FREE(def->type); + VIR_FREE(def->id); + VIR_FREE(def->pci_addr); + + VIR_FREE(def); +} + void virDomainFSDefFree(virDomainFSDefPtr def) { if (!def) @@ -651,6 +664,7 @@ virDomainDiskDefParseXML(virConnectPtr conn, char *source = NULL; char *target = NULL; char *controller = NULL; + char *controller_pci_addr = NULL; char *bus_id = NULL; char *unit_id = NULL; char *controller_id = NULL; @@ -709,6 +723,7 @@ virDomainDiskDefParseXML(virConnectPtr conn, } else if ((controller == NULL) && (xmlStrEqual(cur->name, BAD_CAST "controller"))) { controller_id = virXMLPropString(cur, "id"); + controller_pci_addr = virXMLPropString(cur, "pci_addr"); bus_id = virXMLPropString(cur, "bus"); unit_id = virXMLPropString(cur, "unit"); } else if ((driverName == NULL) && @@ -827,6 +842,19 @@ virDomainDiskDefParseXML(virConnectPtr conn, _("Cannot parse <controller> 'unit' attribute")); goto error; } + + /* TODO: Should we re-use devaddr for this purpose, + or is an extra field justified? */ + if (controller_pci_addr && + sscanf(controller_pci_addr, "%x:%x:%x", + &def->controller_pci_addr.domain, + &def->controller_pci_addr.bus, + &def->controller_pci_addr.slot) < 3) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Unable to parse <controller> 'pci_addr' parameter '%s'"), + controller_pci_addr); + goto error; + } } if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && @@ -895,6 +923,77 @@ cleanup: } +/* Parse the XML definition for a controller + * @param node XML nodeset to parse for controller definition + */ +static virDomainControllerDefPtr +virDomainControllerDefParseXML(virConnectPtr conn, + xmlNodePtr node, + int flags ATTRIBUTE_UNUSED) { + virDomainControllerDefPtr def; + xmlNodePtr cur; + char *type = NULL; + char *bus_addr = NULL; + + if (VIR_ALLOC(def) < 0) { + virReportOOMError(conn); + return NULL; + } + + type = virXMLPropString(node, "type"); + def->type = VIR_DOMAIN_DISK_BUS_SCSI; + if (type) { + if ((def->type = virDomainDiskBusTypeFromString(type)) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("unknown disk bus type '%s'"), type); + goto error; + } + } + + def->id = virXMLPropString(node, "id"); + + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE && + bus_addr == NULL && + xmlStrEqual(cur->name, BAD_CAST "bus")) { + + bus_addr = virXMLPropString(cur, "addr"); + + def->pci_addr.domain = -1; + def->pci_addr.bus = -1; + def->pci_addr.slot = -1; + if (bus_addr && + sscanf(bus_addr, "%x:%x:%x", + &def->pci_addr.domain, + &def->pci_addr.bus, + &def->pci_addr.slot) < 3) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + "Unable to parse <bus> 'addr' parameter '%s'", + bus_addr); + goto error; + } + + VIR_DEBUG("Parse PCI address of controller as %d:%d:%d\n", + def->pci_addr.domain, def->pci_addr.bus, + def->pci_addr.slot); + } + cur = cur->next; + } + + +cleanup: + VIR_FREE(type); + VIR_FREE(bus_addr); + + return def; + + error: + virDomainControllerDefFree(def); + def = NULL; + goto cleanup; +} + /* Parse the XML definition for a disk * @param node XML nodeset to parse for disk definition */ @@ -2375,6 +2474,11 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virConnectPtr conn, dev->type = VIR_DOMAIN_DEVICE_DISK; if (!(dev->data.disk = virDomainDiskDefParseXML(conn, node, flags))) goto error; + } else if (xmlStrEqual(node->name, BAD_CAST "controller")) { + dev->type = VIR_DOMAIN_DEVICE_CONTROLLER; + if (!(dev->data.controller = + virDomainControllerDefParseXML(conn, node, flags))) + goto error; } else if (xmlStrEqual(node->name, BAD_CAST "filesystem")) { dev->type = VIR_DOMAIN_DEVICE_FS; if (!(dev->data.fs = virDomainFSDefParseXML(conn, node, flags))) @@ -2783,6 +2887,27 @@ static virDomainDefPtr virDomainDefParseXML(virConnectPtr conn, } VIR_FREE(nodes); + /* analysis of the disk controllers */ + if ((n = virXPathNodeSet(conn, "./devices/controller", ctxt, &nodes)) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot extract controller devices")); + goto error; + } + if (n && VIR_ALLOC_N(def->controllers, n) < 0) + goto no_memory; + for (i = 0 ; i < n ; i++) { + virDomainControllerDefPtr controller = + virDomainControllerDefParseXML(conn, nodes[i], flags); + if (!controller) + goto error; + + def->controllers[def->ncontrollers++] = controller; + } + /* qsort(def->controllers, def->ncontrollers, sizeof(*def->controllers), + virDomainControllerQSort); */ + VIR_FREE(nodes); + + /* analysis of the filesystems */ if ((n = virXPathNodeSet(conn, "./devices/filesystem", ctxt, &nodes)) < 0) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, diff --git a/src/domain_conf.h b/src/domain_conf.h index 898f6c9..6b3cb09 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -111,6 +111,11 @@ struct _virDomainDiskDef { char *src; char *dst; char *controller_id; + struct { + unsigned domain; + unsigned bus; + unsigned slot; + } controller_pci_addr; char *driverName; char *driverType; char *serial; @@ -125,6 +130,19 @@ struct _virDomainDiskDef { virStorageEncryptionPtr encryption; }; +/* Stores the virtual disk controller configuration */ +typedef struct _virDomainControllerDef virDomainControllerDef; +typedef virDomainControllerDef *virDomainControllerDefPtr; +struct _virDomainControllerDef { + int type; + char *id; + struct { + unsigned domain; + unsigned bus; + unsigned slot; + } pci_addr; +}; + static inline int virDiskHasValidPciAddr(virDomainDiskDefPtr def) { @@ -441,6 +459,7 @@ enum virDomainDeviceType { VIR_DOMAIN_DEVICE_SOUND, VIR_DOMAIN_DEVICE_VIDEO, VIR_DOMAIN_DEVICE_HOSTDEV, + VIR_DOMAIN_DEVICE_CONTROLLER, VIR_DOMAIN_DEVICE_LAST, }; @@ -451,6 +470,7 @@ struct _virDomainDeviceDef { int type; union { virDomainDiskDefPtr disk; + virDomainControllerDefPtr controller; virDomainFSDefPtr fs; virDomainNetDefPtr net; virDomainInputDefPtr input; @@ -561,6 +581,9 @@ struct _virDomainDef { int ndisks; virDomainDiskDefPtr *disks; + int ncontrollers; + virDomainControllerDefPtr *controllers; + int nfss; virDomainFSDefPtr *fss; @@ -637,6 +660,7 @@ virDomainObjPtr virDomainFindByName(const virDomainObjListPtr doms, void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def); void virDomainInputDefFree(virDomainInputDefPtr def); void virDomainDiskDefFree(virDomainDiskDefPtr def); +void virDomainControllerDefFree(virDomainControllerDefPtr def); void virDomainFSDefFree(virDomainFSDefPtr def); void virDomainNetDefFree(virDomainNetDefPtr def); void virDomainChrDefFree(virDomainChrDefPtr def); -- 1.6.4

When disks are added to a qemu backend with attach-device, not just the disk, but a complete new PCI controller with the disk attached is brought into the system. This patch implements a proper disk hotplugging scheme for qemu. Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer(a)siemens.com&gt; Signed-off-by: Jan Kiszka <jan.kiszka(a)siemens.com&gt; --- src/domain_conf.c | 46 ++++++++-- src/domain_conf.h | 2 +- src/qemu_driver.c | 257 ++++++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 277 insertions(+), 28 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index bbaf944..d0fda64 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -663,7 +663,6 @@ virDomainDiskDefParseXML(virConnectPtr conn, char *driverType = NULL; char *source = NULL; char *target = NULL; - char *controller = NULL; char *controller_pci_addr = NULL; char *bus_id = NULL; char *unit_id = NULL; @@ -720,12 +719,13 @@ virDomainDiskDefParseXML(virConnectPtr conn, if (target && STRPREFIX(target, "ioemu:")) memmove(target, target+6, strlen(target)-5); - } else if ((controller == NULL) && + } else if ((controller_id == NULL && + controller_pci_addr == NULL) && (xmlStrEqual(cur->name, BAD_CAST "controller"))) { - controller_id = virXMLPropString(cur, "id"); - controller_pci_addr = virXMLPropString(cur, "pci_addr"); - bus_id = virXMLPropString(cur, "bus"); - unit_id = virXMLPropString(cur, "unit"); + controller_id = virXMLPropString(cur, "id"); + controller_pci_addr = virXMLPropString(cur, "pciaddr"); + bus_id = virXMLPropString(cur, "bus"); + unit_id = virXMLPropString(cur, "unit"); } else if ((driverName == NULL) && (xmlStrEqual(cur->name, BAD_CAST "driver"))) { driverName = virXMLPropString(cur, "name"); @@ -826,7 +826,13 @@ virDomainDiskDefParseXML(virConnectPtr conn, } } - if (controller) { + if (controller_id && controller_pci_addr) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", + _("Both controller ID and address specified!")); + goto error; + } + + if (controller_id) { def->controller_id = controller_id; def->bus_id = -1; @@ -857,12 +863,35 @@ virDomainDiskDefParseXML(virConnectPtr conn, } } + /* TODO: Should we re-use devaddr for this purpose, + or is an extra field justified? */ + def->controller_pci_addr.domain = -1; + def->controller_pci_addr.bus = -1; + def->controller_pci_addr.slot = -1; + + if (controller_pci_addr && + sscanf(controller_pci_addr, "%x:%x:%x", + &def->controller_pci_addr.domain, + &def->controller_pci_addr.bus, + &def->controller_pci_addr.slot) < 3) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Unable to parse <controller> 'pciaddr' parameter '%s'"), + controller_pci_addr); + goto error; + } + + VIR_DEBUG("Controller PCI addr for disk parsed as %d:%d:%d\n", + def->controller_pci_addr.domain, + def->controller_pci_addr.bus, + def->controller_pci_addr.slot); + if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && def->bus != VIR_DOMAIN_DISK_BUS_FDC) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, _("Invalid bus type '%s' for floppy disk"), bus); goto error; } + if (def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && def->bus == VIR_DOMAIN_DISK_BUS_FDC) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, @@ -888,6 +917,9 @@ virDomainDiskDefParseXML(virConnectPtr conn, goto error; } + VIR_DEBUG("Disk PCI address parsed as %d:%d:%d\n", + def->pci_addr.domain, def->pci_addr.bus, def->pci_addr.slot); + def->src = source; source = NULL; def->dst = target; diff --git a/src/domain_conf.h b/src/domain_conf.h index 6b3cb09..41df8f6 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -107,7 +107,7 @@ struct _virDomainDiskDef { int device; int bus; /* Bus type, e.g. scsi or ide */ int bus_id; /* Bus number on the controller */ - int unit_id; /* Unit on the controller */ + int unit_id; /* Unit on the controller (both -1 if unspecified) */ char *src; char *dst; char *controller_id; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index a65334f..4a30615 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -5264,7 +5264,7 @@ qemudParsePciAddReply(virDomainObjPtr vm, { char *s, *e; - DEBUG("%s: pci_add reply: %s", vm->def->name, reply); + VIR_DEBUG("%s: pci_add reply: %s", vm->def->name, reply); /* If the command succeeds qemu prints: * OK bus 0, slot XXX... @@ -5322,16 +5322,70 @@ qemudParsePciAddReply(virDomainObjPtr vm, return 0; } -static int qemudDomainAttachPciDiskDevice(virConnectPtr conn, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev) +static int +qemudParseDriveAddReply(virDomainObjPtr vm, + const char *reply, + int *bus, + int *unit) +{ + char *s, *e; + + DEBUG("%s: drive_add reply: %s", vm->def->name, reply); + + /* If the command succeeds qemu prints: + * OK bus X, unit Y + */ + + if (!(s = strstr(reply, "OK "))) + return -1; + + s += 3; + + if (STRPREFIX(s, "bus ")) { + s += strlen("bus "); + + if (virStrToLong_i(s, &e, 10, bus) == -1) { + VIR_WARN(_("Unable to parse bus '%s'\n"), s); + return -1; + } + + if (!STRPREFIX(e, ", ")) { + VIR_WARN(_("Expected ', ' parsing drive_add reply '%s'\n"), s); + return -1; + } + s = e + 2; + } + + if (!STRPREFIX(s, "unit ")) { + VIR_WARN(_("Expected 'unit ' parsing drive_add reply '%s'\n"), s); + return -1; + } + s += strlen("bus "); + + if (virStrToLong_i(s, &e, 10, unit) == -1) { + VIR_WARN(_("Unable to parse unit number '%s'\n"), s); + return -1; + } + + return 0; +} + +static int qemudDomainAttachDiskDevice(virConnectPtr conn, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) { int ret, i; char *cmd, *reply; char *safe_path; + /* NOTE: Later patch will use type of the controller instead + of the disk */ const char* type = virDomainDiskBusTypeToString(dev->data.disk->bus); int tryOldSyntax = 0; unsigned domain, bus, slot; + int bus_id, unit_id; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char* bus_unit_string; + int controller_specified; for (i = 0 ; i < vm->def->ndisks ; i++) { if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) { @@ -5353,8 +5407,48 @@ try_command: return -1; } - ret = virAsprintf(&cmd, "pci_add %s storage file=%s,if=%s", - (tryOldSyntax ? "0": "pci_addr=auto"), safe_path, type); + controller_specified = 0; + /* Partially specified PCI addresses (should not happen, anyway) + don't count as specification */ + if (dev->data.disk->controller_id != NULL || + (dev->data.disk->controller_pci_addr.domain != -1 && + dev->data.disk->controller_pci_addr.bus != -1 && + dev->data.disk->controller_pci_addr.slot != -1)) { + controller_specified = 1; + } + + if (controller_specified) { + /* NOTE: Proper check for the controller will be implemented + in a later commit */ + domain = dev->data.disk->controller_pci_addr.domain; + bus = dev->data.disk->controller_pci_addr.bus; + slot = dev->data.disk->controller_pci_addr.slot; + + if (dev->data.disk->bus_id != -1) { + virBufferVSprintf(&buf, ",bus=%d", dev->data.disk->bus_id); + } + + if (dev->data.disk->unit_id != -1) { + virBufferVSprintf(&buf, ",unit=%d", dev->data.disk->unit_id); + } + + bus_unit_string = virBufferContentAndReset(&buf); + + VIR_DEBUG ("%s: drive_add %d:%d:%d file=%s,if=%s%s", vm->def->name, + domain, bus, slot, safe_path, type, bus_unit_string); + ret = virAsprintf(&cmd, "drive_add %s%d:%d:%d file=%s,if=%s%s", + (tryOldSyntax ? "" : "pci_addr="), domain, bus, + slot, safe_path, type, bus_unit_string); + } + else { + /* Old-style behaviour: If no <controller> reference is given, then + hotplug a new controller with the disk attached. */ + VIR_DEBUG ("%s: pci_add %s storage file=%s,if=%s", vm->def->name, + (tryOldSyntax ? "0": "pci_addr=auto"), safe_path, type); + ret = virAsprintf(&cmd, "pci_add %s storage file=%s,if=%s", + (tryOldSyntax ? "0": "pci_addr=auto"), safe_path, type); + } + VIR_FREE(safe_path); if (ret == -1) { virReportOOMError(conn); @@ -5370,26 +5464,149 @@ try_command: VIR_FREE(cmd); - if (qemudParsePciAddReply(vm, reply, &domain, &bus, &slot) < 0) { - if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { - VIR_FREE(reply); - tryOldSyntax = 1; - goto try_command; + if (controller_specified) { + if (qemudParseDriveAddReply(vm, reply, &bus_id, &unit_id) < 0) { + if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { + VIR_FREE(reply); + tryOldSyntax = 1; + goto try_command; + } + qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("adding %s disk failed: %s"), type, reply); + VIR_FREE(reply); + return -1; + } + + if (dev->data.disk->bus_id == -1) { + dev->data.disk->bus_id = bus_id; + } + + if (dev->data.disk->unit_id == -1) { + dev->data.disk->unit_id = unit_id; + } + } else { + if (qemudParsePciAddReply(vm, reply, &domain, &bus, &slot) < 0) { + if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { + VIR_FREE(reply); + tryOldSyntax = 1; + goto try_command; + } + + qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("adding %s disk failed: %s"), type, reply); + VIR_FREE(reply); + return -1; + } + } + + dev->data.disk->pci_addr.domain = domain; + dev->data.disk->pci_addr.bus = bus; + dev->data.disk->pci_addr.slot = slot; + + virDomainDiskInsertPreAlloced(vm->def, dev->data.disk); + + + return 0; +} + +static int qemudDomainAttachDiskController(virConnectPtr conn, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int ret, i; + char *cmd, *reply; + char *tmp; + const char* type = virDomainDiskBusTypeToString(dev->data.controller->type); + int tryOldSyntax = 0; + int controllerAddressSpecified = 0; + unsigned domain, bus, slot; + + /* Test if the controller already exists, both by ID and + by PCI address */ + for (i = 0 ; i < vm->def->ncontrollers ; i++) { + if ((dev->data.controller->id && + STREQ(vm->def->controllers[i]->id, dev->data.controller->id)) || + (vm->def->controllers[i]->pci_addr.domain == + dev->data.controller->pci_addr.domain && + vm->def->controllers[i]->pci_addr.bus == + dev->data.controller->pci_addr.bus && + vm->def->controllers[i]->pci_addr.slot == + dev->data.controller->pci_addr.slot)) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("Controller (id:%s, %d:%d:%d) already exists"), + dev->data.controller->id ? + dev->data.controller->id : "(none)", + dev->data.controller->pci_addr.domain, + dev->data.controller->pci_addr.bus, + dev->data.controller->pci_addr.slot); + return -1; } + } - qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("adding %s disk failed: %s"), type, reply); - VIR_FREE(reply); + if (VIR_REALLOC_N(vm->def->controllers, vm->def->ncontrollers+1) < 0) { + virReportOOMError(conn); return -1; } - VIR_FREE(reply); + if (dev->data.controller->pci_addr.domain != -1 && + dev->data.controller->pci_addr.bus != -1 && + dev->data.controller->pci_addr.slot != -1) { + controllerAddressSpecified = 1; + } - dev->data.disk->pci_addr.domain = domain; - dev->data.disk->pci_addr.bus = bus; - dev->data.disk->pci_addr.slot = slot; +try_command: + if (controllerAddressSpecified) { + domain = dev->data.controller->pci_addr.domain; + bus = dev->data.controller->pci_addr.bus; + slot = dev->data.controller->pci_addr.slot; + + ret = virAsprintf(&tmp, "%d:%d:%d", domain, bus, slot); + ret |= virAsprintf(&cmd, "pci_add %s%s storage if=%s", + (tryOldSyntax ? "" : "pci_addr="), tmp, type); + } else { + ret = virAsprintf(&cmd, "pci_add %s storage if=%s", + (tryOldSyntax ? "0" : "pci_addr=auto"), type); + } - virDomainDiskInsertPreAlloced(vm->def, dev->data.disk); + if (ret == -1) { + virReportOOMError(conn); + return ret; + } + + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("cannot attach %s controller"), type); + VIR_FREE(cmd); + return -1; + } + + VIR_FREE(cmd); + + if (qemudParsePciAddReply(vm, reply, &domain, &bus, &slot) < 0) { + if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { + VIR_FREE(reply); + tryOldSyntax = 1; + goto try_command; + } + + qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("adding %s controller failed: %s"), type, reply); + VIR_FREE(reply); + return -1; + } + + /* Also fill in when the address was explicitely specified in + case qemu changed it (TODO: Can this really happen?) */ + dev->data.controller->pci_addr.domain = domain; + dev->data.controller->pci_addr.bus = bus; + dev->data.controller->pci_addr.slot = slot; + + VIR_FREE(reply); + + /* NOTE: Sort function is added in a later commit */ + vm->def->controllers[vm->def->ncontrollers++] = dev->data.controller; + /* qsort(vm->def->disks, vm->def->ndisks, sizeof(*vm->def->disks), + virDomainDiskQSort);*/ return 0; } @@ -5868,7 +6085,7 @@ static int qemudDomainAttachDevice(virDomainPtr dom, ret = qemudDomainAttachUsbMassstorageDevice(dom->conn, vm, dev); } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI || dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { - ret = qemudDomainAttachPciDiskDevice(dom->conn, vm, dev); + ret = qemudDomainAttachDiskDevice(dom->conn, vm, dev); } else { qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, _("disk bus '%s' cannot be hotplugged."), -- 1.6.4

On Fri, Sep 18, 2009 at 05:26:12PM +0200, Wolfgang Mauerer wrote: Any idea what we can do about initial startup? eg, if we start a guest with 1 SCSI controller and 1 disk, and then we've then hotplugged a 2nd controller, and 1 disk. When we later boot or migrate the same guest, we need to have suitable QEMU command line args to make sure we get 2 controllers each with the same disk, rather than 1 controller with 2 disks. I'm not sure how we do this in QEMU, hopefully the existing args support it in some way I've not realized, or failing that the new qdev -device args might help us. I know we used todo this for disk devices, but I'd recommand not going a qsort of devices when hotplugging/unplugging. For hotplug always append to the list, for unplug just shuffle down later devices in the list to fill the hole. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

... by simply traversing the list of controllers to find the associated PCI address. Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer(a)siemens.com&gt; Signed-off-by: Jan Kiszka <jan.kiszka(a)siemens.com&gt; --- src/qemu_driver.c | 41 +++++++++++++++++++++++++++++++---------- 1 files changed, 31 insertions(+), 10 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 3bdd2d7..990f05a 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -5418,19 +5418,40 @@ try_command: } if (controller_specified) { - /* NOTE: Proper check for the controller will be implemented - in a later commit */ - domain = dev->data.disk->controller_pci_addr.domain; - bus = dev->data.disk->controller_pci_addr.bus; - slot = dev->data.disk->controller_pci_addr.slot; - if (dev->data.disk->controller_id) { - /* TODO: Obtain the PCI address of the controller - from the data structures using the ID */ + for (i = 0 ; i < vm->def->ncontrollers ; i++) { + if (STREQ(dev->data.disk->controller_id, + vm->def->controllers[i]->id)) { + break; + } + } + + if (i == vm->def->ncontrollers) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("Controller does not exist")); + return -1; + } + + domain = vm->def->controllers[i]->pci_addr.domain; + bus = vm->def->controllers[i]->pci_addr.bus; + slot = vm->def->controllers[i]->pci_addr.slot; } else { domain = dev->data.disk->controller_pci_addr.domain; bus = dev->data.disk->controller_pci_addr.bus; slot = dev->data.disk->controller_pci_addr.slot; + + for (i = 0 ; i < vm->def->ncontrollers ; i++) { + if (domain == vm->def->controllers[i]->pci_addr.domain && + bus == vm->def->controllers[i]->pci_addr.bus && + slot == vm->def->controllers[i]->pci_addr.slot) + break; + } + + if (i == vm->def->ncontrollers) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("Controller does not exist")); + return -1; + } } if (dev->data.disk->bus_id != -1) { @@ -5457,13 +5478,13 @@ try_command: ret = virAsprintf(&cmd, "pci_add %s storage file=%s,if=%s", (tryOldSyntax ? "0": "pci_addr=auto"), safe_path, type); } - + VIR_FREE(safe_path); if (ret == -1) { virReportOOMError(conn); return ret; } - + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, _("cannot attach %s disk"), type); -- 1.6.4

On Fri, Sep 18, 2009 at 05:26:14PM +0200, Wolfgang Mauerer wrote: Yes, I still think is the good way to go Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Since both disks and disk controllers can be dynamically added to the system, it makes sense to be also able to remove them. Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer(a)siemens.com&gt; Signed-off-by: Jan Kiszka <jan.kiszka(a)siemens.com&gt; --- src/domain_conf.h | 8 +++ src/qemu_driver.c | 161 +++++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 153 insertions(+), 16 deletions(-) diff --git a/src/domain_conf.h b/src/domain_conf.h index 17f8b14..c7d49cf 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -149,6 +149,14 @@ virDiskHasValidPciAddr(virDomainDiskDefPtr def) return def->pci_addr.domain || def->pci_addr.domain || def->pci_addr.slot; } +static inline int +virDiskHasValidController(virDomainDiskDefPtr def) +{ + return def->controller_id != NULL || + def->controller_pci_addr.domain || def->controller_pci_addr.domain + || def->controller_pci_addr.slot; +} + /* Two types of disk backends */ enum virDomainFSType { diff --git a/src/qemu_driver.c b/src/qemu_driver.c index ddc46f6..5ac89a1 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -6204,32 +6204,31 @@ cleanup: return ret; } -static int qemudDomainDetachPciDiskDevice(virConnectPtr conn, - virDomainObjPtr vm, virDomainDeviceDefPtr dev) +static int qemudDomainDetachDiskController(virConnectPtr conn, + virDomainObjPtr vm, virDomainDeviceDefPtr dev) { int i, ret = -1; char *cmd = NULL; char *reply = NULL; - virDomainDiskDefPtr detach = NULL; + virDomainControllerDefPtr detach = NULL; int tryOldSyntax = 0; - for (i = 0 ; i < vm->def->ndisks ; i++) { - if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) { - detach = vm->def->disks[i]; +// virDomainControllerDefPtr is dev->data.controller + for (i = 0 ; i < vm->def->ncontrollers ; i++) { + if (vm->def->controllers[i]->pci_addr.domain == + dev->data.controller->pci_addr.domain && + vm->def->controllers[i]->pci_addr.bus == + dev->data.controller->pci_addr.bus && + vm->def->controllers[i]->pci_addr.slot == + dev->data.controller->pci_addr.slot) { + detach = vm->def->controllers[i]; break; } } if (!detach) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("disk %s not found"), dev->data.disk->dst); - goto cleanup; - } - - if (!virDiskHasValidPciAddr(detach)) { - qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("disk %s cannot be detached - no PCI address for device"), - detach->dst); + _("Controller %s not found"), dev->data.disk->dst); goto cleanup; } @@ -6251,7 +6250,9 @@ try_command: if (qemudMonitorCommand(vm, cmd, &reply) < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("failed to execute detach disk %s command"), detach->dst); + _("failed to execute detach controller %d:%d:%d " \ + "command"), detach->pci_addr.domain, + detach->pci_addr.bus, detach->pci_addr.slot); goto cleanup; } @@ -6267,6 +6268,124 @@ try_command: if (strstr(reply, "invalid slot") || strstr(reply, "Invalid pci address")) { qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to detach controller: invalid PCI address %.4x:%.2x:%.2x: %s"), + detach->pci_addr.domain, + detach->pci_addr.bus, + detach->pci_addr.slot, + reply); + goto cleanup; + } + + if (vm->def->ncontrollers > 1) { + vm->def->controllers[i] = vm->def->controllers[--vm->def->ncontrollers]; + if (VIR_REALLOC_N(vm->def->controllers, vm->def->ncontrollers) < 0) { + virReportOOMError(conn); + goto cleanup; + } + qsort(vm->def->disks, vm->def->ncontrollers, + sizeof(*vm->def->controllers), + virDomainControllerQSort); + } else { + VIR_FREE(vm->def->controllers[0]); + vm->def->controllers = 0; + } + ret = 0; + +cleanup: + VIR_FREE(reply); + VIR_FREE(cmd); + return ret; +} + +static int qemudDomainDetachDiskDevice(virConnectPtr conn, + virDomainObjPtr vm, virDomainDeviceDefPtr dev) +{ + int i, ret = -1; + char *cmd = NULL; + char *reply = NULL; + const char* type; + virDomainDiskDefPtr detach = NULL; + int tryOldSyntax = 0; + + /* If bus and unit are specified, use these first to identify + the disk */ + if (dev->data.disk->controller_pci_addr.domain != -1) { + for (i = 0 ; i < vm->def->ndisks ; i++) { + if (dev->data.disk->bus_id != -1 && + dev->data.disk->bus_id == vm->def->disks[i]->bus_id && + dev->data.disk->unit_id != -1 && + dev->data.disk->unit_id == vm->def->disks[i]->unit_id) { + detach = vm->def->disks[i]; + break; + } + } + } + + /* If the device did not specify a controller explicitely (and therefore + lacks a bus and unit specification), revert to the explicit target + device specification to identify a device for removal. */ + if (!detach) { + for (i = 0 ; i < vm->def->ndisks ; i++) { + if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) { + detach = vm->def->disks[i]; + break; + } + } + } + + if (!detach) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("disk %s not found"), dev->data.disk->dst); + goto cleanup; + } + + if (!virDiskHasValidPciAddr(detach) && !virDiskHasValidController(detach)) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("disk %s cannot be detached - no PCI address for device"), + detach->dst); + goto cleanup; + } + + type = virDomainDiskBusTypeToString(detach->bus); + +try_command: + if (tryOldSyntax) { + if (virAsprintf(&cmd, "drive_del 0 %d:%d:%d bus=%d,unit=%d,if=%s", + detach->pci_addr.domain, detach->pci_addr.bus, + detach->pci_addr.slot, detach->bus_id, + detach->unit_id, type) < 0) { + virReportOOMError(conn); + goto cleanup; + } + } else { + if (virAsprintf(&cmd, "drive_del pci_addr=%d:%d:%d bus=%d,unit=%d,if=%s", + detach->pci_addr.domain, + detach->pci_addr.bus, + detach->pci_addr.slot, detach->bus_id, + detach->unit_id, type) < 0) { + virReportOOMError(conn); + goto cleanup; + } + } + + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to execute detach disk %s command"), detach->dst); + goto cleanup; + } + + DEBUG ("%s: drive_del reply: %s",vm->def->name, reply); + + if (!tryOldSyntax && + strstr(reply, "extraneous characters")) { + tryOldSyntax = 1; + goto try_command; + } + /* OK bux x, unit x is printed on success, but this does not provide + any new information to us.*/ + if (strstr(reply, "Invalid pci address") || + strstr(reply, "No pci device with address")) { + qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, _("failed to detach disk %s: invalid PCI address %.4x:%.2x:%.2x: %s"), detach->dst, detach->pci_addr.domain, @@ -6274,6 +6393,11 @@ try_command: detach->pci_addr.slot, reply); goto cleanup; + } else if (strstr(reply, "Need to specify bus") || + strstr(reply, "Need to specify unit")) { + qemudReportError (conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to detach disk %s: bus and unit not (internal error)"), + detach->dst); } if (vm->def->ndisks > 1) { @@ -6575,7 +6699,7 @@ static int qemudDomainDetachDevice(virDomainPtr dom, dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK && (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI || dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)) { - ret = qemudDomainDetachPciDiskDevice(dom->conn, vm, dev); + ret = qemudDomainDetachDiskDevice(dom->conn, vm, dev); if (driver->securityDriver) driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn, dev->data.disk); if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0) @@ -6584,6 +6708,11 @@ static int qemudDomainDetachDevice(virDomainPtr dom, ret = qemudDomainDetachNetDevice(dom->conn, vm, dev); } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { ret = qemudDomainDetachHostDevice(dom->conn, driver, vm, dev); + } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { + /* NOTE: We can unplug the controller without having to + check if all disks are unplugged; it is at the user's + responsibility to use the required OS services first. */ + ret = qemudDomainDetachDiskController(dom->conn, vm, dev); } else qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, "%s", _("only SCSI or virtio disk device can be detached dynamically")); -- 1.6.4

Daniel P. Berrange wrote: Yes, that's right. I'm not sure if this is worth the extra effort: The disk needs to be removed/unmounted/unplugged from the underlying operating system before it can be removed from the hypervisor (and before the controller is unplugged), but I don't see a way for libvirt to check if the required operations have been performed or not. So if the user does things wrong and removes the disk from the hypervisor without any preparatory work in the guest OS, we will get errors in any case. If, on the other hand, the user does things right (i.e, remove the disk from the OS and then unplug the controller), removing the disks from libvirt is just an extra step. And with real hardware, I would suppose that it's possible to remove a controller with attached disks of the controller allows for PCI hotplug. No clear preference for any of these options from my side, but a slight bias towards not emitting an error message. Any other opinions people... ? ;-) Thanks, Wolfgang