On Thu, Aug 13, 2009 at 05:44:30PM +0100, Mark McLoughlin wrote:

Re-factor the hostdev hotplug code so that we can easily add PCI hostdev hotplug to qemudDomainAttachHostDevice().

* src/qemu_driver.c: rename qemudDomainAttachHostDevice() to qemudDomainAttachHostUsbDevice(); make qemudDomainAttachHostDevice() handle all hostdev types

* src/libvirt_private.syms: export a couple of hostdev related ToString() functions --- src/libvirt_private.syms | 2 ++ src/qemu_driver.c | 43 +++++++++++++++++++++++++++++++++---------- 2 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f20e5ce..642c2bc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -88,6 +88,8 @@ virDomainGetRootFilesystem; virDomainGraphicsTypeFromString; virDomainGraphicsDefFree; virDomainHostdevDefFree; +virDomainHostdevModeTypeToString; +virDomainHostdevSubsysTypeToString; virDomainInputDefFree; virDomainLifecycleTypeFromString; virDomainLifecycleTypeToString; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 3ec8451..f181f27 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -5148,9 +5148,9 @@ cleanup: return -1; }

-static int qemudDomainAttachHostDevice(virConnectPtr conn, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev) +static int qemudDomainAttachHostUsbDevice(virConnectPtr conn, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) { int ret; char *cmd, *reply; @@ -5200,6 +5200,34 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn, return 0; }

+static int qemudDomainAttachHostDevice(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + virDomainHostdevDefPtr hostdev = dev->data.hostdev; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { + qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT, + _("hostdev mode '%s' not supported"), + virDomainHostdevModeTypeToString(hostdev->mode)); + return -1; + } + + if (qemuDomainSetDeviceOwnership(conn, driver, dev, 0) < 0) + return -1; + + switch (hostdev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: + return qemudDomainAttachHostUsbDevice(conn, vm, dev); + default: + qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT, + _("hostdev subsys type '%s' not supported"), + virDomainHostdevSubsysTypeToString(hostdev->source.subsys.type)); + return -1; + } +} + static int qemudDomainAttachDevice(virDomainPtr dom, const char *xml) { struct qemud_driver *driver = dom->conn->privateData; @@ -5301,13 +5329,8 @@ static int qemudDomainAttachDevice(virDomainPtr dom, } } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { ret = qemudDomainAttachNetDevice(dom->conn, driver, vm, dev, qemuCmdFlags); - } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV && - dev->data.hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - dev->data.hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { - if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0) - goto cleanup; - - ret = qemudDomainAttachHostDevice(dom->conn, vm, dev); + } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { + ret = qemudDomainAttachHostDevice(dom->conn, driver, vm, dev); } else { qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, _("device type '%s' cannot be attached"),

ACK Danel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Thu, Aug 13, 2009 at 05:44:31PM +0100, Mark McLoughlin wrote:

Attaching a host PCI device to a qemu guest is done with a straightforward 'pci_add auto host host=XX:XX.X' command.

Like with NIC and disk hotplug, we need to retain the guest PCI address assigned by qemu so that we can use it for hot-unplug.

Identifying a device for detach is done using the host PCI address.

Managed mode is handled by detaching/resetting the device before attaching it to the guest and re-attaching it after detaching it from the guest.

* src/qemu_driver.c: add qemudDomainAttachHostPciDevice() and qemudDomainDetachHostPciDevice()

* src/domain_conf.h: add somewhere to store the guest PCI address

* src/domain_conf.c: handle formatting and parsing the guest PCI address --- src/domain_conf.c | 33 +++++++- src/domain_conf.h | 13 +++ src/qemu_driver.c | 210 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 252 insertions(+), 4 deletions(-)

diff --git a/src/domain_conf.c b/src/domain_conf.c index 2301a96..bad53f7 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -1977,7 +1977,8 @@ out: static int virDomainHostdevSubsysPciDefParseXML(virConnectPtr conn, const xmlNodePtr node, - virDomainHostdevDefPtr def) { + virDomainHostdevDefPtr def, + int flags) {

int ret = -1; xmlNodePtr cur; @@ -2049,6 +2050,20 @@ virDomainHostdevSubsysPciDefParseXML(virConnectPtr conn, _("pci address needs function id")); goto out; } + } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && + xmlStrEqual(cur->name, BAD_CAST "state")) { + char *devaddr = virXMLPropString(cur, "devaddr"); + if (devaddr && + sscanf(devaddr, "%x:%x:%x", + &def->source.subsys.u.pci.guest_addr.domain, + &def->source.subsys.u.pci.guest_addr.bus, + &def->source.subsys.u.pci.guest_addr.slot) < 3) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Unable to parse devaddr parameter '%s'"), + devaddr); + VIR_FREE(devaddr); + goto out; + } } else { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, _("unknown pci source type '%s'"), @@ -2123,7 +2138,7 @@ virDomainHostdevDefParseXML(virConnectPtr conn, } if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) { - if (virDomainHostdevSubsysPciDefParseXML(conn, cur, def) < 0) + if (virDomainHostdevSubsysPciDefParseXML(conn, cur, def, flags) < 0) goto error; } } else { @@ -3937,7 +3952,8 @@ virDomainGraphicsDefFormat(virConnectPtr conn, static int virDomainHostdevDefFormat(virConnectPtr conn, virBufferPtr buf, - virDomainHostdevDefPtr def) + virDomainHostdevDefPtr def, + int flags) { const char *mode = virDomainHostdevModeTypeToString(def->mode); const char *type; @@ -3978,6 +3994,15 @@ virDomainHostdevDefFormat(virConnectPtr conn, def->source.subsys.u.pci.bus, def->source.subsys.u.pci.slot, def->source.subsys.u.pci.function); + if (flags & VIR_DOMAIN_XML_INTERNAL_STATUS) { + virBufferAddLit(buf, " <state"); + if (virHostdevHasValidGuestAddr(def)) + virBufferVSprintf(buf, " devaddr='%.4x:%.2x:%.2x'", + def->source.subsys.u.pci.guest_addr.domain, + def->source.subsys.u.pci.guest_addr.bus, + def->source.subsys.u.pci.guest_addr.slot); + virBufferAddLit(buf, "/>\n"); + } }

virBufferAddLit(buf, " </source>\n"); @@ -4192,7 +4217,7 @@ char *virDomainDefFormat(virConnectPtr conn, goto cleanup;

for (n = 0 ; n < def->nhostdevs ; n++) - if (virDomainHostdevDefFormat(conn, &buf, def->hostdevs[n]) < 0) + if (virDomainHostdevDefFormat(conn, &buf, def->hostdevs[n], flags) < 0) goto cleanup;

virBufferAddLit(&buf, " </devices>\n"); diff --git a/src/domain_conf.h b/src/domain_conf.h index 63fca76..44302be 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -391,6 +391,11 @@ struct _virDomainHostdevDef { unsigned bus; unsigned slot; unsigned function; + struct { + unsigned domain; + unsigned bus; + unsigned slot; + } guest_addr; } pci; } u; } subsys; @@ -404,6 +409,14 @@ struct _virDomainHostdevDef { char* target; };

