Re: [libvirt] [PATCH] pci: Use virPCIDeviceAddress in virPCIDevice

Instead of replicating the information (domain, bus, slot, function) inside the virPCIDevice structure, use the already-existing virPCIDeviceAddress structure.

Outside of the module, the only visible change is that the return value of virPCIDeviceGetAddress() must no longer be freed by the caller. --- Suggested by Laine[1] in the context of a patch series; since the idea is not really tied to the series, sending as a stand-alone cleanup. [1] https://www.redhat.com/archives/libvir-list/2015-December/msg00125.html src/util/virhostdev.c | 4 -- src/util/virpci.c | 182 ++++++++++++++++++++++++++++++++++---------------- src/util/virpci.h | 7 ++ 3 files changed, 133 insertions(+), 60 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index de029a0..173ac58 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -585,7 +585,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, goto cleanup; } - VIR_FREE(devAddr); if (!(devAddr = virPCIDeviceGetAddress(dev))) goto cleanup; @@ -728,7 +727,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virObjectUnlock(hostdev_mgr->activePCIHostdevs); virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); virObjectUnref(pcidevs); - VIR_FREE(devAddr); return ret; } @@ -1581,7 +1579,6 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, out: virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); virObjectUnlock(hostdev_mgr->activePCIHostdevs); - VIR_FREE(devAddr); return ret; } @@ -1613,7 +1610,6 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, out: virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); virObjectUnlock(hostdev_mgr->activePCIHostdevs); - VIR_FREE(devAddr); return ret; } diff --git a/src/util/virpci.c b/src/util/virpci.c index bececb5..2818bf7 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -56,10 +56,7 @@ VIR_ENUM_IMPL(virPCIELinkSpeed, VIR_PCIE_LINK_SPEED_LAST, "", "2.5", "5", "8") struct _virPCIDevice { - unsigned int domain; - unsigned int bus; - unsigned int slot; - unsigned int function; + virPCIDeviceAddressPtr address; char name[PCI_ADDR_LEN]; /* domain:bus:slot.function */ char id[PCI_ID_LEN]; /* product vendor */ @@ -653,12 +650,14 @@ static int virPCIDeviceSharesBusWithActive(virPCIDevicePtr dev, virPCIDevicePtr check, void *data) { virPCIDeviceList *inactiveDevs = data; + virPCIDeviceAddressPtr devAddr = dev->address; + virPCIDeviceAddressPtr checkAddr = check->address; /* Different domain, different bus, or simply identical device */ - if (dev->domain != check->domain || - dev->bus != check->bus || - (dev->slot == check->slot && - dev->function == check->function)) + if (devAddr->domain != checkAddr->domain || + devAddr->bus != checkAddr->bus || + (devAddr->slot == checkAddr->slot && + devAddr->function == checkAddr->function)) return 0; /* same bus, but inactive, i.e. about to be assigned to guest */ @@ -686,10 +685,12 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data) uint16_t device_class; uint8_t header_type, secondary, subordinate; virPCIDevicePtr *best = data; + virPCIDeviceAddressPtr devAddr = dev->address; + virPCIDeviceAddressPtr checkAddr = check->address; int ret = 0; int fd; - if (dev->domain != check->domain) + if (devAddr->domain != checkAddr->domain) return 0; if ((fd = virPCIDeviceConfigOpen(check, false)) < 0) @@ -713,7 +714,7 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data) /* if the secondary bus exactly equals the device's bus, then we found * the direct parent. No further work is necessary */ - if (dev->bus == secondary) { + if (devAddr->bus == secondary) { ret = 1; goto cleanup; } @@ -722,10 +723,10 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data) * In this case, what we need to do is look for the "best" match; i.e. * the most restrictive match that still satisfies all of the conditions. */ - if (dev->bus > secondary && dev->bus <= subordinate) { + if (devAddr->bus > secondary && devAddr->bus <= subordinate) { if (*best == NULL) { - *best = virPCIDeviceNew(check->domain, check->bus, check->slot, - check->function); + *best = virPCIDeviceNew(checkAddr->domain, checkAddr->bus, + checkAddr->slot, checkAddr->function); if (*best == NULL) { ret = -1; goto cleanup; @@ -745,8 +746,8 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data) if (secondary > best_secondary) { virPCIDeviceFree(*best); - *best = virPCIDeviceNew(check->domain, check->bus, check->slot, - check->function); + *best = virPCIDeviceNew(checkAddr->domain, checkAddr->bus, + checkAddr->slot, checkAddr->function); if (*best == NULL) { ret = -1; goto cleanup; @@ -925,6 +926,7 @@ virPCIDeviceReset(virPCIDevicePtr dev, char *drvName = NULL; int ret = -1; int fd = -1; + virPCIDeviceAddressPtr devAddr = dev->address; if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -970,7 +972,7 @@ virPCIDeviceReset(virPCIDevicePtr dev, ret = virPCIDeviceTryPowerManagementReset(dev, fd); /* Bus reset is not an option with the root bus */ - if (ret < 0 && dev->bus != 0) + if (ret < 0 && devAddr->bus != 0) ret = virPCIDeviceTrySecondaryBusReset(dev, fd, inactiveDevs); if (ret < 0) { @@ -1428,6 +1430,7 @@ virPCIDeviceWaitForCleanup(virPCIDevicePtr dev, const char *matcher) bool in_matching_device; int ret; size_t match_depth; + virPCIDeviceAddressPtr devAddr = dev->address; fp = fopen("/proc/iomem", "r"); if (!fp) { @@ -1483,8 +1486,8 @@ virPCIDeviceWaitForCleanup(virPCIDevicePtr dev, const char *matcher) virStrToLong_ui(tmp + 1, &tmp, 16, &function) < 0 || *tmp != '\n') continue; - if (domain != dev->domain || bus != dev->bus || slot != dev->slot || - function != dev->function) + if (domain != devAddr->domain || bus != devAddr->bus || + slot != devAddr->slot || function != devAddr->function) continue; in_matching_device = true; match_depth = strspn(line, " "); @@ -1547,6 +1550,72 @@ virPCIGetAddrString(unsigned int domain, return ret; } +/** + * virPCIDeviceAddressNew: + * @domain: PCI domain + * @bus: PCI bus + * @slot: PCI slot + * @function: PCI function + * + * Create a new virPCIDeviceAddress object. + * + * Returns: a new address, or NULL on failure. + */ +virPCIDeviceAddressPtr +virPCIDeviceAddressNew(unsigned int domain, + unsigned int bus, + unsigned int slot, + unsigned int function) +{ + virPCIDeviceAddressPtr addr; + + if (VIR_ALLOC(addr) < 0) + return NULL; + + addr->domain = domain; + addr->bus = bus; + addr->slot = slot; + addr->function = function; + + return addr; +} + +/** + * virPCIDeviceAddressCopy: + * @addr: PCI address + * + * Create a copy of a PCI address. + * + * Returns: a copy of @addr, or NULL on failure. + */ +virPCIDeviceAddressPtr +virPCIDeviceAddressCopy(virPCIDeviceAddressPtr addr) +{ + virPCIDeviceAddressPtr copy; + + if (!addr) + return NULL; + + if (VIR_ALLOC(copy) < 0) + return NULL; + + *copy = *addr; + + return copy; +} + +/** + * virPCIDeviceAddressFree: + * @addr: PCI address + * + * Release all resources associated with a PCI address. + */ +void +virPCIDeviceAddressFree(virPCIDeviceAddressPtr addr) +{ + VIR_FREE(addr); +} + virPCIDevicePtr virPCIDeviceNew(unsigned int domain, unsigned int bus, @@ -1560,17 +1629,14 @@ virPCIDeviceNew(unsigned int domain, if (VIR_ALLOC(dev) < 0) return NULL; - dev->domain = domain; - dev->bus = bus; - dev->slot = slot; - dev->function = function; + if (!(dev->address = virPCIDeviceAddressNew(domain, bus, slot, function))) + goto error; if (snprintf(dev->name, sizeof(dev->name), "%.4x:%.2x:%.2x.%.1x", - dev->domain, dev->bus, dev->slot, - dev->function) >= sizeof(dev->name)) { + domain, bus, slot, function) >= sizeof(dev->name)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("dev->name buffer overflow: %.4x:%.2x:%.2x.%.1x"), - dev->domain, dev->bus, dev->slot, dev->function); + domain, bus, slot, function); goto error; } if (virAsprintf(&dev->path, PCI_SYSFS "devices/%s/config", @@ -1627,9 +1693,13 @@ virPCIDeviceCopy(virPCIDevicePtr dev) /* shallow copy to take care of most attributes */ *copy = *dev; + + /* deep copy for the rest */ + copy->address = NULL; copy->path = copy->stubDriver = NULL; copy->used_by_drvname = copy->used_by_domname = NULL; - if (VIR_STRDUP(copy->path, dev->path) < 0 || + if (!(copy->address = virPCIDeviceAddressCopy(dev->address)) || + VIR_STRDUP(copy->path, dev->path) < 0 || VIR_STRDUP(copy->stubDriver, dev->stubDriver) < 0 || VIR_STRDUP(copy->used_by_drvname, dev->used_by_drvname) < 0 || VIR_STRDUP(copy->used_by_domname, dev->used_by_domname) < 0) { @@ -1649,6 +1719,7 @@ virPCIDeviceFree(virPCIDevicePtr dev) if (!dev) return; VIR_DEBUG("%s %s: freeing", dev->id, dev->name); + virPCIDeviceAddressFree(dev->address); VIR_FREE(dev->path); VIR_FREE(dev->stubDriver); VIR_FREE(dev->used_by_drvname); @@ -1661,25 +1732,14 @@ virPCIDeviceFree(virPCIDevicePtr dev) * @dev: device to get address from * * Take a PCI device on input and return its PCI address. The - * caller must free the returned value when no longer needed. + * returned object is owned by the device and must not be freed. * * Returns NULL on failure, the device address on success. */ virPCIDeviceAddressPtr virPCIDeviceGetAddress(virPCIDevicePtr dev) { - - virPCIDeviceAddressPtr pciAddrPtr; - - if (!dev || (VIR_ALLOC(pciAddrPtr) < 0)) - return NULL; - - pciAddrPtr->domain = dev->domain; - pciAddrPtr->bus = dev->bus; - pciAddrPtr->slot = dev->slot; - pciAddrPtr->function = dev->function; - - return pciAddrPtr; + return dev->address; } const char * @@ -1888,14 +1948,18 @@ virPCIDeviceListDel(virPCIDeviceListPtr list, int virPCIDeviceListFindIndex(virPCIDeviceListPtr list, virPCIDevicePtr dev) { + virPCIDeviceAddressPtr devAddr = dev->address; + virPCIDeviceAddressPtr tmpAddr; size_t 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) + for (i = 0; i < list->count; i++) { + tmpAddr = list->devs[i]->address; + if (tmpAddr->domain == devAddr->domain && + tmpAddr->bus == devAddr->bus && + tmpAddr->slot == devAddr->slot && + tmpAddr->function == devAddr->function) return i; + } return -1; } @@ -1907,13 +1971,16 @@ virPCIDeviceListFindByIDs(virPCIDeviceListPtr list, unsigned int slot, unsigned int function) { + virPCIDeviceAddressPtr tmpAddr; size_t i; for (i = 0; i < list->count; i++) { - if (list->devs[i]->domain == domain && - list->devs[i]->bus == bus && - list->devs[i]->slot == slot && - list->devs[i]->function == function) + tmpAddr = list->devs[i]->address; + if (tmpAddr && + tmpAddr->domain == domain && + tmpAddr->bus == bus && + tmpAddr->slot == slot && + tmpAddr->function == function) return list->devs[i]; } return NULL; @@ -1942,9 +2009,11 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, int ret = -1; struct dirent *ent; int direrr; + virPCIDeviceAddressPtr devAddr = dev->address; if (virAsprintf(&pcidir, "/sys/bus/pci/devices/%04x:%02x:%02x.%x", - dev->domain, dev->bus, dev->slot, dev->function) < 0) + devAddr->domain, devAddr->bus, + devAddr->slot, devAddr->function) < 0) goto cleanup; if (!(dir = opendir(pcidir))) { @@ -2074,13 +2143,12 @@ virPCIDeviceListPtr virPCIDeviceGetIOMMUGroupList(virPCIDevicePtr dev) { virPCIDeviceListPtr groupList = virPCIDeviceListNew(); - virPCIDeviceAddress devAddr = { dev->domain, dev->bus, - dev->slot, dev->function }; + virPCIDeviceAddressPtr devAddr = dev->address; if (!groupList) goto error; - if (virPCIDeviceAddressIOMMUGroupIterate(&devAddr, + if (virPCIDeviceAddressIOMMUGroupIterate(devAddr, virPCIDeviceGetIOMMUGroupAddOne, groupList) < 0) goto error; @@ -2286,6 +2354,7 @@ static int virPCIDeviceIsBehindSwitchLackingACS(virPCIDevicePtr dev) { virPCIDevicePtr parent; + virPCIDeviceAddressPtr devAddr = dev->address; if (virPCIDeviceGetParent(dev, &parent) < 0) return -1; @@ -2294,7 +2363,7 @@ virPCIDeviceIsBehindSwitchLackingACS(virPCIDevicePtr dev) * into play since devices on the root bus can't P2P without going * through the root IOMMU. */ - if (dev->bus == 0) { + if (devAddr->bus == 0) { return 0; } else { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2664,12 +2733,13 @@ virPCIGetSysfsFile(char *virPCIDeviceName, char **pci_sysfs_device_link) } int -virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddressPtr dev, +virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddressPtr addr, char **pci_sysfs_device_link) { if (virAsprintf(pci_sysfs_device_link, - PCI_SYSFS "devices/%04x:%02x:%02x.%x", dev->domain, - dev->bus, dev->slot, dev->function) < 0) + PCI_SYSFS "devices/%04x:%02x:%02x.%x", + addr->domain, addr->bus, + addr->slot, addr->function) < 0) return -1; return 0; } diff --git a/src/util/virpci.h b/src/util/virpci.h index 6516f05..a137a9a 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -69,6 +69,13 @@ struct _virPCIEDeviceInfo { virPCIELink *link_sta; /* Actually negotiated capabilities */ }; +virPCIDeviceAddressPtr virPCIDeviceAddressNew(unsigned int domain, + unsigned int bus, + unsigned int slot, + unsigned int function); +virPCIDeviceAddressPtr virPCIDeviceAddressCopy(virPCIDeviceAddressPtr addr); +void virPCIDeviceAddressFree(virPCIDeviceAddressPtr addr); + virPCIDevicePtr virPCIDeviceNew(unsigned int domain, unsigned int bus, unsigned int slot,