On Thu, May 19, 2016 at 11:13 PM, Laine Stump <laine@laine.org> wrote:

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.

Agree. Will change the commit message.

On 05/18/2016 05:29 PM, Shivaprasad G Bhat wrote:

...

This has to go. The unlugging is going to be supported.

Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/qemu/qemu_hotplug.c | 60 ----------------------------------------------- 1 file changed, 60 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f40b34d..5b822f9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2770,35 +2770,6 @@ qemuDomainChangeGraphics(virQEMUDriverPtr driver, return ret; } - -static int qemuComparePCIDevice(virDomainDefPtr def ATTRIBUTE_UNUSED, - virDomainDeviceDefPtr device ATTRIBUTE_UNUSED, - virDomainDeviceInfoPtr info1, - void *opaque) -{ - virDomainDeviceInfoPtr info2 = opaque; - - if (info1->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI || - info2->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) - return 0; - - if (info1->addr.pci.domain == info2->addr.pci.domain && - info1->addr.pci.bus == info2->addr.pci.bus && - info1->addr.pci.slot == info2->addr.pci.slot && - info1->addr.pci.function != info2->addr.pci.function) - return -1; - return 0; -} - -static bool qemuIsMultiFunctionDevice(virDomainDefPtr def, - virDomainDeviceInfoPtr dev) -{ - if (virDomainDeviceInfoIterate(def, qemuComparePCIDevice, dev) < 0) - return true; - return false; -} - - static int qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3407,13 +3378,6 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; - if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("cannot hot unplug multifunction PCI device: %s"), - detach->dst); - goto cleanup; - } - if (qemuDomainMachineIsS390CCW(vm->def) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) { if (!virDomainDeviceAddressIsValid(&detach->info, @@ -3636,14 +3600,6 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, goto cleanup; } - if (detach->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && - qemuIsMultiFunctionDevice(vm->def, &detach->info)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("cannot hot unplug multifunction PCI device: %s"), - dev->data.disk->dst); - goto cleanup; - } - if (qemuDomainControllerIsBusy(vm, detach)) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("device cannot be detached: device is busy")); @@ -3679,17 +3635,8 @@ qemuDomainDetachHostPCIDevice(virQEMUDriverPtr driver, virDomainHostdevDefPtr detach) { qemuDomainObjPrivatePtr priv = vm->privateData; - virDomainHostdevSubsysPCIPtr pcisrc = &detach->source.subsys.u.pci; int ret; - if (qemuIsMultiFunctionDevice(vm->def, detach->info)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("cannot hot unplug multifunction PCI device: %.4x:%.2x:%.2x.%.1x"), - pcisrc->addr.domain, pcisrc->addr.bus, - pcisrc->addr.slot, pcisrc->addr.function); - return -1; - } - if (!virDomainDeviceAddressIsValid(detach->info,

VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) { virReportError(VIR_ERR_OPERATION_FAILED, @@ -3921,13 +3868,6 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, "%s", _("device cannot be detached without a PCI address")); goto cleanup; } - - if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("cannot hot unplug multifunction PCI device :%s"), - dev->data.disk->dst); - goto cleanup; - } } if (!detach->info.alias) {

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Since you have to unplug all the functions in a slot at the same time anyway, I don't see the point in reverting this commit - you'll just end up needing to call it multiple times - once for each function that was in the slot. (just guessing without looking at the code - perhaps it's already being called from within lower level functions for each device, and each device gets its own notification from qemu that it's been detached? Or to ask a more specific question - exactly what happens with device detach? Do you send qemu a single detach command and it detaches all the functions as a single unit? Or do you send it multiple detach commands, with function 0 being the last?) On 05/18/2016 05:30 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@linux.vnet.ibm.com> --- 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));

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, May 19, 2016 at 11:45 PM, Laine Stump <laine@laine.org> wrote:

On 05/18/2016 05:30 PM, Shivaprasad G Bhat wrote:

...

This function was not used so far. Now, that we begin to use it, make sure to check the address before actually releasing the address.

Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/conf/domain_addr.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 35c7cd4..7ea9e4d 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -497,8 +497,24 @@ int virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr) { + /* permit any kind of connection type in validation, since we + * already had it, and are giving it back. + */ + virDomainPCIConnectFlags flags = VIR_PCI_CONNECT_TYPES_MASK; + int ret = -1; + char *addrStr = NULL; + + if (!(addrStr = virDomainPCIAddressAsString(addr))) + goto cleanup; + + if (!virDomainPCIAddressValidate(addrs, addr, addrStr, flags, false)) + goto cleanup;

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).

virDomainPCIAddressReleaseSlot valiadates the address before releasing today. I didnt want to skip the checks which used to happen before.

+

...

addrs->buses[addr->bus].slots[addr->slot] &= ~(1 << addr->function); - return 0; + ret = 0; + cleanup: + VIR_FREE(addrStr); + return ret; } int

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, May 19, 2016 at 3:01 AM, Shivaprasad G Bhat < shivaprasadbhat@gmail.com> wrote:

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@linux.vnet.ibm.com> --- 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); +

I realised the Character devices arent on PCI slot to get a PCI address and cant be hotplugged. Will drop the character devices.

+ 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;

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, May 19, 2016 at 11:37 PM, Laine Stump <laine@laine.org> wrote:

On 05/18/2016 05:31 PM, Shivaprasad G Bhat wrote:

...

This patch just introduces the parser function used by the later patches. The parser disallows hostdevices to be used with other virtio devices simultaneously.