+static inline int +virHostdevHasValidGuestAddr(virDomainHostdevDefPtr def) +{ + return def->source.subsys.u.pci.guest_addr.domain || + def->source.subsys.u.pci.guest_addr.bus || + def->source.subsys.u.pci.guest_addr.slot; +} + /* Flags for the 'type' field in next struct */ enum virDomainDeviceType { VIR_DOMAIN_DEVICE_DISK, diff --git a/src/qemu_driver.c b/src/qemu_driver.c index f181f27..041e3da 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -5148,6 +5148,80 @@ cleanup: return -1; }

+static int qemudDomainAttachHostPciDevice(virConnectPtr conn, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + virDomainHostdevDefPtr hostdev = dev->data.hostdev; + char *cmd, *reply; + unsigned domain, bus, slot; + + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs+1) < 0) { + virReportOOMError(conn); + return -1; + } + + if (hostdev->managed) { + pciDevice *pci = pciGetDevice(conn, + hostdev->source.subsys.u.pci.domain, + hostdev->source.subsys.u.pci.bus, + hostdev->source.subsys.u.pci.slot, + hostdev->source.subsys.u.pci.function); + if (!dev) + return -1; + + if (pciDettachDevice(conn, pci) < 0 || + pciResetDevice(conn, pci) < 0) { + pciFreeDevice(conn, pci); + return -1; + } + + pciFreeDevice(conn, pci); + } + + if (virAsprintf(&cmd, "pci_add auto host host=%.2x:%.2x.%.1x", + hostdev->source.subsys.u.pci.bus, + hostdev->source.subsys.u.pci.slot, + hostdev->source.subsys.u.pci.function) < 0) { + virReportOOMError(conn); + return -1; + } + + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + "%s", _("cannot attach host pci device")); + VIR_FREE(cmd); + return -1; + } + + if (strstr(reply, "invalid type: host")) { + qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT, "%s", + _("PCI device assignment is not supported by this version of qemu")); + VIR_FREE(cmd); + VIR_FREE(reply); + return -1; + } + + if (qemudParsePciAddReply(vm, reply, &domain, &bus, &slot) < 0) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + _("parsing pci_add reply failed: %s"), reply); + VIR_FREE(cmd); + VIR_FREE(reply); + return -1; + } + + hostdev->source.subsys.u.pci.guest_addr.domain = domain; + hostdev->source.subsys.u.pci.guest_addr.bus = bus; + hostdev->source.subsys.u.pci.guest_addr.slot = slot; + + vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; + + VIR_FREE(reply); + VIR_FREE(cmd); + + return 0; +} + static int qemudDomainAttachHostUsbDevice(virConnectPtr conn, virDomainObjPtr vm, virDomainDeviceDefPtr dev) @@ -5218,6 +5292,8 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn, return -1;

switch (hostdev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: + return qemudDomainAttachHostPciDevice(conn, vm, dev); case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: return qemudDomainAttachHostUsbDevice(conn, vm, dev); default: @@ -5545,6 +5621,138 @@ cleanup: return ret; }

