[libvirt] [PATCH 4/9] Add disk-based hotplugging for the qemu backend

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