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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list