Why? (and I think you mean "emulated" not "virtio")

Yes. I meant emulated. I am currently disallowing mixing hostdevices with emulated as some drivers might expect themselves to be on function 0.

...

Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- 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); +}

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.

Fixing it.

+

...

/** * 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) +{

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).

Agree. I am doing it.

+ 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;

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 05/18/2016 05:32 PM, Shivaprasad G Bhat wrote:

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.

Since you know that after hotplugging these devices into this slot, you won't be able to add any new devices to it, I think it's unnecessary to keep track of exactly which functions of the slot are occupied and which aren't. Effectively, they *all* are. So instead of copy-pasting the huge chunk of code and allocating one function at a time, how about just marking the entire slot in use at a higher level rather than trying to mark individual functions? (unless you're looking at these individual bits later for something *other* than just deciding which ones to free. Note that you'll need to determine the CONNECT_TYPE at the higher level too, and be sure to choose PCI_DEVICE or PCIE_DEVICE appropriately (and if there is any attempt to mix the two, I would say you should refuse to auto-assign an address (but allow it if they manually specify the address).

The q35 machines needs some level of logic here to get the correct next valid slot so that the hotplug go through fine.

Can you explain that? There should be no difference.

Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- 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;

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, May 19, 2016 at 11:59 PM, Laine Stump <laine@laine.org> wrote:

On 05/18/2016 05:32 PM, Shivaprasad G Bhat wrote:

...

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.

Since you know that after hotplugging these devices into this slot, you won't be able to add any new devices to it, I think it's unnecessary to keep track of exactly which functions of the slot are occupied and which aren't. Effectively, they *all* are.

So instead of copy-pasting the huge chunk of code and allocating one function at a time, how about just marking the entire slot in use at a higher level rather than trying to mark individual functions? (unless you're looking at these individual bits later for something *other* than just deciding which ones to free.

Missed to answer to this point. I am using the function numbers later with the device_add. I need the function numbers to be passed along. So, arriving at it is important here.

Note that you'll need to determine the CONNECT_TYPE at the higher level too, and be sure to choose PCI_DEVICE or PCIE_DEVICE appropriately (and if there is any attempt to mix the two, I would say you should refuse to auto-assign an address (but allow it if they manually specify the address).

...

The q35 machines needs some level of logic here to get the correct next valid slot so that the hotplug go through fine.

Can you explain that? There should be no difference.

...

Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- 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;

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, May 20, 2016 at 12:05 AM, Laine Stump <laine@laine.org> wrote:

On 05/18/2016 05:35 PM, Shivaprasad G Bhat wrote:

...

The hostdevices are the only devices which have dependencies outside of themselves such that, other functions of the PCI card should also have been detached from host driver before attempting the hotplug.

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?

Yes. All devices from same iommu group should be detached from the host. Here, I hope the Card is independent. Otherwise, many cards can also belong to same iommu group. In such case, manual the nodedev-detach for other card functions is necessary.

...

This patch moves the detach to the beginning of the hotplug so that the following patch can detach all funtions first before attempting to hotplug any.

We need not move the detach for net devices using SRIOV as all SRIOV devices are single function devices.

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.

There will be only function zero. So I felt its not necessary. Are you saying different SRIOV functions(all with function zero) be hotplugged as different functions of single card in guest ? In that case, we would need to do that.

...

Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/qemu/qemu_driver.c | 13 ++++++++++++- src/qemu/qemu_hotplug.c | 18 +++++++++--------- 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 37d970e..9cff397 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8273,8 +8273,13 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,

VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0) goto endjob; - if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom)) < 0) + 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; + + if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom)) < 0) + goto undoprepare; /* * update domain status forcibly because the domain status may be * changed even if we failed to attach the device. For example, @@ -8309,6 +8314,12 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, 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); + goto endjob; } static int qemuDomainAttachDevice(virDomainPtr dom, const char *xml) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a2bcd89..bdfbafe 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -899,6 +899,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, actualType = virDomainNetGetActualType(net); if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + virDomainHostdevDefPtr hostdev = virDomainNetGetActualHostdev(net); /* This is really a "smart hostdev", so it should be attached * as a hostdev (the hostdev code will reach over into the * netdev-specific code as appropriate), then also added to @@ -907,8 +908,14 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, * qemuDomainAttachHostDevice uses a connection to resolve * a SCSI hostdev secret, which is not this case, so pass NULL. */ - ret = qemuDomainAttachHostDevice(NULL, driver, vm, - virDomainNetGetActualHostdev(net)); + if (qemuDomainAttachPCIHostDevicePrepare(driver, vm->def, + hostdev, priv->qemuCaps) < 0) + goto cleanup; + + ret = qemuDomainAttachHostDevice(NULL, driver, vm, hostdev); + if (!ret) + qemuHostdevReAttachPCIDevices(driver, vm->def->name, &hostdev, 1); + goto cleanup; } @@ -1248,10 +1255,6 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) return -1; - if (qemuDomainAttachPCIHostDevicePrepare(driver, vm->def, - hostdev, priv->qemuCaps) < 0) - return -1; - backend = hostdev->source.subsys.u.pci.backend; /* Temporarily add the hostdev to the domain definition. This is needed @@ -1330,13 +1333,10 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, if (releaseaddr) qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); - qemuHostdevReAttachPCIDevices(driver, vm->def->name, &hostdev, 1); - VIR_FREE(devstr); VIR_FREE(configfd_name); VIR_FORCE_CLOSE(configfd); - cleanup: return -1; }

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list