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

Re: [libvirt] [PATCH 7/8] Simplify PCI hostdev prepare/re-attach using a pciDeviceList type



On Mon, Aug 17, 2009 at 03:10:20PM +0100, Mark McLoughlin wrote:
> The qemuPrepareHostDevices() and qemuDomainReAttachHostDevices()
> functions are clutter with a bunch of calls to pciGetDevice() and
> pciFreeDevice() obscuring the basic logic.
> 
> Add a pciDeviceList type and add a qemuGetPciHostDeviceList() function
> to build a list from a domain definition. Use this in prepare/re-attach
> fto simplify things and eliminate the multiple pciGetDevice calls.
> 
> This is especially useful because in the next patch we need to iterate
> the hostdevs list a third time and we also need a list type for keeping
> track of active devices.
> 
> * src/pci.[ch]: add pciDeviceList type and also a per-device 'managed'
>   property
> 
> * src/libvirt_private.syms: export the new functions
> 
> * src/qemu_driver.c: add qemuGetPciHostDeviceList() and re-write
>   qemuPrepareHostDevices() and qemuDomainReAttachHostDevices() to use it
> ---
>  src/libvirt_private.syms |    7 ++-
>  src/pci.c                |  109 ++++++++++++++++++++++++++++++
>  src/pci.h                |   20 ++++++
>  src/qemu_driver.c        |  167 +++++++++++++++++++--------------------------
>  4 files changed, 206 insertions(+), 97 deletions(-)


Yeah that's nicer readability in the QEMU driver.

> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 642c2bc..2bf4e15 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -285,7 +285,12 @@ pciFreeDevice;
>  pciDettachDevice;
>  pciReAttachDevice;
>  pciResetDevice;
> -
> +pciDeviceSetManaged;
> +pciDeviceGetManaged;
> +pciDeviceListNew;
> +pciDeviceListFree;
> +pciDeviceListAdd;
> +pciDeviceListDel;
>  
>  # qparams.h
>  qparam_get_query;
> diff --git a/src/pci.c b/src/pci.c
> index 74f7ef0..a37eaf7 100644
> --- a/src/pci.c
> +++ b/src/pci.c
> @@ -63,6 +63,7 @@ struct _pciDevice {
>      unsigned      pci_pm_cap_pos;
>      unsigned      has_flr : 1;
>      unsigned      has_pm_reset : 1;
> +    unsigned      managed : 1;
>  };
>  
>  /* For virReportOOMError()  and virReportSystemError() */
> @@ -890,8 +891,116 @@ pciGetDevice(virConnectPtr conn,
>  void
>  pciFreeDevice(virConnectPtr conn ATTRIBUTE_UNUSED, pciDevice *dev)
>  {
> +    if (!dev)
> +        return;
>      VIR_DEBUG("%s %s: freeing", dev->id, dev->name);
>      if (dev->fd >= 0)
>          close(dev->fd);
>      VIR_FREE(dev);
>  }
> +
> +void pciDeviceSetManaged(pciDevice *dev, unsigned managed)
> +{
> +    dev->managed = !!managed;
> +}
> +
> +unsigned pciDeviceGetManaged(pciDevice *dev)
> +{
> +    return dev->managed;
> +}
> +
> +pciDeviceList *
> +pciDeviceListNew(virConnectPtr conn)
> +{
> +    pciDeviceList *list;
> +
> +    if (VIR_ALLOC(list) < 0) {
> +        virReportOOMError(conn);
> +        return NULL;
> +    }
> +
> +    return list;
> +}
> +
> +void
> +pciDeviceListFree(virConnectPtr conn,
> +                  pciDeviceList *list)
> +{
> +    int i;
> +
> +    if (!list)
> +        return;
> +
> +    for (i = 0; i < list->count; i++) {
> +        pciFreeDevice(conn, list->devs[i]);
> +        list->devs[i] = NULL;
> +    }
> +
> +    list->count = 0;
> +    VIR_FREE(list->devs);
> +    VIR_FREE(list);
> +}
> +
> +int
> +pciDeviceListAdd(virConnectPtr conn,
> +                 pciDeviceList *list,
> +                 pciDevice *dev)
> +{
> +    if (pciDeviceListFind(list, dev)) {
> +        pciReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                       _("Device %s is already in use"), dev->name);
> +        return -1;
> +    }
> +
> +    if (VIR_REALLOC_N(list->devs, list->count+1) < 0) {
> +        virReportOOMError(conn);
> +        return -1;
> +    }
> +
> +    list->devs[list->count++] = dev;
> +
> +    return 0;
> +}
> +
> +void
> +pciDeviceListDel(virConnectPtr conn ATTRIBUTE_UNUSED,
> +                 pciDeviceList *list,
> +                 pciDevice *dev)
> +{
> +    int i;
> +
> +    for (i = 0; i < list->count; i++) {
> +        if (list->devs[i]->domain   != dev->domain ||
> +            list->devs[i]->bus      != dev->bus    ||
> +            list->devs[i]->slot     != dev->slot   ||
> +            list->devs[i]->function != dev->function)
> +            continue;
> +
> +        pciFreeDevice(conn, list->devs[i]);
> +
> +        if (i != --list->count)
> +            memmove(&list->devs[i],
> +                    &list->devs[i+1],
> +                    sizeof(*list->devs) * (list->count-i));
> +
> +        if (VIR_REALLOC_N(list->devs, list->count) < 0) {
> +            ; /* not fatal */
> +        }
> +
> +        break;
> +    }
> +}
> +
> +pciDevice *
> +pciDeviceListFind(pciDeviceList *list, pciDevice *dev)
> +{
> +    int i;
> +
> +    for (i = 0; i < list->count; i++)
> +        if (list->devs[i]->domain   == dev->domain &&
> +            list->devs[i]->bus      == dev->bus    &&
> +            list->devs[i]->slot     == dev->slot   &&
> +            list->devs[i]->function == dev->function)
> +            return list->devs[i];
> +    return NULL;
> +}
> diff --git a/src/pci.h b/src/pci.h
> index 47882ef..75fbd51 100644
> --- a/src/pci.h
> +++ b/src/pci.h
> @@ -27,6 +27,11 @@
>  
>  typedef struct _pciDevice pciDevice;
>  
> +typedef struct {
> +    unsigned count;
> +    pciDevice **devs;
> +} pciDeviceList;
> +
>  pciDevice *pciGetDevice      (virConnectPtr  conn,
>                                unsigned       domain,
>                                unsigned       bus,
> @@ -40,5 +45,20 @@ int        pciReAttachDevice (virConnectPtr  conn,
>                                pciDevice     *dev);
>  int        pciResetDevice    (virConnectPtr  conn,
>                                pciDevice     *dev);
> +void      pciDeviceSetManaged(pciDevice     *dev,
> +                              unsigned       managed);
> +unsigned  pciDeviceGetManaged(pciDevice     *dev);
> +
> +pciDeviceList *pciDeviceListNew  (virConnectPtr conn);
> +void           pciDeviceListFree (virConnectPtr conn,
> +                                  pciDeviceList *list);
> +int            pciDeviceListAdd  (virConnectPtr conn,
> +                                  pciDeviceList *list,
> +                                  pciDevice *dev);
> +void           pciDeviceListDel  (virConnectPtr conn,
> +                                  pciDeviceList *list,
> +                                  pciDevice *dev);
> +pciDevice *    pciDeviceListFind (pciDeviceList *list,
> +                                  pciDevice *dev);
>  
>  #endif /* __VIR_PCI_H__ */
> diff --git a/src/qemu_driver.c b/src/qemu_driver.c
> index a9da387..f2b807b 100644
> --- a/src/qemu_driver.c
> +++ b/src/qemu_driver.c
> @@ -1329,48 +1329,16 @@ static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) {
>      return -1;
>  }
>  
> -static int qemuPrepareHostDevices(virConnectPtr conn,
> -                                  virDomainDefPtr def) {
> +static pciDeviceList *
> +qemuGetPciHostDeviceList(virConnectPtr conn,
> +                         virDomainDefPtr def)
> +{
> +    pciDeviceList *list;
>      int i;
>  
> -    /* We have to use 2 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
> -     */
> -
> -    for (i = 0 ; i < def->nhostdevs ; i++) {
> -        virDomainHostdevDefPtr hostdev = def->hostdevs[i];
> -
> -        if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
> -            continue;
> -        if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
> -            continue;
> -
> -        if (hostdev->managed) {
> -            pciDevice *dev = pciGetDevice(conn,
> -                                          hostdev->source.subsys.u.pci.domain,
> -                                          hostdev->source.subsys.u.pci.bus,
> -                                          hostdev->source.subsys.u.pci.slot,
> -                                          hostdev->source.subsys.u.pci.function);
> -            if (!dev)
> -                goto error;
> -
> -            if (pciDettachDevice(conn, dev) < 0) {
> -                pciFreeDevice(conn, dev);
> -                goto error;
> -            }
> -
> -            pciFreeDevice(conn, dev);
> -        } /* else {
> -             XXX validate that non-managed device isn't in use, eg
> -             by checking that device is either un-bound, or bound
> -             to pci-stub.ko
> -        } */
> -    }
> +    if (!(list = pciDeviceListNew(conn)))
> +        return NULL;
>  
> -    /* Now that all the PCI hostdevs have be dettached, we can safely
> -     * reset them */
>      for (i = 0 ; i < def->nhostdevs ; i++) {
>          virDomainHostdevDefPtr hostdev = def->hostdevs[i];
>          pciDevice *dev;
> @@ -1385,95 +1353,102 @@ static int qemuPrepareHostDevices(virConnectPtr conn,
>                             hostdev->source.subsys.u.pci.bus,
>                             hostdev->source.subsys.u.pci.slot,
>                             hostdev->source.subsys.u.pci.function);
> -        if (!dev)
> -            goto error;
> +        if (!dev) {
> +            pciDeviceListFree(conn, list);
> +            return NULL;
> +        }
>  
> -        if (pciResetDevice(conn, dev) < 0) {
> +        if (pciDeviceListAdd(conn, list, dev) < 0) {
>              pciFreeDevice(conn, dev);
> -            goto error;
> +            pciDeviceListFree(conn, list);
> +            return NULL;
>          }
>  
> -        pciFreeDevice(conn, dev);
> +        pciDeviceSetManaged(dev, hostdev->managed);
>      }
>  
> +    return list;
> +}
> +
> +static int
> +qemuPrepareHostDevices(virConnectPtr conn, virDomainDefPtr def)
> +{
> +    pciDeviceList *pcidevs;
> +    int i;
> +
> +    if (!def->nhostdevs)
> +        return 0;
> +
> +    if (!(pcidevs = qemuGetPciHostDeviceList(conn, def)))
> +        return -1;
> +
> +    /* We have to use 2 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
> +     */
> +
> +    /* XXX validate that non-managed device isn't in use, eg
> +     * by checking that device is either un-bound, or bound
> +     * to pci-stub.ko
> +     */
> +
> +    for (i = 0; i < pcidevs->count; i++)
> +        if (pciDeviceGetManaged(pcidevs->devs[i]) &&
> +            pciDettachDevice(conn, pcidevs->devs[i]) < 0)
> +            goto error;
> +
> +    /* 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)
> +            goto error;
> +
> +    pciDeviceListFree(conn, pcidevs);
>      return 0;
>  
>  error:
> +    pciDeviceListFree(conn, pcidevs);
>      return -1;
>  }
>  
>  static void
>  qemuDomainReAttachHostDevices(virConnectPtr conn, virDomainDefPtr def)
>  {
> +    pciDeviceList *pcidevs;
>      int i;
>  
> -    /* Again 2 loops; reset all the devices before re-attach */
> -
> -    for (i = 0 ; i < def->nhostdevs ; i++) {
> -        virDomainHostdevDefPtr hostdev = def->hostdevs[i];
> -        pciDevice *dev;
> -
> -        if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
> -            continue;
> -        if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
> -            continue;
> -
> -        dev = pciGetDevice(conn,
> -                           hostdev->source.subsys.u.pci.domain,
> -                           hostdev->source.subsys.u.pci.bus,
> -                           hostdev->source.subsys.u.pci.slot,
> -                           hostdev->source.subsys.u.pci.function);
> -        if (!dev) {
> -            virErrorPtr err = virGetLastError();
> -            VIR_ERROR(_("Failed to allocate pciDevice: %s\n"),
> -                      err ? err->message : "");
> -            virResetError(err);
> -            continue;
> -        }
> -
> -        if (pciResetDevice(conn, dev) < 0) {
> -            virErrorPtr err = virGetLastError();
> -            VIR_ERROR(_("Failed to reset PCI device: %s\n"),
> -                      err ? err->message : "");
> -            virResetError(err);
> -        }
> +    if (!def->nhostdevs)
> +        return;
>  
> -        pciFreeDevice(conn, dev);
> +    if (!(pcidevs = qemuGetPciHostDeviceList(conn, def))) {
> +        virErrorPtr err = virGetLastError();
> +        VIR_ERROR(_("Failed to allocate pciDeviceList: %s\n"),
> +                  err ? err->message : "");
> +        virResetError(err);
> +        return;
>      }
>  
> -    for (i = 0 ; i < def->nhostdevs ; i++) {
> -        virDomainHostdevDefPtr hostdev = def->hostdevs[i];
> -        pciDevice *dev;
> -
> -        if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
> -            continue;
> -        if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
> -            continue;
> -        if (!hostdev->managed)
> -            continue;
> +    /* Again 2 loops; reset all the devices before re-attach */
>  
> -        dev = pciGetDevice(conn,
> -                           hostdev->source.subsys.u.pci.domain,
> -                           hostdev->source.subsys.u.pci.bus,
> -                           hostdev->source.subsys.u.pci.slot,
> -                           hostdev->source.subsys.u.pci.function);
> -        if (!dev) {
> +    for (i = 0; i < pcidevs->count; i++)
> +        if (pciResetDevice(conn, pcidevs->devs[i]) < 0) {
>              virErrorPtr err = virGetLastError();
> -            VIR_ERROR(_("Failed to allocate pciDevice: %s\n"),
> +            VIR_ERROR(_("Failed to reset PCI device: %s\n"),
>                        err ? err->message : "");
>              virResetError(err);
> -            continue;
>          }
>  
> -        if (pciReAttachDevice(conn, dev) < 0) {
> +    for (i = 0; i < pcidevs->count; i++)
> +        if (pciDeviceGetManaged(pcidevs->devs[i]) &&
> +            pciReAttachDevice(conn, pcidevs->devs[i]) < 0) {
>              virErrorPtr err = virGetLastError();
>              VIR_ERROR(_("Failed to re-attach PCI device: %s\n"),
>                        err ? err->message : "");
>              virResetError(err);
>          }
>  
> -        pciFreeDevice(conn, dev);
> -    }
> +    pciDeviceListFree(conn, pcidevs);
>  }
>  
>  static const char *const defaultDeviceACL[] = {
> -- 

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]