Re: [libvirt] [PATCHv2 04/12] pci: new iommu_group functions

25 Jun 2013

On Mon, Jun 24, 2013 at 11:05:30PM -0400, Laine Stump wrote:
...
Any device which belongs to an "IOMMU group" (used by vfio) will
have links to all devices of its group listed in
/sys/bus/pci/$device/iommu_group/devices;
/sys/bus/pci/$device/iommu_group is actually a link to
/sys/kernel/iommu_groups/$n, where $n is the group number (there
will be a corresponding device node at /dev/vfio/$n once the
devices are bound to the vfio-pci driver)
...

diff --git a/src/util/virpci.c b/src/util/virpci.c
index fc04cac..5209372 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1852,6 +1852,223 @@ cleanup:
     return ret;
 }
+
+/* virPCIIOMMUGroupIterate:
+ *   Call @actor for all devices in the same iommu_group as orig
+ *   (including orig itself) Even if there is no iommu_group for the
+ *   device, call @actor once for orig.
+ */
+int
+virPCIIOMMUGroupIterate(virPCIDeviceAddressPtr orig,
+                        virPCIDeviceAddressActor actor,
+                        void *opaque)
+{
+    char *groupPath = NULL;
+    DIR *groupDir = NULL;
+    int ret = -1;
+    struct dirent *ent;
+
+    if (virAsprintf(&groupPath,
+                    PCI_SYSFS "devices/%04x:%02x:%02x.%x/iommu_group/devices",
+                    orig->domain, orig->bus, orig->slot, orig->function) < 0) {
+        virReportOOMError();
+        goto cleanup;
+    }
+
+    if (!(groupDir = opendir(groupPath))) {
+        /* just process the original device, nothing more */
+        ret = (actor)(orig, opaque);
+        goto cleanup;
+    }
+
Since Eric isn't around to nit-pick, I'll do it :-)

Set 'errno = 0' here before the readdir() call
...
+    while ((ent = readdir(groupDir)) != NULL) {
+        virPCIDeviceAddress newDev;
+
+        if (ent->d_name[0] == '.')
+            continue;
+
+        if (virPCIParseDeviceAddress(ent->d_name, &newDev) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Found invalid device link '%s' in '%s'"),
+                           ent->d_name, groupPath);
+            goto cleanup;
+        }
+
+        if ((actor)(&newDev, opaque) < 0)
+            goto cleanup;
And here

           errno = 0;
...
+    }
Then do

       if (errno != 0) {
          virReportSystemError(errno, _("Failed to read directory entry for %s"), grouppath)
          goto cleanup
       }

that way you distinguish EOF from error when readdir() returns NULL.

/me makes a note that we should write a wrapper around readdir(),
since almost all our code has this wrong.
...
+
...
+    ret = 0;
+
+cleanup:
+    VIR_FREE(groupPath);
+    if (groupDir)
+        closedir(groupDir);
+    return ret;
+}
+
+
+static int
+virPCIDeviceGetIOMMUGroupAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaque)
+{
+    int ret = -1;
+    virPCIDeviceListPtr groupList = opaque;
+    virPCIDevicePtr newDev;
+
+    if (!(newDev = virPCIDeviceNew(newDevAddr->domain, newDevAddr->bus,
+                                   newDevAddr->slot, newDevAddr->function))) {
+        goto cleanup;
+    }
+
+    if (virPCIDeviceListAdd(groupList, newDev) < 0) {
+        goto cleanup;
+    }
+    newDev = NULL; /* it's now on the list */
+    ret = 0;
+cleanup:
+    virPCIDeviceFree(newDev);
+    return ret;
+}
+
+
+/*
+ * virPCIDeviceGetIOMMUGroupList - return a virPCIDeviceList containing
+ * all of the devices in the same iommu_group as @dev.
+ *
+ * Return the new list, or NULL on failure
+ */
+virPCIDeviceListPtr
+virPCIDeviceGetIOMMUGroupList(virPCIDevicePtr dev)
+{
+    virPCIDeviceListPtr groupList = virPCIDeviceListNew();
+    virPCIDeviceAddress devAddr = { dev->domain, dev->bus,
+                                    dev->slot, dev->function };
+
+    if (!groupList)
+        goto error;
+
+    if (virPCIIOMMUGroupIterate(&devAddr, virPCIDeviceGetIOMMUGroupAddOne,
+                                groupList) < 0) {
+        goto error;
+    }
Overkill with  {} here
...
+static int
+virPCIGetIOMMUGroupAddressesAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaque)
+{
+    int ret = -1;
+    virPCIDeviceAddressListPtr addrList = opaque;
+    virPCIDeviceAddressPtr copyAddr;
+
+    /* make a copy to insert onto the list */
+    if (VIR_ALLOC(copyAddr) < 0)
+        goto cleanup;
+
+    *copyAddr = *newDevAddr;
+
+    if (VIR_APPEND_ELEMENT(*addrList->iommuGroupDevices,
+                           *addrList->nIommuGroupDevices, copyAddr) < 0) {
+        goto cleanup;
+    }
Overkill with {}

Missing virReportOOMError() ?
...
+
+    ret = 0;
+cleanup:
+    VIR_FREE(copyAddr);
+    return ret;
+}
+
+
+/*
+ * virPCIGetIOMMUGroupAddresses - return a virPCIDeviceList containing
+ * all of the devices in the same iommu_group as @dev.
+ *
+ * Return the new list, or NULL on failure
+ */
+int
+virPCIGetIOMMUGroupAddresses(virPCIDeviceAddressPtr devAddr,
+                             virPCIDeviceAddressPtr **iommuGroupDevices,
+                             size_t *nIommuGroupDevices)
+{
+    int ret = -1;
+    virPCIDeviceAddressList addrList = { iommuGroupDevices,
+                                         nIommuGroupDevices };
+
+    if (virPCIIOMMUGroupIterate(devAddr,
+                                virPCIGetIOMMUGroupAddressesAddOne,
+                                &addrList) < 0) {
+        goto cleanup;
+    }
Overkill with {}
...
+
+    ret = 0;
+cleanup:
+    return ret;
+}
+
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

    

Re: [libvirt] [PATCHv2 04/12] pci: new iommu_group functions

Daniel P. Berrange