Instead of replicating the information (domain, bus, slot, function) inside the virPCIDevice structure, use the already-existing virPCIDeviceAddress structure. For users of the module, this means that the object returned by virPCIDeviceGetAddress() can no longer be NULL and must no longer be freed by the caller. --- Changes in v3: * don't check the return value of virPCIDeviceGetAddress(), it can no longer be NULL * don't use a variable to store the device address if it's only going to be used a single time Changes in v2: * use virPCIDeviceAddress instead of virPCIDeviceAddressPtr to avoid extra allocations src/util/virhostdev.c | 20 ++--------- src/util/virpci.c | 97 +++++++++++++++++++++++---------------------------- 2 files changed, 47 insertions(+), 70 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index de029a0..4065535 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -585,15 +585,12 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, goto cleanup; } - VIR_FREE(devAddr); - if (!(devAddr = virPCIDeviceGetAddress(dev))) - goto cleanup; - /* The device is in use by other active domain if * the dev is in list activePCIHostdevs. VFIO devices * belonging to same iommu group can't be shared * across guests. */ + devAddr = virPCIDeviceGetAddress(dev); if (usesVfio) { if (virPCIDeviceAddressIOMMUGroupIterate(devAddr, virHostdevIsPCINodeDeviceUsed, @@ -728,7 +725,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virObjectUnlock(hostdev_mgr->activePCIHostdevs); virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); virObjectUnref(pcidevs); - VIR_FREE(devAddr); return ret; } @@ -1558,7 +1554,6 @@ int virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr pci) { - virPCIDeviceAddressPtr devAddr = NULL; struct virHostdevIsPCINodeDeviceUsedData data = { hostdev_mgr, NULL, false }; int ret = -1; @@ -1566,10 +1561,7 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, virObjectLock(hostdev_mgr->activePCIHostdevs); virObjectLock(hostdev_mgr->inactivePCIHostdevs); - if (!(devAddr = virPCIDeviceGetAddress(pci))) - goto out; - - if (virHostdevIsPCINodeDeviceUsed(devAddr, &data)) + if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) goto out; if (virPCIDeviceDetach(pci, hostdev_mgr->activePCIHostdevs, @@ -1581,7 +1573,6 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, out: virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); virObjectUnlock(hostdev_mgr->activePCIHostdevs); - VIR_FREE(devAddr); return ret; } @@ -1589,7 +1580,6 @@ int virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr pci) { - virPCIDeviceAddressPtr devAddr = NULL; struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, NULL, false}; int ret = -1; @@ -1597,10 +1587,7 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, virObjectLock(hostdev_mgr->activePCIHostdevs); virObjectLock(hostdev_mgr->inactivePCIHostdevs); - if (!(devAddr = virPCIDeviceGetAddress(pci))) - goto out; - - if (virHostdevIsPCINodeDeviceUsed(devAddr, &data)) + if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) goto out; virPCIDeviceReattachInit(pci); @@ -1613,7 +1600,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 7ca3fef..6f0cb8c 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; + virPCIDeviceAddress address; char name[PCI_ADDR_LEN]; /* domain:bus:slot.function */ char id[PCI_ID_LEN]; /* product vendor */ @@ -655,10 +652,10 @@ virPCIDeviceSharesBusWithActive(virPCIDevicePtr dev, virPCIDevicePtr check, void virPCIDeviceList *inactiveDevs = data; /* 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 (dev->address.domain != check->address.domain || + dev->address.bus != check->address.bus || + (dev->address.slot == check->address.slot && + dev->address.function == check->address.function)) return 0; /* same bus, but inactive, i.e. about to be assigned to guest */ @@ -689,7 +686,7 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data) int ret = 0; int fd; - if (dev->domain != check->domain) + if (dev->address.domain != check->address.domain) return 0; if ((fd = virPCIDeviceConfigOpen(check, false)) < 0) @@ -713,7 +710,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 (dev->address.bus == secondary) { ret = 1; goto cleanup; } @@ -722,10 +719,12 @@ 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 (dev->address.bus > secondary && dev->address.bus <= subordinate) { if (*best == NULL) { - *best = virPCIDeviceNew(check->domain, check->bus, check->slot, - check->function); + *best = virPCIDeviceNew(check->address.domain, + check->address.bus, + check->address.slot, + check->address.function); if (*best == NULL) { ret = -1; goto cleanup; @@ -745,8 +744,10 @@ 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(check->address.domain, + check->address.bus, + check->address.slot, + check->address.function); if (*best == NULL) { ret = -1; goto cleanup; @@ -970,7 +971,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 && dev->address.bus != 0) ret = virPCIDeviceTrySecondaryBusReset(dev, fd, inactiveDevs); if (ret < 0) { @@ -1483,8 +1484,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 != dev->address.domain || bus != dev->address.bus || + slot != dev->address.slot || function != dev->address.function) continue; in_matching_device = true; match_depth = strspn(line, " "); @@ -1560,17 +1561,16 @@ virPCIDeviceNew(unsigned int domain, if (VIR_ALLOC(dev) < 0) return NULL; - dev->domain = domain; - dev->bus = bus; - dev->slot = slot; - dev->function = function; + dev->address.domain = domain; + dev->address.bus = bus; + dev->address.slot = slot; + dev->address.function = function; 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", @@ -1661,25 +1661,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. + * Returns: a pointer to the address, which can never be NULL. */ 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 * @@ -1890,12 +1879,14 @@ virPCIDeviceListFindIndex(virPCIDeviceListPtr list, virPCIDevicePtr dev) { 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++) { + virPCIDevicePtr other = list->devs[i]; + if (other->address.domain == dev->address.domain && + other->address.bus == dev->address.bus && + other->address.slot == dev->address.slot && + other->address.function == dev->address.function) return i; + } return -1; } @@ -1910,10 +1901,11 @@ virPCIDeviceListFindByIDs(virPCIDeviceListPtr list, 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) + virPCIDevicePtr other = list->devs[i]; + if (other->address.domain == domain && + other->address.bus == bus && + other->address.slot == slot && + other->address.function == function) return list->devs[i]; } return NULL; @@ -1944,7 +1936,8 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, int direrr; if (virAsprintf(&pcidir, "/sys/bus/pci/devices/%04x:%02x:%02x.%x", - dev->domain, dev->bus, dev->slot, dev->function) < 0) + dev->address.domain, dev->address.bus, + dev->address.slot, dev->address.function) < 0) goto cleanup; if (!(dir = opendir(pcidir))) { @@ -2074,13 +2067,11 @@ virPCIDeviceListPtr virPCIDeviceGetIOMMUGroupList(virPCIDevicePtr dev) { virPCIDeviceListPtr groupList = virPCIDeviceListNew(); - virPCIDeviceAddress devAddr = { dev->domain, dev->bus, - dev->slot, dev->function }; if (!groupList) goto error; - if (virPCIDeviceAddressIOMMUGroupIterate(&devAddr, + if (virPCIDeviceAddressIOMMUGroupIterate(&(dev->address), virPCIDeviceGetIOMMUGroupAddOne, groupList) < 0) goto error; @@ -2294,7 +2285,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 (dev->address.bus == 0) { return 0; } else { virReportError(VIR_ERR_INTERNAL_ERROR, -- 2.5.0