On Mon, Aug 17, 2009 at 03:10:16PM +0100, Mark McLoughlin wrote:

Right now we're only resetting managed devices before hotplug, but we should reset them irrespective of whether they are managed.

* src/qemu_driver.c: reset all PCI hostdevs before hotplug --- src/qemu_driver.c | 27 +++++++++++++-------------- 1 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/src/qemu_driver.c b/src/qemu_driver.c index a638c10..06bbf2a 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -5310,30 +5310,29 @@ static int qemudDomainAttachHostPciDevice(virConnectPtr conn, virDomainHostdevDefPtr hostdev = dev->data.hostdev; char *cmd, *reply; unsigned domain, bus, slot; + pciDevice *pci;

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; - } + 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 ((hostdev->managed && 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,

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 Mon, Aug 17, 2009 at 03:10:17PM +0100, Mark McLoughlin wrote:

When we hot-unplug a PCI host device from a guest, we should reset it.

Both managed and unmanaged devices should be reset, but only managed devices should be re-attached.

* src/qemu_driver.c: reset devices in qemudDomainDetachHostPciDevice() --- src/qemu_driver.c | 22 +++++++++++++--------- 1 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 06bbf2a..187497f 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -5780,6 +5780,7 @@ static int qemudDomainDetachHostPciDevice(virConnectPtr conn, virDomainHostdevDefPtr detach; char *cmd, *reply; int i, ret; + pciDevice *pci;

for (i = 0 ; i < vm->def->nhostdevs ; i++) { unsigned domain = vm->def->hostdevs[i]->source.subsys.u.pci.domain; @@ -5848,16 +5849,19 @@ static int qemudDomainDetachHostPciDevice(virConnectPtr conn,

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) + 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) + ret = -1; + else { + if (pciResetDevice(conn, pci) < 0) + ret = -1; + if (detach->managed && pciReAttachDevice(conn, pci) < 0) ret = -1; - if (pci) - pciFreeDevice(conn, pci); + pciFreeDevice(conn, pci); }

if (i != --vm->def->nhostdevs)

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 Mon, Aug 17, 2009 at 03:10:18PM +0100, Mark McLoughlin wrote:

* src/qemu_driver.c: Add missing break statement in qemudDomainDetachHostDevice() --- src/qemu_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 187497f..048e1c0 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -5894,6 +5894,7 @@ static int qemudDomainDetachHostDevice(virConnectPtr conn, switch (hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: ret = qemudDomainDetachHostPciDevice(conn, vm, dev); + break; default: qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT, _("hostdev subsys type '%s' not supported"),

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 Mon, Aug 17, 2009 at 03:10:19PM +0100, Mark McLoughlin wrote:

Newer versions of QEMU accept 'pci_add auto', but older versions require 'pci_add pci_addr=auto'

Ah yes, storage already does that too

* src/qemu_driver.c: use pci_addr= in qemudDomainAttachHostPciDevice() for older versions of QEMU --- src/qemu_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 048e1c0..a9da387 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -5333,7 +5333,7 @@ static int qemudDomainAttachHostPciDevice(virConnectPtr conn,

pciFreeDevice(conn, pci);

- if (virAsprintf(&cmd, "pci_add auto host host=%.2x:%.2x.%.1x", + if (virAsprintf(&cmd, "pci_add pci_addr=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) {

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 Mon, Aug 17, 2009 at 03:10:20PM +0100, Mark McLoughlin wrote:

The qemuPrepareHostDevices() and qemuDomainReAttachHostDevices() functions are clutter with a bunch of calls to pciGetDevice() and pciFreeDevice() obscuring the basic logic.

Add a pciDeviceList type and add a qemuGetPciHostDeviceList() function to build a list from a domain definition. Use this in prepare/re-attach fto simplify things and eliminate the multiple pciGetDevice calls.

This is especially useful because in the next patch we need to iterate the hostdevs list a third time and we also need a list type for keeping track of active devices.

* src/pci.[ch]: add pciDeviceList type and also a per-device 'managed' property

* src/libvirt_private.syms: export the new functions

* src/qemu_driver.c: add qemuGetPciHostDeviceList() and re-write qemuPrepareHostDevices() and qemuDomainReAttachHostDevices() to use it --- src/libvirt_private.syms | 7 ++- src/pci.c | 109 ++++++++++++++++++++++++++++++ src/pci.h | 20 ++++++ src/qemu_driver.c | 167 +++++++++++++++++++-------------------------- 4 files changed, 206 insertions(+), 97 deletions(-)

Yeah that's nicer readability in the QEMU driver.

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 642c2bc..2bf4e15 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -285,7 +285,12 @@ pciFreeDevice; pciDettachDevice; pciReAttachDevice; pciResetDevice; - +pciDeviceSetManaged; +pciDeviceGetManaged; +pciDeviceListNew; +pciDeviceListFree; +pciDeviceListAdd; +pciDeviceListDel;

# qparams.h qparam_get_query; diff --git a/src/pci.c b/src/pci.c index 74f7ef0..a37eaf7 100644 --- a/src/pci.c +++ b/src/pci.c @@ -63,6 +63,7 @@ struct _pciDevice { unsigned pci_pm_cap_pos; unsigned has_flr : 1; unsigned has_pm_reset : 1; + unsigned managed : 1; };

/* For virReportOOMError() and virReportSystemError() */ @@ -890,8 +891,116 @@ pciGetDevice(virConnectPtr conn, void pciFreeDevice(virConnectPtr conn ATTRIBUTE_UNUSED, pciDevice *dev) { + if (!dev) + return; VIR_DEBUG("%s %s: freeing", dev->id, dev->name); if (dev->fd >= 0) close(dev->fd); VIR_FREE(dev); } + +void pciDeviceSetManaged(pciDevice *dev, unsigned managed) +{ + dev->managed = !!managed; +} + +unsigned pciDeviceGetManaged(pciDevice *dev) +{ + return dev->managed; +} + +pciDeviceList * +pciDeviceListNew(virConnectPtr conn) +{ + pciDeviceList *list; + + if (VIR_ALLOC(list) < 0) { + virReportOOMError(conn); + return NULL; + } + + return list; +} + +void +pciDeviceListFree(virConnectPtr conn, + pciDeviceList *list) +{ + int i; + + if (!list) + return; + + for (i = 0; i < list->count; i++) { + pciFreeDevice(conn, list->devs[i]); + list->devs[i] = NULL; + } + + list->count = 0; + VIR_FREE(list->devs); + VIR_FREE(list); +} + +int +pciDeviceListAdd(virConnectPtr conn, + pciDeviceList *list, + pciDevice *dev) +{ + if (pciDeviceListFind(list, dev)) { + pciReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Device %s is already in use"), dev->name); + return -1; + } + + if (VIR_REALLOC_N(list->devs, list->count+1) < 0) { + virReportOOMError(conn); + return -1; + } + + list->devs[list->count++] = dev; + + return 0; +} + +void +pciDeviceListDel(virConnectPtr conn ATTRIBUTE_UNUSED, + pciDeviceList *list, + pciDevice *dev) +{ + int i; + + for (i = 0; i < list->count; i++) { + if (list->devs[i]->domain != dev->domain || + list->devs[i]->bus != dev->bus || + list->devs[i]->slot != dev->slot || + list->devs[i]->function != dev->function) + continue; + + pciFreeDevice(conn, list->devs[i]); + + if (i != --list->count) + memmove(&list->devs[i], + &list->devs[i+1], + sizeof(*list->devs) * (list->count-i)); + + if (VIR_REALLOC_N(list->devs, list->count) < 0) { + ; /* not fatal */ + } + + break; + } +} + +pciDevice * +pciDeviceListFind(pciDeviceList *list, pciDevice *dev) +{ + int i; + + for (i = 0; i < list->count; i++) + if (list->devs[i]->domain == dev->domain && + list->devs[i]->bus == dev->bus && + list->devs[i]->slot == dev->slot && + list->devs[i]->function == dev->function) + return list->devs[i]; + return NULL; +} diff --git a/src/pci.h b/src/pci.h index 47882ef..75fbd51 100644 --- a/src/pci.h +++ b/src/pci.h @@ -27,6 +27,11 @@

typedef struct _pciDevice pciDevice;

+typedef struct { + unsigned count; + pciDevice **devs; +} pciDeviceList; + pciDevice *pciGetDevice (virConnectPtr conn, unsigned domain, unsigned bus, @@ -40,5 +45,20 @@ int pciReAttachDevice (virConnectPtr conn, pciDevice *dev); int pciResetDevice (virConnectPtr conn, pciDevice *dev); +void pciDeviceSetManaged(pciDevice *dev, + unsigned managed); +unsigned pciDeviceGetManaged(pciDevice *dev); + +pciDeviceList *pciDeviceListNew (virConnectPtr conn); +void pciDeviceListFree (virConnectPtr conn, + pciDeviceList *list); +int pciDeviceListAdd (virConnectPtr conn, + pciDeviceList *list, + pciDevice *dev); +void pciDeviceListDel (virConnectPtr conn, + pciDeviceList *list, + pciDevice *dev); +pciDevice * pciDeviceListFind (pciDeviceList *list, + pciDevice *dev);

#endif /* __VIR_PCI_H__ */ diff --git a/src/qemu_driver.c b/src/qemu_driver.c index a9da387..f2b807b 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1329,48 +1329,16 @@ static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) { return -1; }

-static int qemuPrepareHostDevices(virConnectPtr conn, - virDomainDefPtr def) { +static pciDeviceList * +qemuGetPciHostDeviceList(virConnectPtr conn, + virDomainDefPtr def) +{ + pciDeviceList *list; int i;

- /* We have to use 2 loops here. *All* devices must - * be detached before we reset any of them, because - * in some cases you have to reset the whole PCI, - * which impacts all devices on it - */ - - for (i = 0 ; i < def->nhostdevs ; i++) { - virDomainHostdevDefPtr hostdev = def->hostdevs[i]; - - if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) - continue; - if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) - continue; - - if (hostdev->managed) { - pciDevice *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) - goto error; - - if (pciDettachDevice(conn, dev) < 0) { - pciFreeDevice(conn, dev); - goto error; - } - - pciFreeDevice(conn, dev); - } /* else { - XXX validate that non-managed device isn't in use, eg - by checking that device is either un-bound, or bound - to pci-stub.ko - } */ - } + if (!(list = pciDeviceListNew(conn))) + return NULL;

- /* Now that all the PCI hostdevs have be dettached, we can safely - * reset them */ for (i = 0 ; i < def->nhostdevs ; i++) { virDomainHostdevDefPtr hostdev = def->hostdevs[i]; pciDevice *dev; @@ -1385,95 +1353,102 @@ static int qemuPrepareHostDevices(virConnectPtr conn, hostdev->source.subsys.u.pci.bus, hostdev->source.subsys.u.pci.slot, hostdev->source.subsys.u.pci.function); - if (!dev) - goto error; + if (!dev) { + pciDeviceListFree(conn, list); + return NULL; + }

- if (pciResetDevice(conn, dev) < 0) { + if (pciDeviceListAdd(conn, list, dev) < 0) { pciFreeDevice(conn, dev); - goto error; + pciDeviceListFree(conn, list); + return NULL; }

- pciFreeDevice(conn, dev); + pciDeviceSetManaged(dev, hostdev->managed); }

+ return list; +} + +static int +qemuPrepareHostDevices(virConnectPtr conn, virDomainDefPtr def) +{ + pciDeviceList *pcidevs; + int i; + + if (!def->nhostdevs) + return 0; + + if (!(pcidevs = qemuGetPciHostDeviceList(conn, def))) + return -1; + + /* We have to use 2 loops here. *All* devices must + * be detached before we reset any of them, because + * in some cases you have to reset the whole PCI, + * which impacts all devices on it + */ + + /* XXX validate that non-managed device isn't in use, eg + * by checking that device is either un-bound, or bound + * to pci-stub.ko + */ + + for (i = 0; i < pcidevs->count; i++) + if (pciDeviceGetManaged(pcidevs->devs[i]) && + pciDettachDevice(conn, pcidevs->devs[i]) < 0) + goto error; + + /* Now that all the PCI hostdevs have be dettached, we can safely + * reset them */ + for (i = 0; i < pcidevs->count; i++) + if (pciResetDevice(conn, pcidevs->devs[i]) < 0) + goto error; + + pciDeviceListFree(conn, pcidevs); return 0;

error: + pciDeviceListFree(conn, pcidevs); return -1; }

static void qemuDomainReAttachHostDevices(virConnectPtr conn, virDomainDefPtr def) { + pciDeviceList *pcidevs; 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); - } + if (!def->nhostdevs) + return;

- pciFreeDevice(conn, dev); + if (!(pcidevs = qemuGetPciHostDeviceList(conn, def))) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to allocate pciDeviceList: %s\n"), + err ? err->message : ""); + virResetError(err); + return; }

- 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; + /* Again 2 loops; reset all the devices before re-attach */

- 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) { + for (i = 0; i < pcidevs->count; i++) + if (pciResetDevice(conn, pcidevs->devs[i]) < 0) { virErrorPtr err = virGetLastError(); - VIR_ERROR(_("Failed to allocate pciDevice: %s\n"), + VIR_ERROR(_("Failed to reset PCI device: %s\n"), err ? err->message : ""); virResetError(err); - continue; }

- if (pciReAttachDevice(conn, dev) < 0) { + for (i = 0; i < pcidevs->count; i++) + if (pciDeviceGetManaged(pcidevs->devs[i]) && + pciReAttachDevice(conn, pcidevs->devs[i]) < 0) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to re-attach PCI device: %s\n"), err ? err->message : ""); virResetError(err); }

- pciFreeDevice(conn, dev); - } + pciDeviceListFree(conn, pcidevs); }

static const char *const defaultDeviceACL[] = { --

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 Mon, Aug 17, 2009 at 03:10:21PM +0100, Mark McLoughlin wrote:

As we start/shutdown guests, or hotplug/hot-unplug devices, we can add or delete devices as appropriate from a list of active devices.

Then, in pciReset(), we can use this to determine whether its safe to reset a device as a side effect of resetting another device.

* src/qemu_conf.h: add activePciHostdevs to qemud_driver

* src/qemu_driver.c: maintain the activePciHostdevs list, and pass it to pciResetDevice()

* src/pci.[ch]: pass the activeDevs list to pciResetDevice() and use it to determine whether a Secondary Bus Reset is safe --- src/pci.c | 102 ++++++++++++++++++++++++++------------------ src/pci.h | 3 +- src/qemu_conf.h | 3 + src/qemu_driver.c | 123 ++++++++++++++++++++++++++++++++++++++++++---------- src/xen_unified.c | 2 +- 5 files changed, 165 insertions(+), 68 deletions(-)

diff --git a/src/pci.c b/src/pci.c index a37eaf7..96e5d6d 100644 --- a/src/pci.c +++ b/src/pci.c @@ -226,7 +226,7 @@ pciWrite32(pciDevice *dev, unsigned pos, uint32_t val) pciWrite(dev, pos, &buf[0], sizeof(buf)); }

-typedef int (*pciIterPredicate)(pciDevice *, pciDevice *); +typedef int (*pciIterPredicate)(pciDevice *, pciDevice *, void *);

/* Iterate over available PCI devices calling @predicate * to compare each one to @dev. @@ -237,7 +237,8 @@ static int pciIterDevices(virConnectPtr conn, pciIterPredicate predicate, pciDevice *dev, - pciDevice **matched) + pciDevice **matched, + void *data) { DIR *dir; struct dirent *entry; @@ -255,7 +256,7 @@ pciIterDevices(virConnectPtr conn,

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

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

- try = pciGetDevice(conn, domain, bus, slot, function); - if (!try) { + check = pciGetDevice(conn, domain, bus, slot, function); + if (!check) { 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(dev, check, data)) { + VIR_DEBUG("%s %s: iter matched on %s", dev->id, dev->name, check->name); + *matched = check; break; } - pciFreeDevice(conn, try); + pciFreeDevice(conn, check); } closedir(dir); return ret; @@ -380,63 +381,70 @@ pciDetectPowerManagementReset(pciDevice *dev) return 0; }

-/* Any devices other than the one supplied on the same domain/bus ? */ +/* Any active devices other than the one supplied on the same domain/bus ? */ static int -pciSharesBus(pciDevice *a, pciDevice *b) +pciSharesBusWithActive(pciDevice *dev, pciDevice *check, void *data) { - return - a->domain == b->domain && - a->bus == b->bus && - (a->slot != b->slot || - a->function != b->function); -} + pciDeviceList *activeDevs = data;

-static int -pciBusContainsOtherDevices(virConnectPtr conn, pciDevice *dev) -{ - pciDevice *matched = NULL; - if (pciIterDevices(conn, pciSharesBus, dev, &matched) < 0) - return 1; - if (!matched) + if (dev->domain != check->domain || + dev->bus != check->bus || + (check->slot == check->slot && + check->function == check->function)) + return 0; + + if (activeDevs && !pciDeviceListFind(activeDevs, check)) return 0; - pciFreeDevice(conn, matched); + return 1; }

-/* Is @a the parent of @b ? */ +static pciDevice * +pciBusContainsActiveDevices(virConnectPtr conn, + pciDevice *dev, + pciDeviceList *activeDevs) +{ + pciDevice *active = NULL; + if (pciIterDevices(conn, pciSharesBusWithActive, + dev, &active, activeDevs) < 0) + return NULL; + return active; +} + +/* Is @check the parent of @dev ? */ static int -pciIsParent(pciDevice *a, pciDevice *b) +pciIsParent(pciDevice *dev, pciDevice *check, void *data ATTRIBUTE_UNUSED) { uint16_t device_class; uint8_t header_type, secondary, subordinate;

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

/* Is it a bridge? */ - device_class = pciRead16(a, PCI_CLASS_DEVICE); + device_class = pciRead16(check, 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, 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, PCI_SECONDARY_BUS); + subordinate = pciRead8(check, 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->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, pciIsParent, dev, &parent, NULL); return parent; }

@@ -444,9 +452,11 @@ pciGetParentDevice(virConnectPtr conn, pciDevice *dev) * devices behind a bus. */ static int -pciTrySecondaryBusReset(virConnectPtr conn, pciDevice *dev) +pciTrySecondaryBusReset(virConnectPtr conn, + pciDevice *dev, + pciDeviceList *activeDevs) { - pciDevice *parent; + pciDevice *parent, *conflict; uint8_t config_space[PCI_CONF_LEN]; uint16_t ctl; int ret = -1; @@ -456,10 +466,10 @@ 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 = pciBusContainsActiveDevices(conn, dev, activeDevs))) { pciReportError(conn, VIR_ERR_NO_SUPPORT, - _("Other devices on bus with %s, not doing bus reset"), - dev->name); + _("Active %s devices on bus with %s, not doing bus reset"), + conflict->name, dev->name); return -1; }

@@ -573,10 +583,18 @@ pciInitDevice(virConnectPtr conn, pciDevice *dev) }

int -pciResetDevice(virConnectPtr conn, pciDevice *dev) +pciResetDevice(virConnectPtr conn, + pciDevice *dev, + pciDeviceList *activeDevs) { int ret = -1;

+ if (activeDevs && pciDeviceListFind(activeDevs, dev)) { + pciReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Not resetting active device %s"), dev->name); + return -1; + } + if (!dev->initted && pciInitDevice(conn, dev) < 0) return -1;

@@ -595,7 +613,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, dev, activeDevs);

if (ret < 0) { virErrorPtr err = virGetLastError(); diff --git a/src/pci.h b/src/pci.h index 75fbd51..685b0af 100644 --- a/src/pci.h +++ b/src/pci.h @@ -44,7 +44,8 @@ int pciDettachDevice (virConnectPtr conn, int pciReAttachDevice (virConnectPtr conn, pciDevice *dev); int pciResetDevice (virConnectPtr conn, - pciDevice *dev); + pciDevice *dev, + pciDeviceList *activeDevs); void pciDeviceSetManaged(pciDevice *dev, unsigned managed); unsigned pciDeviceGetManaged(pciDevice *dev); diff --git a/src/qemu_conf.h b/src/qemu_conf.h index 8f9e1c1..a126dac 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -35,6 +35,7 @@ #include "threads.h" #include "security.h" #include "cgroup.h" +#include "pci.h"

#define qemudDebug(fmt, ...) do {} while(0)

@@ -111,6 +112,8 @@ struct qemud_driver { virSecurityDriverPtr securityDriver;

char *saveImageFormat; + + pciDeviceList *activePciHostdevs; };

diff --git a/src/qemu_driver.c b/src/qemu_driver.c index f2b807b..70d3d55 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -128,6 +128,9 @@ static int qemudDomainSetMemoryBalloon(virConnectPtr conn, static int qemudDetectVcpuPIDs(virConnectPtr conn, virDomainObjPtr vm);

+static int qemuUpdateActivePciHostdevs(struct qemud_driver *driver, + virDomainDefPtr def); + static struct qemud_driver *qemu_driver = NULL;

static int qemuCgroupControllerActive(struct qemud_driver *driver, @@ -320,6 +323,10 @@ qemuReconnectDomain(struct qemud_driver *driver, goto error; }

+ if (qemuUpdateActivePciHostdevs(driver, obj->def) < 0) { + goto error; + } + if (obj->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC && driver->securityDriver && driver->securityDriver->domainReserveSecurityLabel && @@ -524,6 +531,9 @@ qemudStartup(int privileged) { if ((qemu_driver->caps = qemudCapsInit(NULL)) == NULL) goto out_of_memory;

+ if ((qemu_driver->activePciHostdevs = pciDeviceListNew(NULL)) == NULL) + goto error; + if (qemudLoadDriverConfig(qemu_driver, driverConf) < 0) { goto error; } @@ -648,6 +658,7 @@ qemudShutdown(void) { return -1;

qemuDriverLock(qemu_driver); + pciDeviceListFree(NULL, qemu_driver->activePciHostdevs); virCapabilitiesFree(qemu_driver->caps);

virDomainObjListFree(&qemu_driver->domains); @@ -1371,7 +1382,38 @@ qemuGetPciHostDeviceList(virConnectPtr conn, }

static int -qemuPrepareHostDevices(virConnectPtr conn, virDomainDefPtr def) +qemuUpdateActivePciHostdevs(struct qemud_driver *driver, + virDomainDefPtr def) +{ + pciDeviceList *pcidevs; + int i, ret; + + if (!def->nhostdevs) + return 0; + + if (!(pcidevs = qemuGetPciHostDeviceList(NULL, def))) + return -1; + + ret = 0; + + for (i = 0; i < pcidevs->count; i++) { + if (pciDeviceListAdd(NULL, + driver->activePciHostdevs, + pcidevs->devs[i]) < 0) { + ret = -1; + break; + } + pcidevs->devs[i] = NULL; + } + + pciDeviceListFree(NULL, pcidevs); + return ret; +} + +static int +qemuPrepareHostDevices(virConnectPtr conn, + struct qemud_driver *driver, + virDomainDefPtr def) { pciDeviceList *pcidevs; int i; @@ -1382,10 +1424,11 @@ qemuPrepareHostDevices(virConnectPtr conn, virDomainDefPtr def) if (!(pcidevs = qemuGetPciHostDeviceList(conn, def))) return -1;

- /* We have to use 2 loops here. *All* devices must + /* We have to use 3 loops here. *All* devices must * be detached before we reset any of them, because * in some cases you have to reset the whole PCI, - * which impacts all devices on it + * which impacts all devices on it. Also, all devices + * must be reset before being marked as active. */

/* XXX validate that non-managed device isn't in use, eg @@ -1401,9 +1444,19 @@ qemuPrepareHostDevices(virConnectPtr conn, virDomainDefPtr def) /* Now that all the PCI hostdevs have be dettached, we can safely * reset them */ for (i = 0; i < pcidevs->count; i++) - if (pciResetDevice(conn, pcidevs->devs[i]) < 0) + if (pciResetDevice(conn, pcidevs->devs[i], + driver->activePciHostdevs) < 0) goto error;

+ /* Now mark all the devices as active */ + for (i = 0; i < pcidevs->count; i++) { + if (pciDeviceListAdd(conn, + driver->activePciHostdevs, + pcidevs->devs[i]) < 0) + goto error; + pcidevs->devs[i] = NULL; + } + pciDeviceListFree(conn, pcidevs); return 0;

@@ -1413,7 +1466,9 @@ error: }

static void -qemuDomainReAttachHostDevices(virConnectPtr conn, virDomainDefPtr def) +qemuDomainReAttachHostDevices(virConnectPtr conn, + struct qemud_driver *driver, + virDomainDefPtr def) { pciDeviceList *pcidevs; int i; @@ -1429,10 +1484,15 @@ qemuDomainReAttachHostDevices(virConnectPtr conn, virDomainDefPtr def) return; }

- /* Again 2 loops; reset all the devices before re-attach */ + /* Again 3 loops; mark all devices as inactive before reset + * them and reset all the devices before re-attach */

for (i = 0; i < pcidevs->count; i++) - if (pciResetDevice(conn, pcidevs->devs[i]) < 0) { + pciDeviceListDel(conn, driver->activePciHostdevs, pcidevs->devs[i]); + + for (i = 0; i < pcidevs->count; i++) + if (pciResetDevice(conn, pcidevs->devs[i], + driver->activePciHostdevs) < 0) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to reset PCI device: %s\n"), err ? err->message : ""); @@ -1976,7 +2036,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, if (qemuSetupCgroup(conn, driver, vm) < 0) goto cleanup;

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

if (VIR_ALLOC(vm->monitor_chr) < 0) { @@ -2158,7 +2218,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, driver, vm->def);

retry: if ((ret = qemuRemoveCgroup(conn, driver, vm)) < 0) { @@ -5279,6 +5339,7 @@ cleanup: }

static int qemudDomainAttachHostPciDevice(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev) { @@ -5301,42 +5362,42 @@ static int qemudDomainAttachHostPciDevice(virConnectPtr conn, return -1;

if ((hostdev->managed && pciDettachDevice(conn, pci) < 0) || - pciResetDevice(conn, pci) < 0) { + pciResetDevice(conn, pci, driver->activePciHostdevs) < 0) { + pciFreeDevice(conn, pci); + return -1; + } + + if (pciDeviceListAdd(conn, driver->activePciHostdevs, pci) < 0) { pciFreeDevice(conn, pci); return -1; }

- pciFreeDevice(conn, pci); + cmd = reply = NULL;

if (virAsprintf(&cmd, "pci_add pci_addr=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; + goto error; }

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; + goto error; }

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; + goto error; }

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; + goto error; }

hostdev->source.subsys.u.pci.guest_addr.domain = domain; @@ -5349,6 +5410,14 @@ static int qemudDomainAttachHostPciDevice(virConnectPtr conn, VIR_FREE(cmd);

return 0; + +error: + pciDeviceListDel(conn, driver->activePciHostdevs, pci); + + VIR_FREE(reply); + VIR_FREE(cmd); + + return -1; }

static int qemudDomainAttachHostUsbDevice(virConnectPtr conn, @@ -5422,7 +5491,7 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn,

switch (hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: - return qemudDomainAttachHostPciDevice(conn, vm, dev); + return qemudDomainAttachHostPciDevice(conn, driver, vm, dev); case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: return qemudDomainAttachHostUsbDevice(conn, vm, dev); default: @@ -5749,6 +5818,7 @@ cleanup: }

static int qemudDomainDetachHostPciDevice(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev) { @@ -5832,7 +5902,8 @@ static int qemudDomainDetachHostPciDevice(virConnectPtr conn, if (!pci) ret = -1; else { - if (pciResetDevice(conn, pci) < 0) + pciDeviceListDel(conn, driver->activePciHostdevs, pci); + if (pciResetDevice(conn, pci, driver->activePciHostdevs) < 0) ret = -1; if (detach->managed && pciReAttachDevice(conn, pci) < 0) ret = -1; @@ -5868,7 +5939,7 @@ static int qemudDomainDetachHostDevice(virConnectPtr conn,

switch (hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: - ret = qemudDomainDetachHostPciDevice(conn, vm, dev); + ret = qemudDomainDetachHostPciDevice(conn, driver, vm, dev); break; default: qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT, @@ -7092,6 +7163,7 @@ out: static int qemudNodeDeviceReset (virNodeDevicePtr dev) { + struct qemud_driver *driver = dev->conn->privateData; pciDevice *pci; unsigned domain, bus, slot, function; int ret = -1; @@ -7103,11 +7175,14 @@ qemudNodeDeviceReset (virNodeDevicePtr dev) if (!pci) return -1;

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

ret = 0; out: + qemuDriverUnlock(driver); pciFreeDevice(dev->conn, pci); return ret; } diff --git a/src/xen_unified.c b/src/xen_unified.c index f2ffc25..dfa9ca5 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, 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 :|