+static int qemudDomainDetachHostPciDevice(virConnectPtr conn, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + virDomainHostdevDefPtr detach; + char *cmd, *reply; + int i, ret; + + for (i = 0 ; i < vm->def->nhostdevs ; i++) { + unsigned domain = vm->def->hostdevs[i]->source.subsys.u.pci.domain; + unsigned bus = vm->def->hostdevs[i]->source.subsys.u.pci.bus; + unsigned slot = vm->def->hostdevs[i]->source.subsys.u.pci.slot; + unsigned function = vm->def->hostdevs[i]->source.subsys.u.pci.function; + + if (dev->data.hostdev->source.subsys.u.pci.domain == domain && + dev->data.hostdev->source.subsys.u.pci.bus == bus && + dev->data.hostdev->source.subsys.u.pci.slot == slot && + dev->data.hostdev->source.subsys.u.pci.function == function) { + detach = vm->def->hostdevs[i]; + break; + } + } + + if (!detach) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("host pci device %.4x:%.2x:%.2x.%.1x not found"), + dev->data.hostdev->source.subsys.u.pci.domain, + dev->data.hostdev->source.subsys.u.pci.bus, + dev->data.hostdev->source.subsys.u.pci.slot, + dev->data.hostdev->source.subsys.u.pci.function); + return -1; + } + + if (!virHostdevHasValidGuestAddr(detach)) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + "%s", _("hostdev cannot be detached - device state missing")); + return -1; + } + + if (virAsprintf(&cmd, "pci_del pci_addr=%.4x:%.2x:%.2x", + detach->source.subsys.u.pci.guest_addr.domain, + detach->source.subsys.u.pci.guest_addr.bus, + detach->source.subsys.u.pci.guest_addr.slot) < 0) { + virReportOOMError(conn); + return -1; + } + + if (qemudMonitorCommand(vm, cmd, &reply) < 0) { + qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + "%s", _("cannot detach host pci device")); + VIR_FREE(cmd); + return -1; + } + + DEBUG("%s: pci_del reply: %s", vm->def->name, reply); + + /* If the command fails due to a wrong PCI address qemu prints + * 'invalid pci address'; nothing is printed on success */ + if (strstr(reply, "Invalid pci address")) { + qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, + _("failed to detach host pci device: invalid PCI address %.4x:%.2x:%.2x: %s"), + detach->source.subsys.u.pci.guest_addr.domain, + detach->source.subsys.u.pci.guest_addr.bus, + detach->source.subsys.u.pci.guest_addr.slot, + reply); + VIR_FREE(reply); + VIR_FREE(cmd); + return -1; + } + + VIR_FREE(reply); + VIR_FREE(cmd); + + ret = 0; + + if (detach->managed) { + pciDevice *pci = pciGetDevice(conn, + detach->source.subsys.u.pci.domain, + detach->source.subsys.u.pci.bus, + detach->source.subsys.u.pci.slot, + detach->source.subsys.u.pci.function); + if (!pci || pciReAttachDevice(conn, pci) < 0) + ret = -1; + if (pci) + pciFreeDevice(conn, pci); + } + + if (vm->def->nhostdevs > 1) { + vm->def->hostdevs[i] = vm->def->hostdevs[--vm->def->nhostdevs]; + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs) < 0) { + virReportOOMError(conn); + ret = -1; + } + } else { + VIR_FREE(vm->def->hostdevs[0]); + vm->def->nhostdevs = 0; + } + + return ret; +} + +static int qemudDomainDetachHostDevice(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + virDomainHostdevDefPtr hostdev = dev->data.hostdev; + int ret; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { + qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT, + _("hostdev mode '%s' not supported"), + virDomainHostdevModeTypeToString(hostdev->mode)); + return -1; + } + + switch (hostdev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: + ret = qemudDomainDetachHostPciDevice(conn, vm, dev); + default: + qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT, + _("hostdev subsys type '%s' not supported"), + virDomainHostdevSubsysTypeToString(hostdev->source.subsys.type)); + return -1; + } + + if (qemuDomainSetDeviceOwnership(conn, driver, dev, 1) < 0) + VIR_WARN0("Fail to restore disk device ownership"); + + return ret; +} + static int qemudDomainDetachDevice(virDomainPtr dom, const char *xml) { struct qemud_driver *driver = dom->conn->privateData; @@ -5585,6 +5793,8 @@ static int qemudDomainDetachDevice(virDomainPtr dom, VIR_WARN0("Fail to restore disk device ownership"); } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { ret = qemudDomainDetachNetDevice(dom->conn, vm, dev); + } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { + ret = qemudDomainDetachHostDevice(dom->conn, driver, vm, dev); } else qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, "%s", _("only SCSI or virtio disk device can be detached dynamically")); --

ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Thu, Aug 13, 2009 at 05:44:32PM +0100, Mark McLoughlin wrote:

PCI device assignment is only supported in KVM's fork of qemu, so we should really detect its availability and give a nice error if its not supported.

* src/qemu_conf.[ch]: introduce QEMUD_CMD_FLAG_PCIDEVICE indicating that the -pcidevice command line option is available

