If you're going to say in the commit that you're reverting something, you should list the commit id of the patch you're reverting (even if, as in this case, the revert couldn't be done with "git revert"). Usually, though, a revert is done to remove something that shouldn't have been committed in the first place, while these bits were correct. Of course I think that after review it would be more proper to squash this patch into the one that actually enables hot-unplug of multifunction devices. ACK on what it does, though. On 05/18/2016 05:29 PM, Shivaprasad G Bhat wrote:

The commit 6fe678c is reverted. The code is moved around and cant revert staright. Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; --- src/conf/domain_addr.c | 22 +++++++++------------- src/libvirt_private.syms | 1 + src/qemu/qemu_domain_address.c | 2 +- 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index acd8ce6..35c7cd4 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -472,21 +472,17 @@ virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs, goto cleanup; if (dev->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - /* We do not support hotplug multi-function PCI device now, so we should - * reserve the whole slot. The function of the PCI device must be 0. - */ - if (dev->addr.pci.function != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Only PCI device addresses with function=0" - " are supported")); - goto cleanup; - } + if (((dev->addr.pci.function == 0) && (dev->addr.pci.multi == VIR_TRISTATE_SWITCH_ON)) || + dev->addr.pci.function != 0) { - if (!virDomainPCIAddressValidate(addrs, &dev->addr.pci, - addrStr, flags, true)) - goto cleanup; + if (!virDomainPCIAddressValidate(addrs, &dev->addr.pci, + addrStr, flags, true)) + goto cleanup; - ret = virDomainPCIAddressReserveSlot(addrs, &dev->addr.pci, flags); + ret = virDomainPCIAddressReserveAddr(addrs, &dev->addr.pci, flags, false, true); + } else { + ret = virDomainPCIAddressReserveSlot(addrs, &dev->addr.pci, flags); + } } else { ret = virDomainPCIAddressReserveNextSlot(addrs, dev, flags); } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fb24808..e4953b7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -98,6 +98,7 @@ virDomainPCIAddressBusSetModel; virDomainPCIAddressEnsureAddr; virDomainPCIAddressFlagsCompatible; virDomainPCIAddressGetNextSlot; +virDomainPCIAddressReleaseAddr; virDomainPCIAddressReleaseSlot; virDomainPCIAddressReserveAddr; virDomainPCIAddressReserveNextSlot; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 9c8c262..1e7d98c 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1682,7 +1682,7 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, NULLSTR(devstr)); else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && - virDomainPCIAddressReleaseSlot(priv->pciaddrs, + virDomainPCIAddressReleaseAddr(priv->pciaddrs, &info->addr.pci) < 0) VIR_WARN("Unable to release PCI address on %s", NULLSTR(devstr));

On Thu, May 19, 2016 at 11:42 PM, Laine Stump <laine(a)laine.org&gt; wrote: I thought of going this way because, incomplete hotplugs can be attempted towards completion if the hotplug reverts failed. Otherwise, we will have the devices left in the xml with the slot marked free if the hotplug abort failed by any means. This being for a negative test case, I am not sure if recovery of such sorts is possible. Yes. There is an event for each function. On x86, any function device_del would detach all functions of the card and events are sent for all functions. On PPC64, each function should be device_del'ed and function 0 at last and on function 0 deletion alone all the events will come for each device. .

On 05/18/2016 05:30 PM, Shivaprasad G Bhat wrote: I don't think this is necessary. virDomainPCIAddressValidate() is used to verify that it's okay to plug a particular device into a particular slot. But since the device is already in the slot, that's a moot point. The only thing that needs to be verified is that libvirt has information that this device is in the given slot (which I'm assuming is done at a higher level).

This patch just introduces the parser function used by the later patches. The parser disallows hostdevices to be used with other virtio devices simultaneously. Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; --- src/conf/domain_conf.c | 236 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 22 ++++ src/libvirt_private.syms | 3 + 3 files changed, 261 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ed0c471..e946147 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -860,6 +860,36 @@ virDomainXMLOptionClassDispose(void *obj) (xmlopt->config.privFree)(xmlopt->config.priv); } +/* virDomainDeviceDefListAddCopy - add a *copy* of the device to this list */ +int +virDomainDeviceDefListAddCopy(virDomainDeviceDefListPtr list, + virDomainDeviceDefPtr dev, + const virDomainDef *def, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt) +{ + virDomainDeviceDefPtr copy = virDomainDeviceDefCopy(dev, def, caps, xmlopt); + + if (!copy) + return -1; + if (VIR_APPEND_ELEMENT(list->devs, list->count, copy) < 0) { + virDomainDeviceDefFree(copy); + return -1; + } + return 0; +} + +void virDomainDeviceDefListDispose(virDomainDeviceDefListPtr list) +{ + size_t i; + + if (!list) + return; + for (i = 0; i < list->count; i++) + virDomainDeviceDefFree(list->devs[i]); + VIR_FREE(list); +} + /** * virDomainKeyWrapCipherDefParseXML: * @@ -24365,3 +24395,209 @@ virDomainObjGetShortName(virDomainObjPtr vm) return ret; } + +static int +virDomainPCIMultifunctionDeviceDefParseXML(xmlXPathContextPtr ctxt, + const virDomainDef *def, + virDomainDeviceDefListPtr devlist, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags) +{ + size_t i, j; + int n; + virDomainDeviceDef device; + xmlNodePtr *nodes = NULL; + char *netprefix = NULL; + int nhostdevs = 0; + + device.type = VIR_DOMAIN_DEVICE_DISK; + + if ((n = virXPathNodeSet("./disk", ctxt, &nodes)) < 0) + goto error; + + for (i = 0; i < n; i++) { + virDomainDiskDefPtr disk = virDomainDiskDefParseXML(xmlopt, + nodes[i], + ctxt, + NULL, + def->seclabels, + def->nseclabels, + flags); + if (!disk) + goto error; + + device.data.disk = disk; + if (virDomainDeviceDefListAddCopy(devlist, &device, def, caps, xmlopt) < 0) + goto error; + VIR_FREE(disk); + } + VIR_FREE(nodes); + + device.type = VIR_DOMAIN_DEVICE_CONTROLLER; + /* analysis of the controller devices */ + if ((n = virXPathNodeSet("./controller", ctxt, &nodes)) < 0) + goto error; + + for (i = 0; i < n; i++) { + virDomainControllerDefPtr controller = virDomainControllerDefParseXML(nodes[i], + ctxt, + flags); + if (!controller) + goto error; + + device.data.controller = controller; + if (virDomainDeviceDefListAddCopy(devlist, &device, def, caps, xmlopt) < 0) + goto error; + VIR_FREE(controller); + } + VIR_FREE(nodes); + + device.type = VIR_DOMAIN_DEVICE_NET; + if ((n = virXPathNodeSet("./interface", ctxt, &nodes)) < 0) + goto error; + + netprefix = caps->host.netprefix; + for (i = 0; i < n; i++) { + virDomainNetDefPtr net = virDomainNetDefParseXML(xmlopt, + nodes[i], + ctxt, + NULL, + netprefix, + flags); + if (!net) + goto error; + + device.data.net = net; + if (virDomainDeviceDefListAddCopy(devlist, &device, def, caps, xmlopt) < 0) + goto error; + VIR_FREE(net); + } + VIR_FREE(nodes); + + /* analysis of the host devices */ + device.type = VIR_DOMAIN_DEVICE_HOSTDEV; + if ((nhostdevs = virXPathNodeSet("./hostdev", ctxt, &nodes)) < 0) + goto error; + if (nhostdevs && devlist->count) + goto misconfig; + for (i = 0; i < nhostdevs; i++) { + virDomainHostdevDefPtr hostdev; + + hostdev = virDomainHostdevDefParseXML(xmlopt, nodes[i], ctxt, + NULL, flags); + if (!hostdev) + goto error; + + if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB || + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Can't add host USB device: " + "USB is disabled in this host")); + virDomainHostdevDefFree(hostdev); + goto error; + } + device.data.hostdev = hostdev; + for (j = 0; j < devlist->count; j++) { + if (virDomainHostdevMatch(hostdev, devlist->devs[j]->data.hostdev)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Duplicate host devices in list")); + goto error; + } + } + if (virDomainDeviceDefListAddCopy(devlist, &device, def, caps, xmlopt) < 0) + goto error; + VIR_FREE(hostdev); + } + VIR_FREE(nodes); + + /* Parse the RNG devices */ + device.type = VIR_DOMAIN_DEVICE_RNG; + if ((n = virXPathNodeSet("./rng", ctxt, &nodes)) < 0) + goto error; + for (i = 0; i < n; i++) { + virDomainRNGDefPtr rng = virDomainRNGDefParseXML(nodes[i], + ctxt, + flags); + if (!rng) + goto error; + + device.data.rng = rng; + if (virDomainDeviceDefListAddCopy(devlist, &device, def, caps, xmlopt) < 0) + goto error; + VIR_FREE(rng); + } + VIR_FREE(nodes); + + device.type = VIR_DOMAIN_DEVICE_CHR; + if ((n = virXPathNodeSet("./serial", ctxt, &nodes)) < 0) + goto error; + + for (i = 0; i < n; i++) { + virDomainChrDefPtr chr = virDomainChrDefParseXML(ctxt, + nodes[i], + def->seclabels, + def->nseclabels, + flags); + if (!chr) + goto error; + + if (chr->target.port == -1) { + int maxport = -1; + for (j = 0; j < i; j++) { + if (def->serials[j]->target.port > maxport) + maxport = def->serials[j]->target.port; + } + chr->target.port = maxport + 1; + } + device.data.chr = chr; + if (virDomainDeviceDefListAddCopy(devlist, &device, def, caps, xmlopt) < 0) + goto error; + VIR_FREE(chr); + } + VIR_FREE(nodes); + + if (nhostdevs && nhostdevs != devlist->count) + goto misconfig; + + return 0; + misconfig: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Shouldn't mix host devices with other devices")); + error: + return -1; +} + + +int +virDomainPCIMultifunctionDeviceDefParseNode(const char *xml, + const virDomainDef *def, + virDomainDeviceDefListPtr devlist, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags) +{ + xmlXPathContextPtr ctxt = NULL; + int ret = -1; + + xmlDocPtr xmlptr; + + if (!(xmlptr = virXMLParse(NULL, xml, _("(device_definition)")))) { + /* We report error anyway later */ + return -1; + } + + ctxt = xmlXPathNewContext(xmlptr); + if (ctxt == NULL) { + virReportOOMError(); + goto cleanup; + } + + ctxt->node = xmlDocGetRootElement(xmlptr); + ret = virDomainPCIMultifunctionDeviceDefParseXML(ctxt, def, devlist, + caps, xmlopt, flags); + + cleanup: + xmlXPathFreeContext(ctxt); + return ret; +} diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b9e696d..9ddfbd1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2462,6 +2462,20 @@ typedef enum { typedef struct _virDomainXMLOption virDomainXMLOption; typedef virDomainXMLOption *virDomainXMLOptionPtr; +struct virDomainDeviceDefList { + virDomainDeviceDefPtr *devs; + size_t count; +}; +typedef struct virDomainDeviceDefList *virDomainDeviceDefListPtr; + +int +virDomainDeviceDefListAddCopy(virDomainDeviceDefListPtr list, virDomainDeviceDefPtr dev, + const virDomainDef *def, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt); +void virDomainDeviceDefListDispose(virDomainDeviceDefListPtr list); + + /* Called once after everything else has been parsed, for adjusting * overall domain defaults. */ typedef int (*virDomainDefPostParseCallback)(virDomainDefPtr def, @@ -3176,6 +3190,14 @@ int virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def, bool virDomainDefHasMemballoon(const virDomainDef *def) ATTRIBUTE_NONNULL(1); +int +virDomainPCIMultifunctionDeviceDefParseNode(const char *xml, + const virDomainDef *def, + virDomainDeviceDefListPtr devlist, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + unsigned int flags); + char *virDomainObjGetShortName(virDomainObjPtr vm); #endif /* __DOMAIN_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e4953b7..d6a60b5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -109,6 +109,7 @@ virDomainPCIAddressSetGrow; virDomainPCIAddressSlotInUse; virDomainPCIAddressValidate; virDomainPCIControllerModelToConnectType; +virDomainPCIMultifunctionDeviceDefParseNode; virDomainVirtioSerialAddrAssign; virDomainVirtioSerialAddrAutoAssign; virDomainVirtioSerialAddrIsComplete; @@ -249,6 +250,8 @@ virDomainDeviceAddressIsValid; virDomainDeviceAddressTypeToString; virDomainDeviceDefCopy; virDomainDeviceDefFree; +virDomainDeviceDefListAddCopy; +virDomainDeviceDefListDispose; virDomainDeviceDefParse; virDomainDeviceFindControllerModel; virDomainDeviceGetInfo;

On 05/18/2016 05:31 PM, Shivaprasad G Bhat wrote: Why? (and I think you mean "emulated" not "virtio") This isn't a vir*Dispose() function, it is a vir*Free() function. the Dispose() functions are used for virObject-based objects, and take a void *obj as their argument. Instead of all these loops for each type of device. I think it would make more sense to separate virDomainDeviceDefParse() so that the original function was: virDomainDeviceDefParse(....) { ... if (!(xml = virXMLParseString(......))) return -1; node = ctxt->node; dev = virDomainDeviceDefParseXML(node, ctxt, def, caps, xmlopt, flags))) xmlFreeDoc(xml) xmlXPathFreeContext(ctxt); return dev; } with a new function virDomainDeviceDefParseXML() that contained all the rest of the insides of the original function. You could then call this new function for each node of the xmlDocPtr that you get after parsing the input to your new "parse multiple devices" function (which could maybe be called virDomainDeviceDefParseMany()). After all of the devices are parsed, then you can validate that they are all PCI devices, and each for a different function of the same slot (if an address has been assigned).

This patch assigns multifunction pci addresses to devices in the devlist. The pciaddrs passed in the argument is not altered so that the actual call to reserve the address using virDomainPCIAddressEnsureAddr() passes. The function focuses on user given address validation and also the auto-assign of addresses. The address auto-assignment works well for PPC64 and x86-i440fx machines. The q35 machines needs some level of logic here to get the correct next valid slot so that the hotplug go through fine. Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; --- src/conf/domain_addr.c | 199 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 4 + src/libvirt_private.syms | 1 3 files changed, 204 insertions(+) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 7ea9e4d..7c52f84 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -454,6 +454,205 @@ virDomainPCIAddressReserveSlot(virDomainPCIAddressSetPtr addrs, return virDomainPCIAddressReserveAddr(addrs, addr, flags, true, false); } +static int +virDomainPCIAddressGetNextFunctionAddr(uint8_t *slot, + virPCIDeviceAddressPtr addr) +{ + size_t i = 0; + + for (i = 0; i < 8; i++) { + if (!(*slot & 1 << i)) { + addr->function = i; + break; + } + } + + return 0; +} + +static int +virDomainPCIAddressAssignFunctionAddr(virDomainPCIAddressSetPtr addrs, + uint8_t *slot, + virPCIDeviceAddressPtr addr, + virDomainPCIConnectFlags flags, + bool fromConfig) +{ + int ret = -1; + char *addrStr = NULL; + virErrorNumber errType = (fromConfig + ? VIR_ERR_XML_ERROR : VIR_ERR_INTERNAL_ERROR); + + if (!(addrStr = virDomainPCIAddressAsString(addr))) + goto cleanup; + + /* Check that the requested bus exists, is the correct type, and we + * are asking for a valid slot and function + */ + if (!virDomainPCIAddressValidate(addrs, addr, addrStr, flags, fromConfig)) + goto cleanup; + + if (*slot & (1 << addr->function)) { + virReportError(errType, + _("Attempted double use of PCI Address %s"), + addrStr); + goto cleanup; + } + *slot |= (1 << addr->function); + if (addr->function == 0) + addr->multi = VIR_TRISTATE_SWITCH_ON; + VIR_DEBUG("Reserving PCI address %s", addrStr); + + ret = 0; + cleanup: + VIR_FREE(addrStr); + return ret; +} + + +static int +virDomainPCIAddressAssignSlotNextFunction(virDomainPCIAddressSetPtr addrs, + uint8_t *slot, + virDomainDeviceInfoPtr dev, + virDomainPCIConnectFlags flags) +{ + if (virDomainPCIAddressGetNextFunctionAddr(slot, &dev->addr.pci) < 0) + return -1; + + if (virDomainPCIAddressAssignFunctionAddr(addrs, slot, &dev->addr.pci, flags, false) < 0) + return -1; + + dev->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + + return 0; +} + + +static int +virDomainPCIAddressAssignSlotAddr(virDomainPCIAddressSetPtr addrs, + uint8_t *slot, + virDomainDeviceInfoPtr dev) +{ + int ret = -1; + char *addrStr = NULL; + virDomainPCIConnectFlags flags = (VIR_PCI_CONNECT_HOTPLUGGABLE | + VIR_PCI_CONNECT_TYPE_PCI_DEVICE); + + if (!(addrStr = virDomainPCIAddressAsString(&dev->addr.pci))) + goto cleanup; + + if (dev->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + if (((dev->addr.pci.function == 0) && (dev->addr.pci.multi == VIR_TRISTATE_SWITCH_ON)) || + dev->addr.pci.function != 0) { + + if (!virDomainPCIAddressValidate(addrs, &dev->addr.pci, + addrStr, flags, true)) + goto cleanup; + + ret = virDomainPCIAddressAssignFunctionAddr(addrs, slot, &dev->addr.pci, flags, true); + } else { + virReportError(VIR_ERR_XML_ERROR, + _("Not a multifunction device address %s"), addrStr); + } + } else { + ret = virDomainPCIAddressAssignSlotNextFunction(addrs, slot, dev, flags); + } + + cleanup: + VIR_FREE(addrStr); + return ret; +} + +int +virDomainPCIMultifunctionDeviceAddressAssign(virDomainPCIAddressSetPtr addrs, virDomainDeviceDefListPtr devlist) +{ + size_t i; + virPCIDeviceAddress addr; + virDomainPCIAddressBusPtr bus; + uint8_t slot; + virDomainDeviceInfoPtr info = NULL, privinfo = NULL; + virDomainPCIConnectFlags flags = VIR_PCI_CONNECT_TYPES_MASK; + + if (virDomainPCIAddressGetNextSlot(addrs, &addr, flags) < 0) + return -1; + + bus = &addrs->buses[addr.bus]; + slot = bus->slots[addr.slot]; + + for (i = 0; i < devlist->count; i++) { + virDomainDeviceDefPtr dev = devlist->devs[devlist->count - i - 1]; + switch ((virDomainDeviceType) dev->type) { + case VIR_DOMAIN_DEVICE_DISK: + info = &dev->data.disk->info; + break; + case VIR_DOMAIN_DEVICE_NET: + info = &dev->data.net->info; + break; + case VIR_DOMAIN_DEVICE_RNG: + info = &dev->data.rng->info; + break; + case VIR_DOMAIN_DEVICE_HOSTDEV: + info = dev->data.hostdev->info; + break; + case VIR_DOMAIN_DEVICE_CONTROLLER: + info = &dev->data.controller->info; + break; + case VIR_DOMAIN_DEVICE_CHR: + info = &dev->data.chr->info; + break; + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_LAST: + break; + } + + if (i == 0 && info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + /* User has given an address in xml */ + bus = &addrs->buses[info->addr.pci.bus]; + slot = bus->slots[info->addr.pci.slot]; + } + + if (privinfo) { + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + if (privinfo->addr.pci.bus != info->addr.pci.bus || + privinfo->addr.pci.slot != info->addr.pci.slot) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Multifunction PCI device " + "cant be split across different guest PCI slots")); + return -1; + } + } + } + + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + info->addr.pci.bus = addr.bus; + info->addr.pci.slot = addr.slot; + info->addr.pci.domain = addr.domain; + } + + if (virDomainPCIAddressAssignSlotAddr(addrs, &slot, info) < 0) + return -1; + privinfo = info; + } + + return 0; +} + + int virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs, virDomainDeviceInfoPtr dev) diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index f3eda89..9eb6b9d 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -157,6 +157,10 @@ int virDomainPCIAddressReserveNextSlot(virDomainPCIAddressSetPtr addrs, virDomainPCIConnectFlags flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int +virDomainPCIMultifunctionDeviceAddressAssign(virDomainPCIAddressSetPtr addrs, + virDomainDeviceDefListPtr devlist); + struct _virDomainCCWAddressSet { virHashTablePtr defined; virDomainDeviceCCWAddress next; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d6a60b5..d72dc63 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -109,6 +109,7 @@ virDomainPCIAddressSetGrow; virDomainPCIAddressSlotInUse; virDomainPCIAddressValidate; virDomainPCIControllerModelToConnectType; +virDomainPCIMultifunctionDeviceAddressAssign; virDomainPCIMultifunctionDeviceDefParseNode; virDomainVirtioSerialAddrAssign; virDomainVirtioSerialAddrAutoAssign;

On Thu, May 19, 2016 at 11:59 PM, Laine Stump <laine(a)laine.org&gt; wrote: This was mainly for validation. If a user gives the same function number for more than one device, I wanted to refuse that. Also, the ordering of the user specified slot numbers can be different. User can give different slot numbers also. But now you point it out, I realize we need the function zero to be at the last in the device list anyway. So, we can just force user specified function numbers to be in decreasing order if specified. Hope that is the right way? Note that you'll need to determine the CONNECT_TYPE at the higher level I somehow couldn't get it working. May be my setup. I'll need some help when I try next time on this machine.

No Functional change. Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; --- src/qemu/qemu_hotplug.c | 89 ++++++++++++++++++++++++++++++----------------- src/qemu/qemu_hotplug.h | 5 +++ 2 files changed, 61 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5b822f9..a2bcd89 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -705,6 +705,59 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, goto cleanup; } +int qemuDomainAttachPCIHostDevicePrepare(virQEMUDriverPtr driver, + virDomainDefPtr def, + virDomainHostdevDefPtr hostdev, + virQEMUCapsPtr qemuCaps) +{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + unsigned int flags = 0; + int ret = -1; + int backend; + + if (!cfg->relaxedACS) + flags |= VIR_HOSTDEV_STRICT_ACS_CHECK; + if (qemuHostdevPreparePCIDevices(driver, def->name, def->uuid, + &hostdev, 1, qemuCaps, flags) < 0) + goto exit; + + /* this could have been changed by qemuHostdevPreparePCIDevices */ + backend = hostdev->source.subsys.u.pci.backend; + + switch ((virDomainHostdevSubsysPCIBackendType) backend) { + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_PCI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("VFIO PCI device assignment is not " + "supported by this version of qemu")); + goto error; + } + break; + + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT: + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM: + break; + + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN: + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("QEMU does not support device assignment mode '%s'"), + virDomainHostdevSubsysPCIBackendTypeToString(backend)); + goto error; + break; + } + + ret = 0; + exit: + virObjectUnref(cfg); + return ret; + error: + qemuHostdevReAttachPCIDevices(driver, def->name, &hostdev, 1); + goto exit; +} + + + int qemuDomainAttachDeviceDiskLive(virConnectPtr conn, @@ -1191,44 +1244,16 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, bool teardowncgroup = false; bool teardownlabel = false; int backend; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - unsigned int flags = 0; if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) return -1; - if (!cfg->relaxedACS) - flags |= VIR_HOSTDEV_STRICT_ACS_CHECK; - if (qemuHostdevPreparePCIDevices(driver, vm->def->name, vm->def->uuid, - &hostdev, 1, priv->qemuCaps, flags) < 0) - goto cleanup; + if (qemuDomainAttachPCIHostDevicePrepare(driver, vm->def, + hostdev, priv->qemuCaps) < 0) + return -1; - /* this could have been changed by qemuHostdevPreparePCIDevices */ backend = hostdev->source.subsys.u.pci.backend; - switch ((virDomainHostdevSubsysPCIBackendType) backend) { - case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO: - if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_VFIO_PCI)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("VFIO PCI device assignment is not " - "supported by this version of qemu")); - goto error; - } - break; - - case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT: - case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM: - break; - - case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN: - case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("QEMU does not support device assignment mode '%s'"), - virDomainHostdevSubsysPCIBackendTypeToString(backend)); - goto error; - break; - } - /* Temporarily add the hostdev to the domain definition. This is needed * because qemuDomainAdjustMaxMemLock() requires the hostdev to be already * part of the domain definition, but other functions like @@ -1291,7 +1316,6 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, VIR_FREE(devstr); VIR_FREE(configfd_name); VIR_FORCE_CLOSE(configfd); - virObjectUnref(cfg); return 0; @@ -1313,7 +1337,6 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, VIR_FORCE_CLOSE(configfd); cleanup: - virObjectUnref(cfg); return -1; } diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 868b4cf..c127a6d 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -108,6 +108,11 @@ int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRNGDefPtr rng); +int qemuDomainAttachPCIHostDevicePrepare(virQEMUDriverPtr driver, + virDomainDefPtr def, + virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps); + int qemuDomainChrInsert(virDomainDefPtr vmdef, virDomainChrDefPtr chr);

On 05/18/2016 05:35 PM, Shivaprasad G Bhat wrote: Are you saying that all the functions on a host device must be detached from their host driver, even if you're only assigning one or another of them to the guest? I'm not sure why that makes any difference. In any case, you should detach all the devices that are going to be assigned, then assign them all, with the one going to function 0 being last.

The flow is to parse and create a list of devices. Go on each device at each step. Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; --- src/qemu/qemu_driver.c | 328 +++++++++++++++++++++++++++++++++++++----------- 1 file changed, 256 insertions(+), 72 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9cff397..70c241c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8191,6 +8191,77 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps, return 0; } +static bool +qemuDomainPCIAddressIsSingleFunctionAddr(virDomainDeviceDefPtr dev) +{ + + virDomainDeviceInfoPtr info = NULL; + switch ((virDomainDeviceType) dev->type) { + case VIR_DOMAIN_DEVICE_DISK: + info = &dev->data.disk->info; + break; + case VIR_DOMAIN_DEVICE_NET: + info = &dev->data.net->info; + break; + case VIR_DOMAIN_DEVICE_RNG: + info = &dev->data.rng->info; + break; + case VIR_DOMAIN_DEVICE_HOSTDEV: + info = dev->data.hostdev->info; + break; + case VIR_DOMAIN_DEVICE_CONTROLLER: + info = &dev->data.controller->info; + break; + case VIR_DOMAIN_DEVICE_CHR: + info = &dev->data.chr->info; + break; + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_LAST: + break; + } + + if (info && info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + /* We do not support hotplug multi-function PCI device now, so we should + * reserve the whole slot. The function of the PCI device must be 0. + */ + if (info->addr.pci.function != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Single function device addresses with function=0" + " expected")); + return false; + } + } + return true; +} + +static bool isMultifunctionDeviceXML(const char *xml) +{ + xmlDocPtr xmlptr; + + if (!(xmlptr = virXMLParse(NULL, xml, _("(device_definition)")))) { + /* We report error anyway later */ + return false; + } + + return STREQ((const char *)(xmlDocGetRootElement(xmlptr))->name, "devices"); +} + static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, unsigned int flags) @@ -8198,7 +8269,8 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; virDomainDefPtr vmdef = NULL; - virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; + virDomainDeviceDefListPtr devlist = NULL, devcopylist = NULL; + virDomainDeviceDefListPtr rollbacklist = NULL; int ret = -1; unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; @@ -8207,6 +8279,11 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; + virDomainDeviceDefPtr dev = NULL; + bool multifunction = false, pciHostdevs = false; + size_t i = 0, j, d; + virDomainDeviceAction actionFlag = VIR_DOMAIN_DEVICE_ACTION_ATTACH; + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -8231,27 +8308,49 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) goto endjob; - dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, - caps, driver->xmlopt, - parse_flags); - if (dev == NULL) + multifunction = isMultifunctionDeviceXML(xml); + + if (VIR_ALLOC(devlist) < 0) goto endjob; + if (multifunction) { + if (virDomainPCIMultifunctionDeviceDefParseNode(xml, vm->def, devlist, + caps, driver->xmlopt, + parse_flags) < 0) + goto endjob; + + if (virDomainPCIMultifunctionDeviceAddressAssign(priv->pciaddrs, devlist) < 0) + goto endjob; + } else { + dev = virDomainDeviceDefParse(xml, vm->def, + caps, driver->xmlopt, + parse_flags); + if (!dev || !qemuDomainPCIAddressIsSingleFunctionAddr(dev) || + virDomainDeviceDefListAddCopy(devlist, dev, vm->def, caps, + driver->xmlopt) < 0) + goto endjob; + } + + devcopylist = devlist; + if (flags & VIR_DOMAIN_AFFECT_CONFIG && flags & VIR_DOMAIN_AFFECT_LIVE) { /* If we are affecting both CONFIG and LIVE * create a deep copy of device as adding * to CONFIG takes one instance. */ - dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, driver->xmlopt); - if (!dev_copy) + if (VIR_ALLOC(devcopylist) < 0) goto endjob; + for (i = 0; i < devlist->count; i++) + if (virDomainDeviceDefListAddCopy(devcopylist, devlist->devs[i], + vm->def, caps, driver->xmlopt) < 0) + goto endjob; } if (priv->qemuCaps) qemuCaps = virObjectRef(priv->qemuCaps); else if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, vm->def->emulator))) - goto cleanup; + goto endjob; if (flags & VIR_DOMAIN_AFFECT_CONFIG) { /* Make a copy for updated domain. */ @@ -8259,27 +8358,42 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, if (!vmdef) goto endjob; - if (virDomainDefCompatibleDevice(vmdef, dev, - VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0) - goto endjob; + for (i = 0; i < devlist->count; i++) + if (virDomainDefCompatibleDevice(vmdef, devlist->devs[i], actionFlag) < 0) + goto endjob; - if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev, - dom->conn)) < 0) - goto endjob; + for (i = 0; i < devlist->count; i++) + if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, + devlist->devs[i], dom->conn)) < 0) + goto endjob; } + if (devlist->devs[0]->type == VIR_DOMAIN_DEVICE_HOSTDEV && + devlist->devs[0]->data.hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) + pciHostdevs = true; + if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (virDomainDefCompatibleDevice(vm->def, dev_copy, - VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0) - goto endjob; + for (i = 0; i < devcopylist->count; i++) + if (virDomainDefCompatibleDevice(vm->def, devcopylist->devs[i], actionFlag) < 0) + goto endjob; - if (dev_copy->type == VIR_DOMAIN_DEVICE_HOSTDEV && - dev_copy->data.hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI && - qemuDomainAttachPCIHostDevicePrepare(driver,vm->def, dev_copy->data.hostdev, qemuCaps) < 0) - goto endjob; + for (d = 0; d < devcopylist->count; d++) + if (pciHostdevs && + qemuDomainAttachPCIHostDevicePrepare(driver, vm->def, + devcopylist->devs[d]->data.hostdev, qemuCaps) < 0) + goto undoprepare; - if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom)) < 0) - goto undoprepare; + /* Keep a copy for reverts. The devices are initialised to null in + * AttachDeviceLive function */ + if (VIR_ALLOC(rollbacklist) < 0) + goto endjob; + for (i = 0; i < devcopylist->count; i++) { + if (virDomainDeviceDefListAddCopy(rollbacklist, devlist->devs[i], + vm->def, caps, driver->xmlopt) < 0) + goto rollback_livehotplug; + if ((ret = qemuDomainAttachDeviceLive(vm, devcopylist->devs[i], dom)) < 0) + goto rollback_livehotplug; + } /* * update domain status forcibly because the domain status may be * changed even if we failed to attach the device. For example, @@ -8306,19 +8420,39 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, cleanup: virObjectUnref(qemuCaps); virDomainDefFree(vmdef); - if (dev != dev_copy) - virDomainDeviceDefFree(dev_copy); - virDomainDeviceDefFree(dev); + if (devlist != devcopylist) + virDomainDeviceDefListDispose(devcopylist); + virDomainDeviceDefListDispose(devlist); + virDomainDeviceDefListDispose(rollbacklist); virDomainObjEndAPI(&vm); virObjectUnref(caps); virObjectUnref(cfg); virNWFilterUnlockFilterUpdates(); return ret; - undoprepare: - if (dev_copy->type == VIR_DOMAIN_DEVICE_HOSTDEV && - dev_copy->data.hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) - qemuHostdevReAttachPCIDevices(driver, vm->def->name, &dev_copy->data.hostdev, 1); + rollback_livehotplug: + /* The last hotplug on list failed. So "count-1". + * Rollback takes care of cleaning up of any preparations done + * by the respective devices. So, not necessary to do + * any cleanup explicitly here. + */ + for (j = 0; j < rollbacklist->count-1; j++) { + if ((ret = qemuDomainDetachDeviceLive(vm, rollbacklist->devs[j], dom)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Couldnt revert hotplug")); + goto endjob; + } + } + + undoprepare: + if (pciHostdevs) { + /* Devices from o to rollbacklist->count are reattached as part of + * device_deleted event rest of the devices should be bound back to host + * here. 'd' is the last device we detached */ + for (j = rollbacklist ? rollbacklist->count : 0; j < d; j++) + qemuHostdevReAttachPCIDevices(driver, vm->def->name, + &devcopylist->devs[j]->data.hostdev, 1); + } + ret = -1; goto endjob; } @@ -8336,13 +8470,17 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; virDomainDefPtr vmdef = NULL; - virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; + virDomainDeviceDefPtr dev = NULL; bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0; int ret = -1; virQEMUCapsPtr qemuCaps = NULL; + virDomainDeviceDefListPtr devlist = NULL, devcopylist = NULL; qemuDomainObjPrivatePtr priv; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; + bool multifunction = false; + size_t i; + virDomainDeviceAction actionFlag = VIR_DOMAIN_DEVICE_ACTION_UPDATE; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -8366,12 +8504,25 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; - dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, - caps, driver->xmlopt, - VIR_DOMAIN_DEF_PARSE_INACTIVE); - if (dev == NULL) + multifunction = isMultifunctionDeviceXML(xml); + + if (VIR_ALLOC(devlist) < 0) goto endjob; + if (multifunction) { + if (virDomainPCIMultifunctionDeviceDefParseNode(xml, vm->def, devlist, + caps, driver->xmlopt, VIR_DOMAIN_DEF_PARSE_INACTIVE) < 0) + goto endjob; + } else { + dev = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, + VIR_DOMAIN_DEF_PARSE_INACTIVE); + if (!dev || virDomainDeviceDefListAddCopy(devlist, dev, vm->def, caps, + driver->xmlopt) < 0) + goto endjob; + } + + devcopylist = devlist; + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) goto endjob; @@ -8381,9 +8532,12 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, * create a deep copy of device as adding * to CONFIG takes one instance. */ - dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, driver->xmlopt); - if (!dev_copy) + if (VIR_ALLOC(devcopylist) < 0) goto endjob; + for (i = 0; i < devlist->count; i++) + if (virDomainDeviceDefListAddCopy(devcopylist, dev, vm->def, + caps, driver->xmlopt) < 0) + goto endjob; } if (priv->qemuCaps) @@ -8396,22 +8550,26 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); if (!vmdef) goto endjob; + for (i = 0; i < devlist->count; i++) + if (virDomainDefCompatibleDevice(vmdef, devlist->devs[i], actionFlag) < 0) + goto endjob; - if (virDomainDefCompatibleDevice(vmdef, dev, - VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0) - goto endjob; - - if ((ret = qemuDomainUpdateDeviceConfig(qemuCaps, vmdef, dev)) < 0) - goto endjob; + for (i = 0; i < devlist->count; i++) + if ((ret = qemuDomainUpdateDeviceConfig(qemuCaps, vmdef, devlist->devs[i])) < 0) + goto endjob; } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (virDomainDefCompatibleDevice(vm->def, dev_copy, - VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0) - goto endjob; + for (i = 0; i < devcopylist->count; i++) + if (virDomainDefCompatibleDevice(vm->def, devcopylist->devs[i], + actionFlag) < 0) + goto endjob; - if ((ret = qemuDomainUpdateDeviceLive(dom->conn, vm, dev_copy, dom, force)) < 0) - goto endjob; + for (i = 0; i < devcopylist->count; i++) + if ((ret = qemuDomainUpdateDeviceLive(dom->conn, vm, + devcopylist->devs[i], dom, + force)) < 0) + goto endjob; /* * update domain status forcibly because the domain status may be * changed even if we failed to attach the device. For example, @@ -8438,9 +8596,9 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, cleanup: virObjectUnref(qemuCaps); virDomainDefFree(vmdef); - if (dev != dev_copy) - virDomainDeviceDefFree(dev_copy); - virDomainDeviceDefFree(dev); + if (devlist != devcopylist) + virDomainDeviceDefListDispose(devcopylist); + virDomainDeviceDefListDispose(devlist); virDomainObjEndAPI(&vm); virObjectUnref(caps); virObjectUnref(cfg); @@ -8455,13 +8613,17 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; virDomainDefPtr vmdef = NULL; - virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; + virDomainDeviceDefPtr dev = NULL; + virDomainDeviceDefListPtr devlist = NULL, devcopylist = NULL; int ret = -1; unsigned int parse_flags = 0; virQEMUCapsPtr qemuCaps = NULL; qemuDomainObjPrivatePtr priv; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; + bool multifunction = false; + size_t i; + virDomainDeviceAction actionFlag = VIR_DOMAIN_DEVICE_ACTION_DETACH; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -8489,21 +8651,38 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, !(flags & VIR_DOMAIN_AFFECT_LIVE)) parse_flags |= VIR_DOMAIN_DEF_PARSE_INACTIVE; - dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, - caps, driver->xmlopt, - parse_flags); - if (dev == NULL) + multifunction = isMultifunctionDeviceXML(xml); + + if (VIR_ALLOC(devlist) < 0) goto endjob; + if (multifunction) { + if (virDomainPCIMultifunctionDeviceDefParseNode(xml, vm->def, devlist, + caps, driver->xmlopt, + parse_flags) < 0) + goto endjob; + } else { + dev = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, parse_flags); + if (!dev || !qemuDomainPCIAddressIsSingleFunctionAddr(dev) || + virDomainDeviceDefListAddCopy(devlist, dev, vm->def, caps, + driver->xmlopt) < 0) + goto endjob; + } + + devcopylist = devlist; + if (flags & VIR_DOMAIN_AFFECT_CONFIG && flags & VIR_DOMAIN_AFFECT_LIVE) { /* If we are affecting both CONFIG and LIVE * create a deep copy of device as adding * to CONFIG takes one instance. */ - dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, driver->xmlopt); - if (!dev_copy) + if (VIR_ALLOC(devcopylist) < 0) goto endjob; + for (i = 0; i < devcopylist->count; i++) + if (virDomainDeviceDefListAddCopy(devcopylist, dev, vm->def, caps, + driver->xmlopt) < 0) + goto endjob; } if (priv->qemuCaps) @@ -8517,21 +8696,26 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, if (!vmdef) goto endjob; - if (virDomainDefCompatibleDevice(vmdef, dev, - VIR_DOMAIN_DEVICE_ACTION_DETACH) < 0) - goto endjob; - - if ((ret = qemuDomainDetachDeviceConfig(vmdef, dev)) < 0) - goto endjob; + for (i = 0; i < devlist->count; i++) + if (virDomainDefCompatibleDevice(vmdef, devlist->devs[i], actionFlag) < 0) + goto endjob; + for (i = 0; i < devlist->count; i++) + if ((ret = qemuDomainDetachDeviceConfig(vmdef, devlist->devs[i])) < 0) + goto endjob; } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (virDomainDefCompatibleDevice(vm->def, dev_copy, - VIR_DOMAIN_DEVICE_ACTION_DETACH) < 0) - goto endjob; + for (i = 0; i < devcopylist->count; i++) + if (virDomainDefCompatibleDevice(vm->def, devcopylist->devs[i], actionFlag) < 0) + goto endjob; - if ((ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom)) < 0) - goto endjob; + for (i = 0; i < devcopylist->count; i++) { + if ((ret = qemuDomainDetachDeviceLive(vm, devcopylist->devs[i], dom)) < 0) + goto endjob; + /* Detaching any one of the functions is sufficient to unplug */ + if (ARCH_IS_X86(vm->def->os.arch)) + break; + } /* * update domain status forcibly because the domain status may be * changed even if we failed to attach the device. For example, @@ -8558,9 +8742,9 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, cleanup: virObjectUnref(qemuCaps); virDomainDefFree(vmdef); - if (dev != dev_copy) - virDomainDeviceDefFree(dev_copy); - virDomainDeviceDefFree(dev); + if (devlist != devcopylist) + virDomainDeviceDefListDispose(devcopylist); + virDomainDeviceDefListDispose(devlist); virDomainObjEndAPI(&vm); virObjectUnref(caps); virObjectUnref(cfg);