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

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(-)

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[] = { --