* tests/*: update the tests --- src/qemu_conf.c | 8 ++++++++ src/qemu_conf.h | 2 ++ tests/qemuargv2xmltest.c | 2 +- tests/qemuhelptest.c | 6 ++++-- tests/qemuxml2argvtest.c | 2 +- 5 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 6b0b404..1b160c9 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -771,6 +771,9 @@ static unsigned int qemudComputeCmdFlags(const char *help, flags |= QEMUD_CMD_FLAG_VGA; if (strstr(help, "boot=on")) flags |= QEMUD_CMD_FLAG_DRIVE_BOOT; + if (strstr(help, "-pcidevice")) + flags |= QEMUD_CMD_FLAG_PCIDEVICE; + if (version >= 9000) flags |= QEMUD_CMD_FLAG_VNC_COLON;

@@ -2070,6 +2073,11 @@ int qemudBuildCommandLine(virConnectPtr conn, /* PCI */ if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) { + if (!(qemuCmdFlags & QEMUD_CMD_FLAG_PCIDEVICE)) { + qemudReportError(conn, NULL, NULL, VIR_ERR_NO_SUPPORT, "%s", + _("PCI device assignment is not supported by this version of qemu")); + goto error; + } ret = virAsprintf(&pcidev, "host=%.2x:%.2x.%.1x", hostdev->source.subsys.u.pci.bus, hostdev->source.subsys.u.pci.slot, diff --git a/src/qemu_conf.h b/src/qemu_conf.h index 517626a..aea9843 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -64,6 +64,8 @@ enum qemud_cmd_flags { QEMUD_CMD_FLAG_0_10 = (1 << 16), QEMUD_CMD_FLAG_NET_NAME = QEMUD_CMD_FLAG_0_10, /* -net ...,name=str */ QEMUD_CMD_FLAG_HOST_NET_ADD = QEMUD_CMD_FLAG_0_10, /* host_net_add monitor command */ + + QEMUD_CMD_FLAG_PCIDEVICE = (1 << 17), /* PCI device assignment only supported by qemu-kvm */ };

/* Main driver state */ diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index f2537b7..7861520 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -216,7 +216,7 @@ mymain(int argc, char **argv) DO_TEST("hostdev-usb-product", 0); DO_TEST("hostdev-usb-address", 0);

- DO_TEST("hostdev-pci-address", 0); + DO_TEST("hostdev-pci-address", QEMUD_CMD_FLAG_PCIDEVICE);

DO_TEST_FULL("restore-v1", QEMUD_CMD_FLAG_MIGRATE_KVM_STDIO, "stdio"); DO_TEST_FULL("restore-v2", QEMUD_CMD_FLAG_MIGRATE_QEMU_EXEC, "stdio"); diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c index 1948bd1..ad2045f 100644 --- a/tests/qemuhelptest.c +++ b/tests/qemuhelptest.c @@ -135,7 +135,8 @@ mymain(int argc, char **argv) QEMUD_CMD_FLAG_KVM | QEMUD_CMD_FLAG_DRIVE_FORMAT | QEMUD_CMD_FLAG_VGA | - QEMUD_CMD_FLAG_0_10, + QEMUD_CMD_FLAG_0_10 | + QEMUD_CMD_FLAG_PCIDEVICE, 10005, 1, 0); DO_TEST("kvm-86", QEMUD_CMD_FLAG_VNC_COLON | @@ -151,7 +152,8 @@ mymain(int argc, char **argv) QEMUD_CMD_FLAG_KVM | QEMUD_CMD_FLAG_DRIVE_FORMAT | QEMUD_CMD_FLAG_VGA | - QEMUD_CMD_FLAG_0_10, + QEMUD_CMD_FLAG_0_10 | + QEMUD_CMD_FLAG_PCIDEVICE, 10050, 1, 0);

return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 73a6709..6f25e7d 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -263,7 +263,7 @@ mymain(int argc, char **argv) DO_TEST("hostdev-usb-product", 0); DO_TEST("hostdev-usb-address", 0);

- DO_TEST("hostdev-pci-address", 0); + DO_TEST("hostdev-pci-address", QEMUD_CMD_FLAG_PCIDEVICE);

DO_TEST_FULL("restore-v1", QEMUD_CMD_FLAG_MIGRATE_KVM_STDIO, "stdio"); DO_TEST_FULL("restore-v2", QEMUD_CMD_FLAG_MIGRATE_QEMU_EXEC, "stdio");

ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Thu, Aug 13, 2009 at 05:44:33PM +0100, Mark McLoughlin wrote:

It turns out that a PCI Power Management reset only affects individual functions, and not the whole device.

The PCI Power Management spec talks about resetting the 'device' rather than the 'function', but Intel's Dexuan Cui informs me that it is actually a per-function reset.

Also, Yu Zhao has added pci_pm_reset() to the kernel, and it doesn't reject multi-function devices, so it must be true! :-)

(A side issue is that we could defer the PM reset to the kernel if we could detect that the kernel has PM reset support, but barring version number checks we don't have a way to detect that support)

* src/pci.c: remove the pciDeviceContainsOtherFunctions() check from pciTryPowerManagementReset() and prefer PM reset over bus reset where both are available

Cc: Cui, Dexuan <dexuan.cui@intel.com> Cc: Yu Zhao <yu.zhao@intel.com> --- src/pci.c | 48 +++++++++--------------------------------------- 1 files changed, 9 insertions(+), 39 deletions(-)

diff --git a/src/pci.c b/src/pci.c index 2dc2e1c..11b3e8b 100644 --- a/src/pci.c +++ b/src/pci.c @@ -402,29 +402,6 @@ pciBusContainsOtherDevices(virConnectPtr conn, pciDevice *dev) return 1; }

-/* Any other functions on this device ? */ -static int -pciSharesDevice(pciDevice *a, pciDevice *b) -{ - return - a->domain == b->domain && - a->bus == b->bus && - a->slot == b->slot && - a->function != b->function; -} - -static int -pciDeviceContainsOtherFunctions(virConnectPtr conn, pciDevice *dev) -{ - pciDevice *matched = NULL; - if (pciIterDevices(conn, pciSharesDevice, dev, &matched) < 0) - return 1; - if (!matched) - return 0; - pciFreeDevice(conn, matched); - return 1; -} - /* Is @a the parent of @b ? */ static int pciIsParent(pciDevice *a, pciDevice *b) @@ -529,7 +506,7 @@ out: * above we require the device supports a full internal reset. */ static int -pciTryPowerManagementReset(virConnectPtr conn, pciDevice *dev) +pciTryPowerManagementReset(virConnectPtr conn ATTRIBUTE_UNUSED, pciDevice *dev) { uint8_t config_space[PCI_CONF_LEN]; uint32_t ctl; @@ -537,16 +514,6 @@ pciTryPowerManagementReset(virConnectPtr conn, pciDevice *dev) if (!dev->pci_pm_cap_pos) return -1;

- /* For now, we just refuse to do a power management reset - * if there are other functions on this device. - * In future, we could allow it so long as those functions - * are not in use by the host or other guests. - */ - if (pciDeviceContainsOtherFunctions(conn, dev)) { - VIR_WARN("%s contains other functions, not resetting", dev->name); - return -1; - } - /* Save and restore the device's config space. */ if (pciRead(dev, 0, &config_space[0], PCI_CONF_LEN) < 0) { VIR_WARN("Failed to save PCI config space for %s", dev->name); @@ -604,14 +571,17 @@ pciResetDevice(virConnectPtr conn, pciDevice *dev) if (dev->has_flr) return 0;

+ /* If the device supports PCI power management reset, + * that's the next best thing because it only resets + * the function, not the whole device. + */ + if (dev->has_pm_reset) + ret = pciTryPowerManagementReset(conn, dev); + /* Bus reset is not an option with the root bus */ - if (dev->bus != 0) + if (ret < 0 && dev->bus != 0) ret = pciTrySecondaryBusReset(conn, dev);

- /* Next best option is a PCI power management reset */ - if (ret < 0 && dev->has_pm_reset) - ret = pciTryPowerManagementReset(conn, dev); - if (ret < 0) pciReportError(conn, VIR_ERR_NO_SUPPORT, _("No PCI reset capability available for %s"),

ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Thu, Aug 13, 2009 at 05:44:34PM +0100, Mark McLoughlin wrote:

When the guest shuts down, we should attempt to restore all PCI host devices to a sane state.

In the case of managed hostdevs, we should reset and re-attach the devices. In the case of unmanaged hostdevs, we should just reset them.

Note, KVM will already reset assigned devices when the guest shuts down using whatever means it can, so we are only doing it to cover the cases the kernel can't handle.

* src/qemu_driver.c: add qemuDomainReAttachHostDevices() and call it from qemudShutdownVMDaemon() --- src/qemu_driver.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 76 insertions(+), 0 deletions(-)

diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 041e3da..ed2f3c4 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1402,6 +1402,80 @@ error: return -1; }

+static void +qemuDomainReAttachHostDevices(virConnectPtr conn, virDomainDefPtr def) +{ + int i; + + /* Again 2 loops; reset all the devices before re-attach */ + + for (i = 0 ; i < def->nhostdevs ; i++) { + virDomainHostdevDefPtr hostdev = def->hostdevs[i]; + pciDevice *dev; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + continue; + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) + continue; + + dev = pciGetDevice(conn, + hostdev->source.subsys.u.pci.domain, + hostdev->source.subsys.u.pci.bus, + hostdev->source.subsys.u.pci.slot, + hostdev->source.subsys.u.pci.function); + if (!dev) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to allocate pciDevice: %s\n"), + err ? err->message : ""); + virResetError(err); + continue; + } + + if (pciResetDevice(conn, dev) < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to reset PCI device: %s\n"), + err ? err->message : ""); + virResetError(err); + } + + pciFreeDevice(conn, dev); + } + + for (i = 0 ; i < def->nhostdevs ; i++) { + virDomainHostdevDefPtr hostdev = def->hostdevs[i]; + pciDevice *dev; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + continue; + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) + continue; + if (!hostdev->managed) + continue; + + dev = pciGetDevice(conn, + hostdev->source.subsys.u.pci.domain, + hostdev->source.subsys.u.pci.bus, + hostdev->source.subsys.u.pci.slot, + hostdev->source.subsys.u.pci.function); + if (!dev) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to allocate pciDevice: %s\n"), + err ? err->message : ""); + virResetError(err); + continue; + } + + if (pciDettachDevice(conn, dev) < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to reset PCI device: %s\n"), + err ? err->message : ""); + virResetError(err); + } + + pciFreeDevice(conn, dev); + } +} + static const char *const defaultDeviceACL[] = { "/dev/null", "/dev/full", "/dev/zero", "/dev/random", "/dev/urandom", @@ -2109,6 +2183,8 @@ static void qemudShutdownVMDaemon(virConnectPtr conn, VIR_WARN("Failed to restore all device ownership for %s", vm->def->name);

+ qemuDomainReAttachHostDevices(conn, vm->def); + retry: if ((ret = qemuRemoveCgroup(conn, driver, vm)) < 0) { if (ret == -EBUSY && (retries++ < 5)) {

ACk Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Thu, Aug 13, 2009 at 05:44:35PM +0100, Mark McLoughlin wrote:

Currently, if we are unable to reset a PCI device we return a fairly generic 'No PCI reset capability available' error message.

Fix that by returning an error from the individual reset messages and using that error to construct the higher level error mesage.

* src/pci.c: set errors in pciTryPowerManagementReset() and pciTrySecondaryBusReset() on failure; use those error messages in pciResetDevice(), or explain that no reset support is available --- src/pci.c | 44 +++++++++++++++++++++++++++++++------------- src/qemu_driver.c | 4 ++-- 2 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/src/pci.c b/src/pci.c index 11b3e8b..74f7ef0 100644 --- a/src/pci.c +++ b/src/pci.c @@ -456,15 +456,18 @@ pciTrySecondaryBusReset(virConnectPtr conn, pciDevice *dev) * are not in use by the host or other guests. */ if (pciBusContainsOtherDevices(conn, dev)) { - VIR_WARN("Other devices on bus with %s, not doing bus reset", - dev->name); + pciReportError(conn, VIR_ERR_NO_SUPPORT, + _("Other devices on bus with %s, not doing bus reset"), + dev->name); return -1; }

/* Find the parent bus */ parent = pciGetParentDevice(conn, dev); if (!parent) { - VIR_WARN("Failed to find parent device for %s", dev->name); + pciReportError(conn, VIR_ERR_NO_SUPPORT, + _("Failed to find parent device for %s"), + dev->name); return -1; }

@@ -475,7 +478,9 @@ pciTrySecondaryBusReset(virConnectPtr conn, pciDevice *dev) * are multiple devices/functions */ if (pciRead(dev, 0, config_space, PCI_CONF_LEN) < 0) { - VIR_WARN("Failed to save PCI config space for %s", dev->name); + pciReportError(conn, VIR_ERR_NO_SUPPORT, + _("Failed to save PCI config space for %s"), + dev->name); goto out; }

@@ -492,9 +497,12 @@ pciTrySecondaryBusReset(virConnectPtr conn, pciDevice *dev)

usleep(200 * 1000); /* sleep 200ms */

- if (pciWrite(dev, 0, config_space, PCI_CONF_LEN) < 0) - VIR_WARN("Failed to restore PCI config space for %s", dev->name); - + if (pciWrite(dev, 0, config_space, PCI_CONF_LEN) < 0) { + pciReportError(conn, VIR_ERR_NO_SUPPORT, + _("Failed to restore PCI config space for %s"), + dev->name); + goto out; + } ret = 0; out: pciFreeDevice(conn, parent); @@ -516,7 +524,9 @@ pciTryPowerManagementReset(virConnectPtr conn ATTRIBUTE_UNUSED, pciDevice *dev)

/* Save and restore the device's config space. */ if (pciRead(dev, 0, &config_space[0], PCI_CONF_LEN) < 0) { - VIR_WARN("Failed to save PCI config space for %s", dev->name); + pciReportError(conn, VIR_ERR_NO_SUPPORT, + _("Failed to save PCI config space for %s"), + dev->name); return -1; }

@@ -533,8 +543,12 @@ pciTryPowerManagementReset(virConnectPtr conn ATTRIBUTE_UNUSED, pciDevice *dev)

usleep(10 * 1000); /* sleep 10ms */

- if (pciWrite(dev, 0, &config_space[0], PCI_CONF_LEN) < 0) - VIR_WARN("Failed to restore PCI config space for %s", dev->name); + if (pciWrite(dev, 0, &config_space[0], PCI_CONF_LEN) < 0) { + pciReportError(conn, VIR_ERR_NO_SUPPORT, + _("Failed to restore PCI config space for %s"), + dev->name); + return -1; + }

return 0; } @@ -582,10 +596,14 @@ pciResetDevice(virConnectPtr conn, pciDevice *dev) if (ret < 0 && dev->bus != 0) ret = pciTrySecondaryBusReset(conn, dev);

- if (ret < 0) + if (ret < 0) { + virErrorPtr err = virGetLastError(); pciReportError(conn, VIR_ERR_NO_SUPPORT, - _("No PCI reset capability available for %s"), - dev->name); + _("Unable to reset PCI device %s: %s"), + dev->name, + err ? err->message : _("no FLR, PM reset or bus reset available")); + } + return ret; }

diff --git a/src/qemu_driver.c b/src/qemu_driver.c index ed2f3c4..6d1ec06 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1465,9 +1465,9 @@ qemuDomainReAttachHostDevices(virConnectPtr conn, virDomainDefPtr def) continue; }

