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

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



Anybody could help review this? Thanks

Osier
于 2011年09月27日 14:53, 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 are 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, (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 "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)
> ---
>  src/libvirt_private.syms |    2 ++
>  src/qemu/qemu_hostdev.c  |   31 ++++++++++++++++++++++++++++---
>  src/qemu/qemu_hostdev.h  |    2 ++
>  src/qemu/qemu_hotplug.c  |    4 ++--
>  src/util/pci.c           |   22 ++++++++++++++++++++++
>  src/util/pci.h           |    3 +++
>  6 files changed, 59 insertions(+), 5 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 8235ea1..a5c5e6c 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -872,6 +872,7 @@ virNWFilterHashTableRemoveEntry;
>  pciDettachDevice;
>  pciDeviceFileIterate;
>  pciDeviceGetManaged;
> +pciDeviceGetUsedBy;
>  pciDeviceIsAssignable;
>  pciDeviceIsVirtualFunction;
>  pciDeviceListAdd;
> @@ -884,6 +885,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..ef9e3b7 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)
>  {
> @@ -126,7 +127,10 @@ 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;
> +            goto cleanup;
> +
> +        if (pciDeviceListFind(driver->activePciHostdevs, dev))
> +            goto cleanup;
>  
>          if (pciDeviceGetManaged(dev) &&
>              pciDettachDevice(dev, driver->activePciHostdevs) < 0)
> @@ -156,6 +160,14 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver,
>          pciDeviceListSteal(pcidevs, dev);
>      }
>  
> +    /* Now set the used_by_domain of the device in driver->activePciHostdevs
> +     * as domain name.
> +     */
> +    for (i = 0; i < pciDeviceListCount(driver->activePciHostdevs); i++) {
> +        pciDevice * dev = pciDeviceListGet(driver->activePciHostdevs, i);
> +        pciDeviceSetUsedBy(dev, name);
> +    }
> +
>      ret = 0;
>      goto cleanup;
>  
> @@ -183,7 +195,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,11 +270,13 @@ void qemuReattachPciDevice(pciDevice *dev, struct qemud_driver *driver)
>  
>  
>  void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver,
> +                                      const char *name,
>                                        virDomainHostdevDefPtr *hostdevs,
>                                        int nhostdevs)
>  {
>      pciDeviceList *pcidevs;
>      int i;
> +    const char *used_by = NULL;
>  
>      if (!(pcidevs = qemuGetPciHostDeviceList(hostdevs, nhostdevs))) {
>          virErrorPtr err = virGetLastError();
> @@ -277,6 +291,17 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver,
>  
>      for (i = 0; i < pciDeviceListCount(pcidevs); i++) {
>          pciDevice *dev = pciDeviceListGet(pcidevs, i);
> +        pciDevice *activeDev = 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(used_by, name))
> +            continue;
> +
>          pciDeviceListDel(driver->activePciHostdevs, dev);
>      }
>  
> @@ -305,5 +330,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 6cfe392..dc920e7 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -859,7 +859,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)) {
> @@ -925,7 +925,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..38548c7 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;
> +    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);
> +    VIR_FREE(dev->used_by);
>      VIR_FREE(dev);
>  }
>  
> @@ -1387,6 +1390,25 @@ unsigned pciDeviceGetManaged(pciDevice *dev)
>      return dev->managed;
>  }
>  
> +int
> +pciDeviceSetUsedBy(pciDevice *dev, const char *name)
> +{
> +    dev->used_by = strdup(name);
> +
> +    if (!dev->used_by) {
> +        virReportOOMError();
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +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..c9d8227 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);
> +int       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]