8:42 a.m.
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,
--
2.5.0