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

Re: [libvirt] [PATCH v2] qemu: Do not reattach PCI device used by other domain when shutdown



于 2011年10月13日 12:05, Osier Yang 写道:
> When failing on starting a domain, it tries to reattach all the PCI
> devices defined in the domain conf, regardless of whether the devices
> are still used by other domain. This will cause the devices to be deleted
> from the list qemu_driver->activePciHostdevs, thus the devices will be
> thought as usable even if it's not true. And following commands
> nodedev-{reattach,reset} will be successful.
>
> How to reproduce:
>   1) Define two domains with same PCI device defined in the confs.
>   2) # virsh start domain1
>   3) # virsh start domain2
>   4) # virsh nodedev-reattach $pci_device
>
> You will see the device will be reattached to host successfully.
> As pciDeviceReattach just check if the device is still used by
> other domain via checking if the device is in list driver->activePciHostdevs,
> however, the device is deleted from the list by step 2).
>
> This patch is to prohibit the bug by:
>   1) Prohibit a domain starting or device attachment right at
>      preparation period (qemuPrepareHostdevPCIDevices) if the
>      device is in list driver->activePciHostdevs, which means
>      it's used by other domain.
>
>   2) Introduces a new field for struct _pciDevice, (const char *used_by),
>      it will be set as the domain name at preparation period,
>      (qemuPrepareHostdevPCIDevices). Thus we can prohibit deleting
>      the device from driver->activePciHostdevs if it's still used by
>      other domain when stopping the domain process.
>
> * src/pci.h (define two internal functions, pciDeviceSetUsedBy and
>     pciDevceGetUsedBy)
> * src/pci.c (new field "const char *used_by" for struct _pciDevice,
>     implementations for the two new functions)
> * src/libvirt_private.syms (Add the two new internal functions)
> * src/qemu_hostdev.h (Modify the definition of functions
>     qemuPrepareHostdevPCIDevices, and qemuDomainReAttachHostdevDevices)
> * src/qemu_hostdev.c (Prohibit preparation and don't delete the
>     device from activePciHostdevs list if it's still used by other domain)
> * src/qemu_hotplug.c (Update function usage, as the definitions are
>     changed)
>
> v1 - v2:
>    * char * used_by => const char *used_by, and no strdup()
>    * Other nits pointed out by Eric.
> ---
>  src/libvirt_private.syms |    2 ++
>  src/qemu/qemu_hostdev.c  |   43 ++++++++++++++++++++++++++++++++++++++-----
>  src/qemu/qemu_hostdev.h  |    2 ++
>  src/qemu/qemu_hotplug.c  |    4 ++--
>  src/util/pci.c           |   15 +++++++++++++++
>  src/util/pci.h           |    3 +++
>  6 files changed, 62 insertions(+), 7 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 4f96518..4d50d86 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -880,6 +880,7 @@ virNWFilterHashTableRemoveEntry;
>  pciDettachDevice;
>  pciDeviceFileIterate;
>  pciDeviceGetManaged;
> +pciDeviceGetUsedBy;
>  pciDeviceIsAssignable;
>  pciDeviceIsVirtualFunction;
>  pciDeviceListAdd;
> @@ -892,6 +893,7 @@ pciDeviceListSteal;
>  pciDeviceNetName;
>  pciDeviceReAttachInit;
>  pciDeviceSetManaged;
> +pciDeviceSetUsedBy;
>  pciFreeDevice;
>  pciGetDevice;
>  pciGetPhysicalFunction;
> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> index 6f77717..f354ea8 100644
> --- a/src/qemu/qemu_hostdev.c
> +++ b/src/qemu/qemu_hostdev.c
> @@ -101,6 +101,7 @@ cleanup:
>  
>  
>  int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver,
> +                                 const char *name,
>                                   virDomainHostdevDefPtr *hostdevs,
>                                   int nhostdevs)
>  {
> @@ -111,7 +112,7 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver,
>      if (!(pcidevs = qemuGetPciHostDeviceList(hostdevs, nhostdevs)))
>          return -1;
>  
> -    /* We have to use 4 loops here. *All* devices must
> +    /* We have to use 6 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. Also, all devices
> @@ -125,9 +126,16 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver,
>  
>      for (i = 0; i < pciDeviceListCount(pcidevs); i++) {
>          pciDevice *dev = pciDeviceListGet(pcidevs, i);
> -        if (!pciDeviceIsAssignable(dev, !driver->relaxedACS))
> -            goto reattachdevs;
> +        /* The device is in use by other active domain if
> +         * the dev is in list driver->activePciHostdevs.
> +         */
> +        if (!pciDeviceIsAssignable(dev, !driver->relaxedACS) ||
> +            pciDeviceListFind(driver->activePciHostdevs, dev))
> +            goto cleanup;
> +    }
>  
> +    for (i = 0; i < pciDeviceListCount(pcidevs); i++) {
> +        pciDevice *dev = pciDeviceListGet(pcidevs, i);
>          if (pciDeviceGetManaged(dev) &&
>              pciDettachDevice(dev, driver->activePciHostdevs) < 0)
>              goto reattachdevs;
> @@ -150,6 +158,18 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver,
>          }
>      }
>  
> +    /* Now set the used_by_domain of the device in driver->activePciHostdevs
> +     * as domain name.
> +     */
> +    for (i = 0; i < pciDeviceListCount(pcidevs); i++) {
> +        pciDevice *dev, *activeDev;
> +
> +        dev = pciDeviceListGet(pcidevs, i);
> +        activeDev = pciDeviceListFind(driver->activePciHostdevs, dev);
> +
> +        pciDeviceSetUsedBy(activeDev, name);
> +    }
> +
>      /* Now steal all the devices from pcidevs */
>      while (pciDeviceListCount(pcidevs) > 0) {
>          pciDevice *dev = pciDeviceListGet(pcidevs, 0);
> @@ -183,7 +203,7 @@ static int
>  qemuPrepareHostPCIDevices(struct qemud_driver *driver,
>                            virDomainDefPtr def)
>  {
> -    return qemuPrepareHostdevPCIDevices(driver, def->hostdevs, def->nhostdevs);
> +    return qemuPrepareHostdevPCIDevices(driver, def->name, def->hostdevs, def->nhostdevs);
>  }
>  
>  
> @@ -258,6 +278,7 @@ void qemuReattachPciDevice(pciDevice *dev, struct qemud_driver *driver)
>  
>  
>  void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver,
> +                                      const char *name,
>                                        virDomainHostdevDefPtr *hostdevs,
>                                        int nhostdevs)
>  {
> @@ -277,6 +298,18 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver,
>  
>      for (i = 0; i < pciDeviceListCount(pcidevs); i++) {
>          pciDevice *dev = pciDeviceListGet(pcidevs, i);
> +        pciDevice *activeDev = NULL;
> +        const char *used_by = NULL;
> +
> +        /* Never delete the dev from list driver->activePciHostdevs
> +         *  if it's used by other domain.
> +         */
> +        activeDev = pciDeviceListFind(driver->activePciHostdevs, dev);
> +        if (activeDev &&
> +            (used_by = pciDeviceGetUsedBy(activeDev)) &&
> +            STRNEQ(name, used_by))

used_by is kept as it might be NULL.

> +            continue;
> +
>          pciDeviceListDel(driver->activePciHostdevs, dev);
>      }
>  
> @@ -305,5 +338,5 @@ void qemuDomainReAttachHostDevices(struct qemud_driver *driver,
>      if (!def->nhostdevs)
>          return;
>  
> -    qemuDomainReAttachHostdevDevices(driver, def->hostdevs, def->nhostdevs);
> +    qemuDomainReAttachHostdevDevices(driver, def->name, def->hostdevs, def->nhostdevs);
>  }
> diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h
> index 1f3d1bc..07d7de2 100644
> --- a/src/qemu/qemu_hostdev.h
> +++ b/src/qemu/qemu_hostdev.h
> @@ -30,12 +30,14 @@
>  int qemuUpdateActivePciHostdevs(struct qemud_driver *driver,
>                                  virDomainDefPtr def);
>  int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver,
> +                                 const char *name,
>                                   virDomainHostdevDefPtr *hostdevs,
>                                   int nhostdevs);
>  int qemuPrepareHostDevices(struct qemud_driver *driver,
>                             virDomainDefPtr def);
>  void qemuReattachPciDevice(pciDevice *dev, struct qemud_driver *driver);
>  void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver,
> +                                      const char *name,
>                                        virDomainHostdevDefPtr *hostdevs,
>                                        int nhostdevs);
>  void qemuDomainReAttachHostDevices(struct qemud_driver *driver,
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 4c1705d..bfa524b 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -893,7 +893,7 @@ int qemuDomainAttachHostPciDevice(struct qemud_driver *driver,
>          return -1;
>      }
>  
> -    if (qemuPrepareHostdevPCIDevices(driver, &hostdev, 1) < 0)
> +    if (qemuPrepareHostdevPCIDevices(driver, vm->def->name, &hostdev, 1) < 0)
>          return -1;
>  
>      if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> @@ -959,7 +959,7 @@ error:
>                                          hostdev->info.addr.pci.slot) < 0)
>          VIR_WARN("Unable to release PCI address on host device");
>  
> -    qemuDomainReAttachHostdevDevices(driver, &hostdev, 1);
> +    qemuDomainReAttachHostdevDevices(driver, vm->def->name, &hostdev, 1);
>  
>      VIR_FREE(devstr);
>      VIR_FREE(configfd_name);
> diff --git a/src/util/pci.c b/src/util/pci.c
> index 8d8e157..888784a 100644
> --- a/src/util/pci.c
> +++ b/src/util/pci.c
> @@ -62,6 +62,7 @@ struct _pciDevice {
>      char          name[PCI_ADDR_LEN]; /* domain:bus:slot.function */
>      char          id[PCI_ID_LEN];     /* product vendor */
>      char          *path;
> +    const char    *used_by;           /* The domain which uses the device */
>      int           fd;
>  
>      unsigned      initted;
> @@ -1312,6 +1313,7 @@ pciGetDevice(unsigned domain,
>      dev->bus      = bus;
>      dev->slot     = slot;
>      dev->function = function;
> +    dev->used_by  = NULL;
>  
>      if (snprintf(dev->name, sizeof(dev->name), "%.4x:%.2x:%.2x.%.1x",
>                   dev->domain, dev->bus, dev->slot,
> @@ -1374,6 +1376,7 @@ pciFreeDevice(pciDevice *dev)
>      VIR_DEBUG("%s %s: freeing", dev->id, dev->name);
>      pciCloseConfig(dev);
>      VIR_FREE(dev->path);
> +    dev->used_by = NULL;
>      VIR_FREE(dev);
>  }
>  
> @@ -1387,6 +1390,18 @@ unsigned pciDeviceGetManaged(pciDevice *dev)
>      return dev->managed;
>  }
>  
> +void
> +pciDeviceSetUsedBy(pciDevice *dev, const char *name)
> +{
> +    dev->used_by = name;
> +}
> +
> +const char *
> +pciDeviceGetUsedBy(pciDevice *dev)
> +{
> +    return dev->used_by;
> +}
> +
>  void pciDeviceReAttachInit(pciDevice *pci)
>  {
>      pci->unbind_from_stub = 1;
> diff --git a/src/util/pci.h b/src/util/pci.h
> index a1600fe..640c6af 100644
> --- a/src/util/pci.h
> +++ b/src/util/pci.h
> @@ -47,6 +47,9 @@ int        pciResetDevice    (pciDevice     *dev,
>  void      pciDeviceSetManaged(pciDevice     *dev,
>                                unsigned       managed);
>  unsigned  pciDeviceGetManaged(pciDevice     *dev);
> +void      pciDeviceSetUsedBy(pciDevice     *dev,
> +                             const char *used_by);
> +const char *pciDeviceGetUsedBy(pciDevice   *dev);
>  void      pciDeviceReAttachInit(pciDevice   *dev);
>  
>  pciDeviceList *pciDeviceListNew  (void);


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