[PATCH] pci: Use virPCIDeviceAddress in virPCIDevice

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

 



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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]