[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 7/8] Allow pciResetDevice() to reset multiple devices



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]