- if (pciDettachDevice(conn, dev) < 0) { + if (pciReAttachDevice(conn, dev) < 0) { virErrorPtr err = virGetLastError(); - VIR_ERROR(_("Failed to reset PCI device: %s\n"), + VIR_ERROR(_("Failed to re-attach PCI device: %s\n"), err ? err->message : ""); virResetError(err); }

ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Thu, Aug 13, 2009 at 05:44:36PM +0100, Mark McLoughlin wrote:

When using a Secondary Bus Reset, all devices on the bus are reset.

Extend the pciResetDevice() API so that a 'check' callback can be supplied which will verify that it is safe to reset the other devices on the bus.

The virDomainObjPtr parameter is needed so that when the check function iterates over the domain list, it can avoid double locking.

* src/pci.[ch]: add a 'check' callback to pciResetDevice(), re-work pciIterDevices() to pass the check function to the iter functions, use the check function in the bus iterator, return the first unsafe device from pciBusCheckOtherDevices() and include its details in the bus reset error message.

* src/qemu_driver.c, src/xen_uninified.c: just pass NULL as the check function for now --- src/pci.c | 113 +++++++++++++++++++++++++++++++++------------------- src/pci.h | 15 ++++++- src/qemu_driver.c | 21 ++++++---- src/xen_unified.c | 2 +- 4 files changed, 98 insertions(+), 53 deletions(-)

diff --git a/src/pci.c b/src/pci.c index 74f7ef0..6a2e860 100644 --- a/src/pci.c +++ b/src/pci.c @@ -225,7 +225,11 @@ pciWrite32(pciDevice *dev, unsigned pos, uint32_t val) pciWrite(dev, pos, &buf[0], sizeof(buf)); }

-typedef int (*pciIterPredicate)(pciDevice *, pciDevice *); +typedef int (*pciIterPredicate)(virConnectPtr, + virDomainObjPtr, + pciDevice *, + pciResetCheckFunc, + pciDevice *);

/* Iterate over available PCI devices calling @predicate * to compare each one to @dev. @@ -234,8 +238,10 @@ typedef int (*pciIterPredicate)(pciDevice *, pciDevice *); */ static int pciIterDevices(virConnectPtr conn, + virDomainObjPtr vm, pciIterPredicate predicate, pciDevice *dev, + pciResetCheckFunc check, pciDevice **matched) { DIR *dir; @@ -254,7 +260,7 @@ pciIterDevices(virConnectPtr conn,

while ((entry = readdir(dir))) { unsigned domain, bus, slot, function; - pciDevice *try; + pciDevice *check_dev;

/* Ignore '.' and '..' */ if (entry->d_name[0] == '.') @@ -266,18 +272,18 @@ pciIterDevices(virConnectPtr conn, continue; }

- try = pciGetDevice(conn, domain, bus, slot, function); - if (!try) { + check_dev = pciGetDevice(conn, domain, bus, slot, function); + if (!check_dev) { ret = -1; break; }

- if (predicate(try, dev)) { - VIR_DEBUG("%s %s: iter matched on %s", dev->id, dev->name, try->name); - *matched = try; + if (predicate(conn, vm, dev, check, check_dev)) { + VIR_DEBUG("%s %s: iter matched on %s", dev->id, dev->name, check_dev->name); + *matched = check_dev; break; } - pciFreeDevice(conn, try); + pciFreeDevice(conn, check_dev); } closedir(dir); return ret; @@ -379,63 +385,73 @@ pciDetectPowerManagementReset(pciDevice *dev) return 0; }

-/* Any devices other than the one supplied on the same domain/bus ? */ +/* Check any devices other than the one supplied on the same domain/bus */ static int -pciSharesBus(pciDevice *a, pciDevice *b) +pciCheckSharesBus(virConnectPtr conn, + virDomainObjPtr vm, + pciDevice *dev, + pciResetCheckFunc check, + pciDevice *check_dev) { - return - a->domain == b->domain && - a->bus == b->bus && - (a->slot != b->slot || - a->function != b->function); + if (check_dev->domain != dev->domain || check_dev->bus != dev->bus) + return 0; + if (check_dev->slot == dev->slot && check_dev->function == dev->function) + return 0; + if (check(conn, vm, check_dev)) + return 0; + return 1; }

-static int -pciBusContainsOtherDevices(virConnectPtr conn, pciDevice *dev) +static pciDevice * +pciBusCheckOtherDevices(virConnectPtr conn, + virDomainObjPtr vm, + pciDevice *dev, + pciResetCheckFunc check) { - pciDevice *matched = NULL; - if (pciIterDevices(conn, pciSharesBus, dev, &matched) < 0) - return 1; - if (!matched) - return 0; - pciFreeDevice(conn, matched); - return 1; + pciDevice *conflict = NULL; + pciIterDevices(conn, vm, pciCheckSharesBus, dev, check, &conflict); + return conflict; }

-/* Is @a the parent of @b ? */ +/* Is @check_dev the parent of @dev ? */ static int -pciIsParent(pciDevice *a, pciDevice *b) +pciIsParent(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainObjPtr vm ATTRIBUTE_UNUSED, + pciDevice *dev, + pciResetCheckFunc check ATTRIBUTE_UNUSED, + pciDevice *check_dev) { uint16_t device_class; uint8_t header_type, secondary, subordinate;

- if (a->domain != b->domain) + if (check_dev->domain != dev->domain) return 0;

/* Is it a bridge? */ - device_class = pciRead16(a, PCI_CLASS_DEVICE); + device_class = pciRead16(check_dev, PCI_CLASS_DEVICE); if (device_class != PCI_CLASS_BRIDGE_PCI) return 0;

/* Is it a plane? */ - header_type = pciRead8(a, PCI_HEADER_TYPE); + header_type = pciRead8(check_dev, PCI_HEADER_TYPE); if ((header_type & PCI_HEADER_TYPE_MASK) != PCI_HEADER_TYPE_BRIDGE) return 0;

- secondary = pciRead8(a, PCI_SECONDARY_BUS); - subordinate = pciRead8(a, PCI_SUBORDINATE_BUS); + secondary = pciRead8(check_dev, PCI_SECONDARY_BUS); + subordinate = pciRead8(check_dev, PCI_SUBORDINATE_BUS);

- VIR_DEBUG("%s %s: found parent device %s\n", b->id, b->name, a->name); + VIR_DEBUG("%s %s: found parent device %s\n", + dev->id, dev->name, check_dev->name);

/* No, it's superman! */ - return (b->bus >= secondary && b->bus <= subordinate); + return (dev->bus >= secondary && dev->bus <= subordinate); }

static pciDevice * pciGetParentDevice(virConnectPtr conn, pciDevice *dev) { pciDevice *parent = NULL; - pciIterDevices(conn, pciIsParent, dev, &parent); + pciIterDevices(conn, NULL, pciIsParent, dev, NULL, &parent); return parent; }

@@ -443,9 +459,12 @@ pciGetParentDevice(virConnectPtr conn, pciDevice *dev) * devices behind a bus. */ static int -pciTrySecondaryBusReset(virConnectPtr conn, pciDevice *dev) +pciTrySecondaryBusReset(virConnectPtr conn, + virDomainObjPtr vm, + pciDevice *dev, + pciResetCheckFunc check) { - pciDevice *parent; + pciDevice *parent, *conflict; uint8_t config_space[PCI_CONF_LEN]; uint16_t ctl; int ret = -1; @@ -455,10 +474,11 @@ pciTrySecondaryBusReset(virConnectPtr conn, pciDevice *dev) * In future, we could allow it so long as those devices * are not in use by the host or other guests. */ - if (pciBusContainsOtherDevices(conn, dev)) { + if ((conflict = pciBusCheckOtherDevices(conn, vm, dev, check))) { pciReportError(conn, VIR_ERR_NO_SUPPORT, - _("Other devices on bus with %s, not doing bus reset"), - dev->name); + _("Unable to reset %s using bus reset as this would cause %s to be reset"), + dev->name, conflict->name); + pciFreeDevice(conn, conflict); return -1; }

@@ -572,13 +592,24 @@ pciInitDevice(virConnectPtr conn, pciDevice *dev) }

int -pciResetDevice(virConnectPtr conn, pciDevice *dev) +pciResetDevice(virConnectPtr conn, + virDomainObjPtr vm, + pciDevice *dev, + pciResetCheckFunc check) { int ret = -1;

if (!dev->initted && pciInitDevice(conn, dev) < 0) return -1;

+ /* Check that the device isn't owned by a running VM */ + if (!check(conn, vm, dev)) { + pciReportError(conn, VIR_ERR_NO_SUPPORT, + _("Unable to reset PCI device %s: device is in use"), + dev->name); + return -1; + } + /* KVM will perform FLR when starting and stopping * a guest, so there is no need for us to do it here. */ @@ -594,7 +625,7 @@ pciResetDevice(virConnectPtr conn, pciDevice *dev)

/* Bus reset is not an option with the root bus */ if (ret < 0 && dev->bus != 0) - ret = pciTrySecondaryBusReset(conn, dev); + ret = pciTrySecondaryBusReset(conn, vm, dev, check);

if (ret < 0) { virErrorPtr err = virGetLastError(); diff --git a/src/pci.h b/src/pci.h index 47882ef..15da057 100644 --- a/src/pci.h +++ b/src/pci.h @@ -24,6 +24,7 @@

#include <config.h> #include "internal.h" +#include "domain_conf.h"

typedef struct _pciDevice pciDevice;

@@ -38,7 +39,17 @@ int pciDettachDevice (virConnectPtr conn, pciDevice *dev); int pciReAttachDevice (virConnectPtr conn, pciDevice *dev); -int pciResetDevice (virConnectPtr conn, - pciDevice *dev); + +/* pciResetDevice() may cause other devices to be reset; + * The check function is called for each other device to + * be reset and by returning zero may cause the reset to + * fail if it is not safe to reset the supplied device. + */ +typedef int (*pciResetCheckFunc)(virConnectPtr, virDomainObjPtr, pciDevice *); + +int pciResetDevice(virConnectPtr conn, + virDomainObjPtr vm, + pciDevice *dev, + pciResetCheckFunc check);

#endif /* __VIR_PCI_H__ */ diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 6d1ec06..bfa06a5 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1329,8 +1329,10 @@ static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) { return -1; }

-static int qemuPrepareHostDevices(virConnectPtr conn, - virDomainDefPtr def) { +static int +qemuPrepareHostDevices(virConnectPtr conn, virDomainObjPtr vm) +{ + virDomainDefPtr def = vm->def; int i;

/* We have to use 2 loops here. *All* devices must @@ -1388,7 +1390,7 @@ static int qemuPrepareHostDevices(virConnectPtr conn, if (!dev) goto error;

- if (pciResetDevice(conn, dev) < 0) { + if (pciResetDevice(conn, vm, dev, NULL) < 0) { pciFreeDevice(conn, dev); goto error; } @@ -1403,8 +1405,9 @@ error: }

static void -qemuDomainReAttachHostDevices(virConnectPtr conn, virDomainDefPtr def) +qemuDomainReAttachHostDevices(virConnectPtr conn, virDomainObjPtr vm) { + virDomainDefPtr def = vm->def; int i;

/* Again 2 loops; reset all the devices before re-attach */ @@ -1431,7 +1434,7 @@ qemuDomainReAttachHostDevices(virConnectPtr conn, virDomainDefPtr def) continue; }

- if (pciResetDevice(conn, dev) < 0) { + if (pciResetDevice(conn, vm, dev, NULL) < 0) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to reset PCI device: %s\n"), err ? err->message : ""); @@ -2001,7 +2004,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, if (qemuSetupCgroup(conn, driver, vm) < 0) goto cleanup;

- if (qemuPrepareHostDevices(conn, vm->def) < 0) + if (qemuPrepareHostDevices(conn, vm) < 0) goto cleanup;

if (VIR_ALLOC(vm->monitor_chr) < 0) { @@ -2183,7 +2186,7 @@ static void qemudShutdownVMDaemon(virConnectPtr conn, VIR_WARN("Failed to restore all device ownership for %s", vm->def->name);

- qemuDomainReAttachHostDevices(conn, vm->def); + qemuDomainReAttachHostDevices(conn, vm);

retry: if ((ret = qemuRemoveCgroup(conn, driver, vm)) < 0) { @@ -5247,7 +5250,7 @@ static int qemudDomainAttachHostPciDevice(virConnectPtr conn, return -1;

if (pciDettachDevice(conn, pci) < 0 || - pciResetDevice(conn, pci) < 0) { + pciResetDevice(conn, vm, pci, NULL) < 0) { pciFreeDevice(conn, pci); return -1; } @@ -7049,7 +7052,7 @@ qemudNodeDeviceReset (virNodeDevicePtr dev) if (!pci) return -1;

- if (pciResetDevice(dev->conn, pci) < 0) + if (pciResetDevice(dev->conn, NULL, pci, NULL) < 0) goto out;

ret = 0; diff --git a/src/xen_unified.c b/src/xen_unified.c index f2ffc25..dd8f10b 100644 --- a/src/xen_unified.c +++ b/src/xen_unified.c @@ -1641,7 +1641,7 @@ xenUnifiedNodeDeviceReset (virNodeDevicePtr dev) if (!pci) return -1;

- if (pciResetDevice(dev->conn, pci) < 0) + if (pciResetDevice(dev->conn, NULL, pci, NULL) < 0) goto out;

ret = 0;

ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Thu, Aug 13, 2009 at 05:44:37PM +0100, Mark McLoughlin wrote:

If a PCI device reset causes other devices to be reset, allow it so long as those other devices are note assigned to another active domain.

Note, we need to take the driver lock qemudNodeDeviceReset() because the check function will iterate over the domain list.

* src/qemu_conf.c: add qemuCheckPciHostDevice() to iterate over active domains checking whether the affected device is assigned

* src/pci.[ch]: add pciDeviceEquals() helper --- src/libvirt_private.syms | 1 + src/pci.c | 15 ++++++++++++ src/pci.h | 7 ++++++ src/qemu_driver.c | 54 ++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 642c2bc..23fa01b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -283,6 +283,7 @@ virNodeDeviceAssignDef; pciGetDevice; pciFreeDevice; pciDettachDevice; +pciDeviceEquals; pciReAttachDevice; pciResetDevice;

diff --git a/src/pci.c b/src/pci.c index 6a2e860..619853b 100644 --- a/src/pci.c +++ b/src/pci.c @@ -926,3 +926,18 @@ pciFreeDevice(virConnectPtr conn ATTRIBUTE_UNUSED, pciDevice *dev) close(dev->fd); VIR_FREE(dev); } + +int +pciDeviceEquals(virConnectPtr conn ATTRIBUTE_UNUSED, + pciDevice *dev, + unsigned domain, + unsigned bus, + unsigned slot, + unsigned function) +{ + return + dev->domain == domain && + dev->bus == bus && + dev->slot == slot && + dev->function == function; +} diff --git a/src/pci.h b/src/pci.h index 15da057..d5e680c 100644 --- a/src/pci.h +++ b/src/pci.h @@ -52,4 +52,11 @@ int pciResetDevice(virConnectPtr conn, pciDevice *dev, pciResetCheckFunc check);

+int pciDeviceEquals(virConnectPtr conn, + pciDevice *dev, + unsigned domain, + unsigned bus, + unsigned slot, + unsigned function); + #endif /* __VIR_PCI_H__ */ diff --git a/src/qemu_driver.c b/src/qemu_driver.c index bfa06a5..4b1aeea 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1330,6 +1330,48 @@ static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) { }

static int +qemuCheckPciHostDevice(virConnectPtr conn, + virDomainObjPtr owner_vm, + pciDevice *dev) +{ + struct qemud_driver *driver = conn->privateData; + int ret = 1, i; + + for (i = 0; i < driver->domains.count && ret; i++) { + virDomainObjPtr vm = driver->domains.objs[i]; + + if (vm == owner_vm) + continue; + + virDomainObjLock(vm); + + if (virDomainIsActive(vm)) { + int j; + + for (j = 0; j < vm->def->nhostdevs && ret; j++) { + virDomainHostdevDefPtr hostdev = vm->def->hostdevs[j]; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + continue; + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) + continue; + + if (pciDeviceEquals(conn, dev, + hostdev->source.subsys.u.pci.domain, + hostdev->source.subsys.u.pci.bus, + hostdev->source.subsys.u.pci.slot, + hostdev->source.subsys.u.pci.function)) + ret = 0; + } + } + + virDomainObjUnlock(vm); + } + + return ret; +} + +static int qemuPrepareHostDevices(virConnectPtr conn, virDomainObjPtr vm) { virDomainDefPtr def = vm->def; @@ -1390,7 +1432,7 @@ qemuPrepareHostDevices(virConnectPtr conn, virDomainObjPtr vm) if (!dev) goto error;

- if (pciResetDevice(conn, vm, dev, NULL) < 0) { + if (pciResetDevice(conn, vm, dev, qemuCheckPciHostDevice) < 0) { pciFreeDevice(conn, dev); goto error; } @@ -1434,7 +1476,7 @@ qemuDomainReAttachHostDevices(virConnectPtr conn, virDomainObjPtr vm) continue; }

- if (pciResetDevice(conn, vm, dev, NULL) < 0) { + if (pciResetDevice(conn, vm, dev, qemuCheckPciHostDevice) < 0) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to reset PCI device: %s\n"), err ? err->message : ""); @@ -5250,7 +5292,7 @@ static int qemudDomainAttachHostPciDevice(virConnectPtr conn, return -1;

if (pciDettachDevice(conn, pci) < 0 || - pciResetDevice(conn, vm, pci, NULL) < 0) { + pciResetDevice(conn, vm, pci, qemuCheckPciHostDevice) < 0) { pciFreeDevice(conn, pci); return -1; } @@ -7041,6 +7083,7 @@ out: static int qemudNodeDeviceReset (virNodeDevicePtr dev) { + struct qemud_driver *driver = dev->conn->privateData; pciDevice *pci; unsigned domain, bus, slot, function; int ret = -1; @@ -7052,11 +7095,14 @@ qemudNodeDeviceReset (virNodeDevicePtr dev) if (!pci) return -1;

- if (pciResetDevice(dev->conn, NULL, pci, NULL) < 0) + qemuDriverLock(driver); + + if (pciResetDevice(dev->conn, NULL, pci, qemuCheckPciHostDevice) < 0) goto out;

ret = 0; out: + qemuDriverUnlock(driver); pciFreeDevice(dev->conn, pci); return ret; }

ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|