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