[Qemu-devel] [RFC PATCH] vfio: Add sysfsdev property for pci & platform

[Qemu-devel] [RFC PATCH] vfio: Add sysfsdev property for pci & platform

[Alex Williamson]

vfio-pci currently requires a host= parameter, which comes in the
form of a PCI address in [domain:]<bus:slot.function> notation.  We
expect to find a matching entry in sysfs for that under
/sys/bus/pci/devices/.  vfio-platform takes a similar approach, but
defines the host= parameter to be a string, which can be matched
directly under /sys/bus/platform/devices/.  On the PCI side, we have
some interest in using vfio to expose vGPU devices.  These are not
actual discrete PCI devices, so they don't have a compatible host PCI
bus address or a device link where QEMU wants to look for it.  There's
also really no requirement that vfio can only be used to expose
physical devices, a new vfio bus and iommu driver could expose a
completely emulated device.  To fit within the vfio framework, it
would need a kernel struct device and associated IOMMU group, but
those are easy constraints to manage.

To support such devices, which would include vGPUs, that honor the
VFIO PCI programming API, but are not necessarily backed by a unique
PCI address, add support for specifying any device in sysfs.  The
vfio API already has support for probing the device type to ensure
compatibility with either vfio-pci or vfio-platform.

With this, a vfio-pci device could either be specified as:

-device vfio-pci,host=02:00.0

or

-device vfio-pci,sysfsdev=/sys/devices/pci0000:00/0000:00:1c.0/0000:02:00.0

or even

-device vfio-pci,sysfsdev=/sys/bus/pci/devices/0000:02:00.0

When vGPU support comes along, this might look something more like:

-device vfio-pci,sysfsdev=/sys/devices/virtual/intel-vgpu/vgpu0@0000:00:02.0

NB - This is only a made up example path, but it should be noted that
the device namespace is global for vfio, a virtual device cannot
overlap with existing namespaces and should not create a name prone to
conflict, such as a simple instance number.

The same changes is made for vfio-platform but is only compile tested.
In both cases, specifying sysfsdev has precedence over the old host
option.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/vfio/pci.c                 |  130 +++++++++++++++++------------------------
 hw/vfio/platform.c            |   55 ++++++++++-------
 include/hw/vfio/vfio-common.h |    1 
 3 files changed, 86 insertions(+), 100 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 1fb868c..bfe4215 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -880,12 +880,8 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
     if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
         /* Since pci handles romfile, just print a message and return */
         if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.romfile) {
-            error_printf("Warning : Device at %04x:%02x:%02x.%x "
-                         "is known to cause system instability issues during "
-                         "option rom execution. "
-                         "Proceeding anyway since user specified romfile\n",
-                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
-                         vdev->host.function);
+            error_printf("Warning : Device at %s is known to cause system instability issues during option rom execution. Proceeding anyway since user specified romfile\n",
+                         vdev->vbasedev.name);
         }
         return;
     }
@@ -898,9 +894,7 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
         pwrite(fd, &size, 4, offset) != 4 ||
         pread(fd, &size, 4, offset) != 4 ||
         pwrite(fd, &orig, 4, offset) != 4) {
-        error_report("%s(%04x:%02x:%02x.%x) failed: %m",
-                     __func__, vdev->host.domain, vdev->host.bus,
-                     vdev->host.slot, vdev->host.function);
+        error_report("%s(%s) failed: %m", __func__, vdev->vbasedev.name);
         return;
     }
 
@@ -912,29 +906,18 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
 
     if (vfio_blacklist_opt_rom(vdev)) {
         if (dev->opts && qemu_opt_get(dev->opts, "rombar")) {
-            error_printf("Warning : Device at %04x:%02x:%02x.%x "
-                         "is known to cause system instability issues during "
-                         "option rom execution. "
-                         "Proceeding anyway since user specified non zero value for "
-                         "rombar\n",
-                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
-                         vdev->host.function);
+            error_printf("Warning : Device at %s is known to cause system instability issues during option rom execution. Proceeding anyway since user specified non zero value for rombar\n",
+                         vdev->vbasedev.name);
         } else {
-            error_printf("Warning : Rom loading for device at "
-                         "%04x:%02x:%02x.%x has been disabled due to "
-                         "system instability issues. "
-                         "Specify rombar=1 or romfile to force\n",
-                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
-                         vdev->host.function);
+            error_printf("Warning : Rom loading for device at %s has been disabled due to system instability issues. Specify rombar=1 or romfile to force\n",
+                         vdev->vbasedev.name);
             return;
         }
     }
 
     trace_vfio_pci_size_rom(vdev->vbasedev.name, size);
 
-    snprintf(name, sizeof(name), "vfio[%04x:%02x:%02x.%x].rom",
-             vdev->host.domain, vdev->host.bus, vdev->host.slot,
-             vdev->host.function);
+    snprintf(name, sizeof(name), "vfio[%s].rom", vdev->vbasedev.name);
 
     memory_region_init_io(&vdev->pdev.rom, OBJECT(vdev),
                           &vfio_rom_ops, vdev, name, size);
@@ -1048,9 +1031,8 @@ uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len)
         ret = pread(vdev->vbasedev.fd, &phys_val, len,
                     vdev->config_offset + addr);
         if (ret != len) {
-            error_report("%s(%04x:%02x:%02x.%x, 0x%x, 0x%x) failed: %m",
-                         __func__, vdev->host.domain, vdev->host.bus,
-                         vdev->host.slot, vdev->host.function, addr, len);
+            error_report("%s(%s, 0x%x, 0x%x) failed: %m",
+                         __func__, vdev->vbasedev.name, addr, len);
             return -errno;
         }
         phys_val = le32_to_cpu(phys_val);
@@ -1074,9 +1056,8 @@ void vfio_pci_write_config(PCIDevice *pdev,
     /* Write everything to VFIO, let it filter out what we can't write */
     if (pwrite(vdev->vbasedev.fd, &val_le, len, vdev->config_offset + addr)
                 != len) {
-        error_report("%s(%04x:%02x:%02x.%x, 0x%x, 0x%x, 0x%x) failed: %m",
-                     __func__, vdev->host.domain, vdev->host.bus,
-                     vdev->host.slot, vdev->host.function, addr, val, len);
+        error_report("%s(%s, 0x%x, 0x%x, 0x%x) failed: %m",
+                     __func__, vdev->vbasedev.name, addr, val, len);
     }
 
     /* MSI/MSI-X Enabling/Disabling */
@@ -1347,9 +1328,7 @@ static void vfio_map_bar(VFIOPCIDevice *vdev, int nr)
         return;
     }
 
-    snprintf(name, sizeof(name), "VFIO %04x:%02x:%02x.%x BAR %d",
-             vdev->host.domain, vdev->host.bus, vdev->host.slot,
-             vdev->host.function, nr);
+    snprintf(name, sizeof(name), "VFIO %s BAR %d", vdev->vbasedev.name, nr);
 
     /* Determine what type of BAR this is for registration */
     ret = pread(vdev->vbasedev.fd, &pci_bar, sizeof(pci_bar),
@@ -1719,9 +1698,8 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
     }
 
     if (ret < 0) {
-        error_report("vfio: %04x:%02x:%02x.%x Error adding PCI capability "
-                     "0x%x[0x%x]@0x%x: %d", vdev->host.domain,
-                     vdev->host.bus, vdev->host.slot, vdev->host.function,
+        error_report("vfio: %s Error adding PCI capability "
+                     "0x%x[0x%x]@0x%x: %d", vdev->vbasedev.name,
                      cap_id, size, pos, ret);
         return ret;
     }
@@ -1783,11 +1761,14 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev)
     vfio_intx_enable(vdev);
 }
 
-static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
-                                PCIHostDeviceAddress *host2)
+static bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name)
 {
-    return (host1->domain == host2->domain && host1->bus == host2->bus &&
-            host1->slot == host2->slot && host1->function == host2->function);
+    char tmp[13];
+
+    sprintf(tmp, "%04x:%02x:%02x.%1x", addr->domain,
+            addr->bus, addr->slot, addr->function);
+
+    return (strcmp(tmp, name) == 0);
 }
 
 static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
@@ -1812,9 +1793,8 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
     if (ret && errno != ENOSPC) {
         ret = -errno;
         if (!vdev->has_pm_reset) {
-            error_report("vfio: Cannot reset device %04x:%02x:%02x.%x, "
-                         "no available reset mechanism.", vdev->host.domain,
-                         vdev->host.bus, vdev->host.slot, vdev->host.function);
+            error_report("vfio: Cannot reset device %s, "
+                         "no available reset mechanism.", vdev->vbasedev.name);
         }
         goto out_single;
     }
@@ -1847,7 +1827,7 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
         trace_vfio_pci_hot_reset_dep_devices(host.domain,
                 host.bus, host.slot, host.function, devices[i].group_id);
 
-        if (vfio_pci_host_match(&host, &vdev->host)) {
+        if (vfio_pci_host_match(&host, vdev->vbasedev.name)) {
             continue;
         }
 
@@ -1873,7 +1853,7 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
                 continue;
             }
             tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
-            if (vfio_pci_host_match(&host, &tmp->host)) {
+            if (vfio_pci_host_match(&host, tmp->vbasedev.name)) {
                 if (single) {
                     ret = -EINVAL;
                     goto out_single;
@@ -1935,7 +1915,7 @@ out:
         host.slot = PCI_SLOT(devices[i].devfn);
         host.function = PCI_FUNC(devices[i].devfn);
 
-        if (vfio_pci_host_match(&host, &vdev->host)) {
+        if (vfio_pci_host_match(&host, vdev->vbasedev.name)) {
             continue;
         }
 
@@ -1954,7 +1934,7 @@ out:
                 continue;
             }
             tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
-            if (vfio_pci_host_match(&host, &tmp->host)) {
+            if (vfio_pci_host_match(&host, tmp->vbasedev.name)) {
                 vfio_pci_post_reset(tmp);
                 break;
             }
@@ -2160,10 +2140,7 @@ static void vfio_err_notifier_handler(void *opaque)
      * guest to contain the error.
      */
 
-    error_report("%s(%04x:%02x:%02x.%x) Unrecoverable error detected.  "
-                 "Please collect any data possible and then kill the guest",
-                 __func__, vdev->host.domain, vdev->host.bus,
-                 vdev->host.slot, vdev->host.function);
+    error_report("%s(%s) Unrecoverable error detected. Please collect any data possible and then kill the guest", __func__, vdev->vbasedev.name);
 
     vm_stop(RUN_STATE_INTERNAL_ERROR);
 }
@@ -2344,42 +2321,43 @@ static int vfio_initfn(PCIDevice *pdev)
     VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
     VFIODevice *vbasedev_iter;
     VFIOGroup *group;
-    char path[PATH_MAX], iommu_group_path[PATH_MAX], *group_name;
+    char *tmp, group_path[PATH_MAX], *group_name;
     ssize_t len;
     struct stat st;
     int groupid;
     int ret;
 
-    /* Check that the host device exists */
-    snprintf(path, sizeof(path),
-             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/",
-             vdev->host.domain, vdev->host.bus, vdev->host.slot,
-             vdev->host.function);
-    if (stat(path, &st) < 0) {
-        error_report("vfio: error: no such host device: %s", path);
+    if (!vdev->vbasedev.sysfsdev) {
+        vdev->vbasedev.sysfsdev =
+            g_strdup_printf("/sys/bus/pci/devices/%04x:%02x:%02x.%01x",
+                            vdev->host.domain, vdev->host.bus,
+                            vdev->host.slot, vdev->host.function);
+    }
+
+    if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
+        error_report("vfio: error: no such host device: %s",
+                     vdev->vbasedev.sysfsdev);
         return -errno;
     }
 
+    vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
     vdev->vbasedev.ops = &vfio_pci_ops;
-
     vdev->vbasedev.type = VFIO_DEVICE_TYPE_PCI;
-    vdev->vbasedev.name = g_strdup_printf("%04x:%02x:%02x.%01x",
-                                          vdev->host.domain, vdev->host.bus,
-                                          vdev->host.slot, vdev->host.function);
 
-    strncat(path, "iommu_group", sizeof(path) - strlen(path) - 1);
+    tmp = g_strdup_printf("%s/iommu_group", vdev->vbasedev.sysfsdev);
+    len = readlink(tmp, group_path, sizeof(group_path));
+    g_free(tmp);
 
-    len = readlink(path, iommu_group_path, sizeof(path));
-    if (len <= 0 || len >= sizeof(path)) {
+    if (len <= 0 || len >= sizeof(group_path)) {
         error_report("vfio: error no iommu_group for device");
         return len < 0 ? -errno : -ENAMETOOLONG;
     }
 
-    iommu_group_path[len] = 0;
-    group_name = basename(iommu_group_path);
+    group_path[len] = 0;
 
+    group_name = basename(group_path);
     if (sscanf(group_name, "%d", &groupid) != 1) {
-        error_report("vfio: error reading %s: %m", path);
+        error_report("vfio: error reading %s: %m", group_path);
         return -errno;
     }
 
@@ -2391,21 +2369,18 @@ static int vfio_initfn(PCIDevice *pdev)
         return -ENOENT;
     }
 
-    snprintf(path, sizeof(path), "%04x:%02x:%02x.%01x",
-            vdev->host.domain, vdev->host.bus, vdev->host.slot,
-            vdev->host.function);
-
     QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
         if (strcmp(vbasedev_iter->name, vdev->vbasedev.name) == 0) {
-            error_report("vfio: error: device %s is already attached", path);
+            error_report("vfio: error: device %s is already attached",
+                         vdev->vbasedev.name);
             vfio_put_group(group);
             return -EBUSY;
         }
     }
 
-    ret = vfio_get_device(group, path, &vdev->vbasedev);
+    ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev);
     if (ret) {
-        error_report("vfio: failed to get device %s", path);
+        error_report("vfio: failed to get device %s", vdev->vbasedev.name);
         vfio_put_group(group);
         return ret;
     }
@@ -2622,6 +2597,7 @@ static void vfio_instance_init(Object *obj)
 
 static Property vfio_pci_dev_properties[] = {
     DEFINE_PROP_PCI_HOST_DEVADDR("host", VFIOPCIDevice, host),
+    DEFINE_PROP_STRING("sysfsdev", VFIOPCIDevice, vbasedev.sysfsdev),
     DEFINE_PROP_UINT32("x-intx-mmap-timeout-ms", VFIOPCIDevice,
                        intx.mmap_timeout, 1100),
     DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 289b498..99f0642 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -559,38 +559,45 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
 {
     VFIOGroup *group;
     VFIODevice *vbasedev_iter;
-    char path[PATH_MAX], iommu_group_path[PATH_MAX], *group_name;
+    char *tmp, group_path[PATH_MAX], *group_name;
     ssize_t len;
     struct stat st;
     int groupid;
     int ret;
 
-    /* name must be set prior to the call */
-    if (!vbasedev->name || strchr(vbasedev->name, '/')) {
-        return -EINVAL;
-    }
+    /* @sysfsdev takes precedence over @host */
+    if (vbasedev->sysfsdev) {
+        g_free(vbasedev->name);
+        vbasedev->name = g_strdup(basename(vbasedev->sysfsdev));
+    } else {
+        if (!vbasedev->name || strchr(vbasedev->name, '/')) {
+            return -EINVAL;
+        }
 
-    /* Check that the host device exists */
-    g_snprintf(path, sizeof(path), "/sys/bus/platform/devices/%s/",
-               vbasedev->name);
+        vbasedev->sysfsdev = g_strdup_printf("/sys/bus/platform/devices/%s",
+                                             vbasedev->name);
+    }
 
-    if (stat(path, &st) < 0) {
-        error_report("vfio: error: no such host device: %s", path);
+    if (stat(vbasedev->sysfsdev, &st) < 0) {
+        error_report("vfio: error: no such host device: %s",
+                     vbasedev->sysfsdev);
         return -errno;
     }
 
-    g_strlcat(path, "iommu_group", sizeof(path));
-    len = readlink(path, iommu_group_path, sizeof(iommu_group_path));
-    if (len < 0 || len >= sizeof(iommu_group_path)) {
+    tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev);
+    len = readlink(tmp, group_path, sizeof(group_path));
+    g_free(tmp);
+
+    if (len < 0 || len >= sizeof(group_path)) {
         error_report("vfio: error no iommu_group for device");
         return len < 0 ? -errno : -ENAMETOOLONG;
     }
 
-    iommu_group_path[len] = 0;
-    group_name = basename(iommu_group_path);
+    group_path[len] = 0;
 
+    group_name = basename(group_path);
     if (sscanf(group_name, "%d", &groupid) != 1) {
-        error_report("vfio: error reading %s: %m", path);
+        error_report("vfio: error reading %s: %m", group_path);
         return -errno;
     }
 
@@ -602,25 +609,24 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
         return -ENOENT;
     }
 
-    g_snprintf(path, sizeof(path), "%s", vbasedev->name);
-
     QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
         if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) {
-            error_report("vfio: error: device %s is already attached", path);
+            error_report("vfio: error: device %s is already attached",
+                         vbasedev->name);
             vfio_put_group(group);
             return -EBUSY;
         }
     }
-    ret = vfio_get_device(group, path, vbasedev);
+    ret = vfio_get_device(group, vbasedev->name, vbasedev);
     if (ret) {
-        error_report("vfio: failed to get device %s", path);
+        error_report("vfio: failed to get device %s", vbasedev->name);
         vfio_put_group(group);
         return ret;
     }
 
     ret = vfio_populate_device(vbasedev);
     if (ret) {
-        error_report("vfio: failed to populate device %s", path);
+        error_report("vfio: failed to populate device %s", vbasedev->name);
         vfio_put_group(group);
     }
 
@@ -680,7 +686,9 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
     vbasedev->type = VFIO_DEVICE_TYPE_PLATFORM;
     vbasedev->ops = &vfio_platform_ops;
 
-    trace_vfio_platform_realize(vbasedev->name, vdev->compat);
+    trace_vfio_platform_realize(vbasedev->sysfsdev ?
+                                vbasedev->sysfsdev : vbasedev->name,
+                                vdev->compat);
 
     ret = vfio_base_device_init(vbasedev);
     if (ret) {
@@ -702,6 +710,7 @@ static const VMStateDescription vfio_platform_vmstate = {
 
 static Property vfio_platform_dev_properties[] = {
     DEFINE_PROP_STRING("host", VFIOPlatformDevice, vbasedev.name),
+    DEFINE_PROP_STRING("sysfsdev", VFIOPlatformDevice, vbasedev.sysfsdev),
     DEFINE_PROP_BOOL("x-no-mmap", VFIOPlatformDevice, vbasedev.no_mmap, false),
     DEFINE_PROP_UINT32("mmap-timeout-ms", VFIOPlatformDevice,
                        mmap_timeout, 1100),
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index f037f3c..7e00ffc 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -89,6 +89,7 @@ typedef struct VFIODeviceOps VFIODeviceOps;
 typedef struct VFIODevice {
     QLIST_ENTRY(VFIODevice) next;
     struct VFIOGroup *group;
+    char *sysfsdev;
     char *name;
     int fd;
     int type;

Re: [Qemu-devel] [RFC PATCH] vfio: Add sysfsdev property for pci & platform

[Daniel P. Berrange]

On Wed, Jan 20, 2016 at 11:06:55AM -0700, Alex Williamson wrote:
> vfio-pci currently requires a host= parameter, which comes in the
> form of a PCI address in [domain:]<bus:slot.function> notation.  We
> expect to find a matching entry in sysfs for that under
> /sys/bus/pci/devices/.  vfio-platform takes a similar approach, but
> defines the host= parameter to be a string, which can be matched
> directly under /sys/bus/platform/devices/.  On the PCI side, we have
> some interest in using vfio to expose vGPU devices.  These are not
> actual discrete PCI devices, so they don't have a compatible host PCI
> bus address or a device link where QEMU wants to look for it.  There's
> also really no requirement that vfio can only be used to expose
> physical devices, a new vfio bus and iommu driver could expose a
> completely emulated device.  To fit within the vfio framework, it
> would need a kernel struct device and associated IOMMU group, but
> those are easy constraints to manage.
> 
> To support such devices, which would include vGPUs, that honor the
> VFIO PCI programming API, but are not necessarily backed by a unique
> PCI address, add support for specifying any device in sysfs.  The
> vfio API already has support for probing the device type to ensure
> compatibility with either vfio-pci or vfio-platform.
> 
> With this, a vfio-pci device could either be specified as:
> 
> -device vfio-pci,host=02:00.0
> 
> or
> 
> -device vfio-pci,sysfsdev=/sys/devices/pci0000:00/0000:00:1c.0/0000:02:00.0
> 
> or even
> 
> -device vfio-pci,sysfsdev=/sys/bus/pci/devices/0000:02:00.0
> 
> When vGPU support comes along, this might look something more like:
> 
> -device vfio-pci,sysfsdev=/sys/devices/virtual/intel-vgpu/vgpu0@0000:00:02.0
> 
> NB - This is only a made up example path, but it should be noted that
> the device namespace is global for vfio, a virtual device cannot
> overlap with existing namespaces and should not create a name prone to
> conflict, such as a simple instance number.
> 
> The same changes is made for vfio-platform but is only compile tested.
> In both cases, specifying sysfsdev has precedence over the old host
> option.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  hw/vfio/pci.c                 |  130 +++++++++++++++++------------------------
>  hw/vfio/platform.c            |   55 ++++++++++-------
>  include/hw/vfio/vfio-common.h |    1 
>  3 files changed, 86 insertions(+), 100 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 1fb868c..bfe4215 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -880,12 +880,8 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
>      if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
>          /* Since pci handles romfile, just print a message and return */
>          if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.romfile) {
> -            error_printf("Warning : Device at %04x:%02x:%02x.%x "
> -                         "is known to cause system instability issues during "
> -                         "option rom execution. "
> -                         "Proceeding anyway since user specified romfile\n",
> -                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
> -                         vdev->host.function);
> +            error_printf("Warning : Device at %s is known to cause system instability issues during option rom execution. Proceeding anyway since user specified romfile\n",
> +                         vdev->vbasedev.name);

This error message should really be split across lines like
the original code was. Likewise for other cases in this patch


Regards,
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: [Qemu-devel] [RFC PATCH] vfio: Add sysfsdev property for pci & platform

[Alex Williamson]

On Wed, 2016-01-20 at 18:11 +0000, Daniel P. Berrange wrote:
> On Wed, Jan 20, 2016 at 11:06:55AM -0700, Alex Williamson wrote:
> > vfio-pci currently requires a host= parameter, which comes in the
> > form of a PCI address in [domain:]<bus:slot.function> notation.  We
> > expect to find a matching entry in sysfs for that under
> > /sys/bus/pci/devices/.  vfio-platform takes a similar approach, but
> > defines the host= parameter to be a string, which can be matched
> > directly under /sys/bus/platform/devices/.  On the PCI side, we
> > have
> > some interest in using vfio to expose vGPU devices.  These are not
> > actual discrete PCI devices, so they don't have a compatible host
> > PCI
> > bus address or a device link where QEMU wants to look for
> > it.  There's
> > also really no requirement that vfio can only be used to expose
> > physical devices, a new vfio bus and iommu driver could expose a
> > completely emulated device.  To fit within the vfio framework, it
> > would need a kernel struct device and associated IOMMU group, but
> > those are easy constraints to manage.
> > 
> > To support such devices, which would include vGPUs, that honor the
> > VFIO PCI programming API, but are not necessarily backed by a
> > unique
> > PCI address, add support for specifying any device in sysfs.  The
> > vfio API already has support for probing the device type to ensure
> > compatibility with either vfio-pci or vfio-platform.
> > 
> > With this, a vfio-pci device could either be specified as:
> > 
> > -device vfio-pci,host=02:00.0
> > 
> > or
> > 
> > -device vfio-
> > pci,sysfsdev=/sys/devices/pci0000:00/0000:00:1c.0/0000:02:00.0
> > 
> > or even
> > 
> > -device vfio-pci,sysfsdev=/sys/bus/pci/devices/0000:02:00.0
> > 
> > When vGPU support comes along, this might look something more like:
> > 
> > -device vfio-pci,sysfsdev=/sys/devices/virtual/intel-vgpu/vgpu0@000
> > 0:00:02.0
> > 
> > NB - This is only a made up example path, but it should be noted
> > that
> > the device namespace is global for vfio, a virtual device cannot
> > overlap with existing namespaces and should not create a name prone
> > to
> > conflict, such as a simple instance number.
> > 
> > The same changes is made for vfio-platform but is only compile
> > tested.
> > In both cases, specifying sysfsdev has precedence over the old host
> > option.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >  hw/vfio/pci.c                 |  130 +++++++++++++++++----------
> > --------------
> >  hw/vfio/platform.c            |   55 ++++++++++-------
> >  include/hw/vfio/vfio-common.h |    1 
> >  3 files changed, 86 insertions(+), 100 deletions(-)
> > 
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 1fb868c..bfe4215 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -880,12 +880,8 @@ static void vfio_pci_size_rom(VFIOPCIDevice
> > *vdev)
> >      if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
> >          /* Since pci handles romfile, just print a message and
> > return */
> >          if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.romfile) {
> > -            error_printf("Warning : Device at %04x:%02x:%02x.%x "
> > -                         "is known to cause system instability
> > issues during "
> > -                         "option rom execution. "
> > -                         "Proceeding anyway since user specified
> > romfile\n",
> > -                         vdev->host.domain, vdev->host.bus, vdev-
> > >host.slot,
> > -                         vdev->host.function);
> > +            error_printf("Warning : Device at %s is known to cause
> > system instability issues during option rom execution. Proceeding
> > anyway since user specified romfile\n",
> > +                         vdev->vbasedev.name);
> 
> This error message should really be split across lines like
> the original code was. Likewise for other cases in this patch

kernel and QEMU development disagree here and I tend to favor the
kernel's argument that if a user is inclined enough to go grep through
the source to find an error message, it should work.  Breaking printed
error messages at arbitrary lengths to fit an 80 column window makes
that more difficult.  Is there sufficient passion for a hard 80 column
limit on printed strings in QEMU to ignore that useful property?
Thanks,

Alex

Re: [Qemu-devel] [RFC PATCH] vfio: Add sysfsdev property for pci & platform

[P J P]

[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]

+-- On Wed, 20 Jan 2016, Alex Williamson wrote --+
| > > +            error_printf("Warning : Device at %s is known to cause
| > > system instability issues during option rom execution. Proceeding
| > > anyway since user specified romfile\n",
| > > +                         vdev->vbasedev.name);
| > 
| > This error message should really be split across lines like
| > the original code was. Likewise for other cases in this patch
| 
| the source to find an error message, it should work.  Breaking printed
| error messages at arbitrary lengths to fit an 80 column window makes
| that more difficult.  Is there sufficient passion for a hard 80 column
| limit on printed strings in QEMU to ignore that useful property?

  I'd say +1 for breaking it at 80'th column. IMO it helps with readability in 
editor or tools like less(1). As for grep, it can still work with a 
substring/pattern from the error message.

--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [RFC PATCH] vfio: Add sysfsdev property for pci & platform

[Tian, Kevin]

> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Thursday, January 21, 2016 2:07 AM
> 
> vfio-pci currently requires a host= parameter, which comes in the
> form of a PCI address in [domain:]<bus:slot.function> notation.  We
> expect to find a matching entry in sysfs for that under
> /sys/bus/pci/devices/.  vfio-platform takes a similar approach, but
> defines the host= parameter to be a string, which can be matched
> directly under /sys/bus/platform/devices/.  On the PCI side, we have
> some interest in using vfio to expose vGPU devices.  These are not
> actual discrete PCI devices, so they don't have a compatible host PCI
> bus address or a device link where QEMU wants to look for it.  There's
> also really no requirement that vfio can only be used to expose
> physical devices, a new vfio bus and iommu driver could expose a
> completely emulated device.  To fit within the vfio framework, it
> would need a kernel struct device and associated IOMMU group, but
> those are easy constraints to manage.
> 
> To support such devices, which would include vGPUs, that honor the
> VFIO PCI programming API, but are not necessarily backed by a unique
> PCI address, add support for specifying any device in sysfs.  The
> vfio API already has support for probing the device type to ensure
> compatibility with either vfio-pci or vfio-platform.
> 
> With this, a vfio-pci device could either be specified as:
> 
> -device vfio-pci,host=02:00.0
> 
> or
> 
> -device vfio-pci,sysfsdev=/sys/devices/pci0000:00/0000:00:1c.0/0000:02:00.0
> 
> or even
> 
> -device vfio-pci,sysfsdev=/sys/bus/pci/devices/0000:02:00.0
> 
> When vGPU support comes along, this might look something more like:
> 
> -device vfio-pci,sysfsdev=/sys/devices/virtual/intel-vgpu/vgpu0@0000:00:02.0
> 
> NB - This is only a made up example path, but it should be noted that
> the device namespace is global for vfio, a virtual device cannot
> overlap with existing namespaces and should not create a name prone to
> conflict, such as a simple instance number.
> 

Thanks Alex! It's a good improvement to support coming vgpu feature.
Just curious. Does the virtual device name has to include a BDF format
or it can be random strings (e.g. just "vgpu0")? In the latter case, then
overlapping chance would be small.

Thanks
Kevin

Re: [Qemu-devel] [RFC PATCH] vfio: Add sysfsdev property for pci & platform

[Alex Williamson]

On Thu, 2016-01-21 at 07:51 +0000, Tian, Kevin wrote:
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Thursday, January 21, 2016 2:07 AM
> > 
> > vfio-pci currently requires a host= parameter, which comes in the
> > form of a PCI address in [domain:] notation.  We
> > expect to find a matching entry in sysfs for that under
> > /sys/bus/pci/devices/.  vfio-platform takes a similar approach, but
> > defines the host= parameter to be a string, which can be matched
> > directly under /sys/bus/platform/devices/.  On the PCI side, we have
> > some interest in using vfio to expose vGPU devices.  These are not
> > actual discrete PCI devices, so they don't have a compatible host PCI
> > bus address or a device link where QEMU wants to look for it.  There's
> > also really no requirement that vfio can only be used to expose
> > physical devices, a new vfio bus and iommu driver could expose a
> > completely emulated device.  To fit within the vfio framework, it
> > would need a kernel struct device and associated IOMMU group, but
> > those are easy constraints to manage.
> > 
> > To support such devices, which would include vGPUs, that honor the
> > VFIO PCI programming API, but are not necessarily backed by a unique
> > PCI address, add support for specifying any device in sysfs.  The
> > vfio API already has support for probing the device type to ensure
> > compatibility with either vfio-pci or vfio-platform.
> > 
> > With this, a vfio-pci device could either be specified as:
> > 
> > -device vfio-pci,host=02:00.0
> > 
> > or
> > 
> > -device vfio-pci,sysfsdev=/sys/devices/pci0000:00/0000:00:1c.0/0000:02:00.0
> > 
> > or even
> > 
> > -device vfio-pci,sysfsdev=/sys/bus/pci/devices/0000:02:00.0
> > 
> > When vGPU support comes along, this might look something more like:
> > 
> > -device vfio-pci,sysfsdev=/sys/devices/virtual/intel-vgpu/vgpu0@0000:00:02.0
> > 
> > NB - This is only a made up example path, but it should be noted that
> > the device namespace is global for vfio, a virtual device cannot
> > overlap with existing namespaces and should not create a name prone to
> > conflict, such as a simple instance number.
> > 
> 
> Thanks Alex! It's a good improvement to support coming vgpu feature.
> Just curious. Does the virtual device name has to include a BDF format
> or it can be random strings (e.g. just "vgpu0")? In the latter case, then
> overlapping chance would be small.

Hi Kevin,

Yeah, looking at the vfio code again (vfio_device_get_from_name), as
long as the name is unique within the IOMMU group, I think we'll be
fine.  I expect that vGPUs will create singleton groups, so the
namespace constraints I mention above are maybe not a concern.  For
vendors that can support multiple GPUs, each with vGPUs, userspace will
probably want some way to determine the source of a vGPU for load
balancing and locality purpose, but that's better handled through
parent/child device links in sysfs rather than embedding it in the
device name.  Thanks,

Alex

Re: [Qemu-devel] [RFC PATCH] vfio: Add sysfsdev property for pci & platform

[Tian, Kevin]

> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Thursday, January 21, 2016 11:15 PM
> 
> On Thu, 2016-01-21 at 07:51 +0000, Tian, Kevin wrote:
> > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > Sent: Thursday, January 21, 2016 2:07 AM
> > >
> > > vfio-pci currently requires a host= parameter, which comes in the
> > > form of a PCI address in [domain:] notation.  We
> > > expect to find a matching entry in sysfs for that under
> > > /sys/bus/pci/devices/.  vfio-platform takes a similar approach, but
> > > defines the host= parameter to be a string, which can be matched
> > > directly under /sys/bus/platform/devices/.  On the PCI side, we have
> > > some interest in using vfio to expose vGPU devices.  These are not
> > > actual discrete PCI devices, so they don't have a compatible host PCI
> > > bus address or a device link where QEMU wants to look for it.  There's
> > > also really no requirement that vfio can only be used to expose
> > > physical devices, a new vfio bus and iommu driver could expose a
> > > completely emulated device.  To fit within the vfio framework, it
> > > would need a kernel struct device and associated IOMMU group, but
> > > those are easy constraints to manage.
> > >
> > > To support such devices, which would include vGPUs, that honor the
> > > VFIO PCI programming API, but are not necessarily backed by a unique
> > > PCI address, add support for specifying any device in sysfs.  The
> > > vfio API already has support for probing the device type to ensure
> > > compatibility with either vfio-pci or vfio-platform.
> > >
> > > With this, a vfio-pci device could either be specified as:
> > >
> > > -device vfio-pci,host=02:00.0
> > >
> > > or
> > >
> > > -device vfio-pci,sysfsdev=/sys/devices/pci0000:00/0000:00:1c.0/0000:02:00.0
> > >
> > > or even
> > >
> > > -device vfio-pci,sysfsdev=/sys/bus/pci/devices/0000:02:00.0
> > >
> > > When vGPU support comes along, this might look something more like:
> > >
> > > -device vfio-pci,sysfsdev=/sys/devices/virtual/intel-vgpu/vgpu0@0000:00:02.0
> > >
> > > NB - This is only a made up example path, but it should be noted that
> > > the device namespace is global for vfio, a virtual device cannot
> > > overlap with existing namespaces and should not create a name prone to
> > > conflict, such as a simple instance number.
> > >
> >
> > Thanks Alex! It's a good improvement to support coming vgpu feature.
> > Just curious. Does the virtual device name has to include a BDF format
> > or it can be random strings (e.g. just "vgpu0")? In the latter case, then
> > overlapping chance would be small.
> 
> Hi Kevin,
> 
> Yeah, looking at the vfio code again (vfio_device_get_from_name), as
> long as the name is unique within the IOMMU group, I think we'll be
> fine.  I expect that vGPUs will create singleton groups, so the
> namespace constraints I mention above are maybe not a concern.  For
> vendors that can support multiple GPUs, each with vGPUs, userspace will
> probably want some way to determine the source of a vGPU for load
> balancing and locality purpose, but that's better handled through
> parent/child device links in sysfs rather than embedding it in the
> device name.  Thanks,
> 

Agree. It's clear now.

Thanks
Kevin

Re: [Qemu-devel] [RFC PATCH] vfio: Add sysfsdev property for pci & platform

[Eric Auger]

Hi Alex,

I did a try with both legacy cmd line and new one and it works fine for
vfio platform too:
-device vfio-calxeda-xgmac,host="fff51000.ethernet"
-device
vfio-calxeda-xgmac,sysfsdev="/sys/bus/platform/devices/fff51000.ethernet"

Tested-by: Eric Auger <eric.auger@linaro.org>
Reviewed-by: Eric Auger <eric.auger@linaro.org>

just 1 question below.

Best Regards

Eric



On 01/20/2016 07:06 PM, Alex Williamson wrote:
> vfio-pci currently requires a host= parameter, which comes in the
> form of a PCI address in [domain:]<bus:slot.function> notation.  We
> expect to find a matching entry in sysfs for that under
> /sys/bus/pci/devices/.  vfio-platform takes a similar approach, but
> defines the host= parameter to be a string, which can be matched
> directly under /sys/bus/platform/devices/.  On the PCI side, we have
> some interest in using vfio to expose vGPU devices.  These are not
> actual discrete PCI devices, so they don't have a compatible host PCI
> bus address or a device link where QEMU wants to look for it.  There's
> also really no requirement that vfio can only be used to expose
> physical devices, a new vfio bus and iommu driver could expose a
> completely emulated device.  To fit within the vfio framework, it
> would need a kernel struct device and associated IOMMU group, but
> those are easy constraints to manage.
> 
> To support such devices, which would include vGPUs, that honor the
> VFIO PCI programming API, but are not necessarily backed by a unique
> PCI address, add support for specifying any device in sysfs.  The
> vfio API already has support for probing the device type to ensure
> compatibility with either vfio-pci or vfio-platform.
> 
> With this, a vfio-pci device could either be specified as:
> 
> -device vfio-pci,host=02:00.0
> 
> or
> 
> -device vfio-pci,sysfsdev=/sys/devices/pci0000:00/0000:00:1c.0/0000:02:00.0
> 
> or even
> 
> -device vfio-pci,sysfsdev=/sys/bus/pci/devices/0000:02:00.0
> 
> When vGPU support comes along, this might look something more like:
> 
> -device vfio-pci,sysfsdev=/sys/devices/virtual/intel-vgpu/vgpu0@0000:00:02.0
> 
> NB - This is only a made up example path, but it should be noted that
> the device namespace is global for vfio, a virtual device cannot
> overlap with existing namespaces and should not create a name prone to
> conflict, such as a simple instance number.
> 
> The same changes is made for vfio-platform but is only compile tested.
> In both cases, specifying sysfsdev has precedence over the old host
> option.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  hw/vfio/pci.c                 |  130 +++++++++++++++++------------------------
>  hw/vfio/platform.c            |   55 ++++++++++-------
>  include/hw/vfio/vfio-common.h |    1 
>  3 files changed, 86 insertions(+), 100 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 1fb868c..bfe4215 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -880,12 +880,8 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
>      if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
>          /* Since pci handles romfile, just print a message and return */
>          if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.romfile) {
> -            error_printf("Warning : Device at %04x:%02x:%02x.%x "
> -                         "is known to cause system instability issues during "
> -                         "option rom execution. "
> -                         "Proceeding anyway since user specified romfile\n",
> -                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
> -                         vdev->host.function);
> +            error_printf("Warning : Device at %s is known to cause system instability issues during option rom execution. Proceeding anyway since user specified romfile\n",
> +                         vdev->vbasedev.name);
>          }
>          return;
>      }
> @@ -898,9 +894,7 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
>          pwrite(fd, &size, 4, offset) != 4 ||
>          pread(fd, &size, 4, offset) != 4 ||
>          pwrite(fd, &orig, 4, offset) != 4) {
> -        error_report("%s(%04x:%02x:%02x.%x) failed: %m",
> -                     __func__, vdev->host.domain, vdev->host.bus,
> -                     vdev->host.slot, vdev->host.function);
> +        error_report("%s(%s) failed: %m", __func__, vdev->vbasedev.name);
>          return;
>      }
>  
> @@ -912,29 +906,18 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
>  
>      if (vfio_blacklist_opt_rom(vdev)) {
>          if (dev->opts && qemu_opt_get(dev->opts, "rombar")) {
> -            error_printf("Warning : Device at %04x:%02x:%02x.%x "
> -                         "is known to cause system instability issues during "
> -                         "option rom execution. "
> -                         "Proceeding anyway since user specified non zero value for "
> -                         "rombar\n",
> -                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
> -                         vdev->host.function);
> +            error_printf("Warning : Device at %s is known to cause system instability issues during option rom execution. Proceeding anyway since user specified non zero value for rombar\n",
> +                         vdev->vbasedev.name);
>          } else {
> -            error_printf("Warning : Rom loading for device at "
> -                         "%04x:%02x:%02x.%x has been disabled due to "
> -                         "system instability issues. "
> -                         "Specify rombar=1 or romfile to force\n",
> -                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
> -                         vdev->host.function);
> +            error_printf("Warning : Rom loading for device at %s has been disabled due to system instability issues. Specify rombar=1 or romfile to force\n",
> +                         vdev->vbasedev.name);
>              return;
>          }
>      }
>  
>      trace_vfio_pci_size_rom(vdev->vbasedev.name, size);
>  
> -    snprintf(name, sizeof(name), "vfio[%04x:%02x:%02x.%x].rom",
> -             vdev->host.domain, vdev->host.bus, vdev->host.slot,
> -             vdev->host.function);
> +    snprintf(name, sizeof(name), "vfio[%s].rom", vdev->vbasedev.name);
>  
>      memory_region_init_io(&vdev->pdev.rom, OBJECT(vdev),
>                            &vfio_rom_ops, vdev, name, size);
> @@ -1048,9 +1031,8 @@ uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len)
>          ret = pread(vdev->vbasedev.fd, &phys_val, len,
>                      vdev->config_offset + addr);
>          if (ret != len) {
> -            error_report("%s(%04x:%02x:%02x.%x, 0x%x, 0x%x) failed: %m",
> -                         __func__, vdev->host.domain, vdev->host.bus,
> -                         vdev->host.slot, vdev->host.function, addr, len);
> +            error_report("%s(%s, 0x%x, 0x%x) failed: %m",
> +                         __func__, vdev->vbasedev.name, addr, len);
>              return -errno;
>          }
>          phys_val = le32_to_cpu(phys_val);
> @@ -1074,9 +1056,8 @@ void vfio_pci_write_config(PCIDevice *pdev,
>      /* Write everything to VFIO, let it filter out what we can't write */
>      if (pwrite(vdev->vbasedev.fd, &val_le, len, vdev->config_offset + addr)
>                  != len) {
> -        error_report("%s(%04x:%02x:%02x.%x, 0x%x, 0x%x, 0x%x) failed: %m",
> -                     __func__, vdev->host.domain, vdev->host.bus,
> -                     vdev->host.slot, vdev->host.function, addr, val, len);
> +        error_report("%s(%s, 0x%x, 0x%x, 0x%x) failed: %m",
> +                     __func__, vdev->vbasedev.name, addr, val, len);
>      }
>  
>      /* MSI/MSI-X Enabling/Disabling */
> @@ -1347,9 +1328,7 @@ static void vfio_map_bar(VFIOPCIDevice *vdev, int nr)
>          return;
>      }
>  
> -    snprintf(name, sizeof(name), "VFIO %04x:%02x:%02x.%x BAR %d",
> -             vdev->host.domain, vdev->host.bus, vdev->host.slot,
> -             vdev->host.function, nr);
> +    snprintf(name, sizeof(name), "VFIO %s BAR %d", vdev->vbasedev.name, nr);
>  
>      /* Determine what type of BAR this is for registration */
>      ret = pread(vdev->vbasedev.fd, &pci_bar, sizeof(pci_bar),
> @@ -1719,9 +1698,8 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>      }
>  
>      if (ret < 0) {
> -        error_report("vfio: %04x:%02x:%02x.%x Error adding PCI capability "
> -                     "0x%x[0x%x]@0x%x: %d", vdev->host.domain,
> -                     vdev->host.bus, vdev->host.slot, vdev->host.function,
> +        error_report("vfio: %s Error adding PCI capability "
> +                     "0x%x[0x%x]@0x%x: %d", vdev->vbasedev.name,
>                       cap_id, size, pos, ret);
>          return ret;
>      }
> @@ -1783,11 +1761,14 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev)
>      vfio_intx_enable(vdev);
>  }
>  
> -static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
> -                                PCIHostDeviceAddress *host2)
> +static bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name)
>  {
> -    return (host1->domain == host2->domain && host1->bus == host2->bus &&
> -            host1->slot == host2->slot && host1->function == host2->function);
> +    char tmp[13];
> +
> +    sprintf(tmp, "%04x:%02x:%02x.%1x", addr->domain,
> +            addr->bus, addr->slot, addr->function);
> +
> +    return (strcmp(tmp, name) == 0);
>  }
>  
>  static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
> @@ -1812,9 +1793,8 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
>      if (ret && errno != ENOSPC) {
>          ret = -errno;
>          if (!vdev->has_pm_reset) {
> -            error_report("vfio: Cannot reset device %04x:%02x:%02x.%x, "
> -                         "no available reset mechanism.", vdev->host.domain,
> -                         vdev->host.bus, vdev->host.slot, vdev->host.function);
> +            error_report("vfio: Cannot reset device %s, "
> +                         "no available reset mechanism.", vdev->vbasedev.name);
>          }
>          goto out_single;
>      }
> @@ -1847,7 +1827,7 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
>          trace_vfio_pci_hot_reset_dep_devices(host.domain,
>                  host.bus, host.slot, host.function, devices[i].group_id);
>  
> -        if (vfio_pci_host_match(&host, &vdev->host)) {
> +        if (vfio_pci_host_match(&host, vdev->vbasedev.name)) {
>              continue;
>          }
>  
> @@ -1873,7 +1853,7 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
>                  continue;
>              }
>              tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> -            if (vfio_pci_host_match(&host, &tmp->host)) {
> +            if (vfio_pci_host_match(&host, tmp->vbasedev.name)) {
>                  if (single) {
>                      ret = -EINVAL;
>                      goto out_single;
> @@ -1935,7 +1915,7 @@ out:
>          host.slot = PCI_SLOT(devices[i].devfn);
>          host.function = PCI_FUNC(devices[i].devfn);
>  
> -        if (vfio_pci_host_match(&host, &vdev->host)) {
> +        if (vfio_pci_host_match(&host, vdev->vbasedev.name)) {
>              continue;
>          }
>  
> @@ -1954,7 +1934,7 @@ out:
>                  continue;
>              }
>              tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> -            if (vfio_pci_host_match(&host, &tmp->host)) {
> +            if (vfio_pci_host_match(&host, tmp->vbasedev.name)) {
>                  vfio_pci_post_reset(tmp);
>                  break;
>              }
> @@ -2160,10 +2140,7 @@ static void vfio_err_notifier_handler(void *opaque)
>       * guest to contain the error.
>       */
>  
> -    error_report("%s(%04x:%02x:%02x.%x) Unrecoverable error detected.  "
> -                 "Please collect any data possible and then kill the guest",
> -                 __func__, vdev->host.domain, vdev->host.bus,
> -                 vdev->host.slot, vdev->host.function);
> +    error_report("%s(%s) Unrecoverable error detected. Please collect any data possible and then kill the guest", __func__, vdev->vbasedev.name);
>  
>      vm_stop(RUN_STATE_INTERNAL_ERROR);
>  }
> @@ -2344,42 +2321,43 @@ static int vfio_initfn(PCIDevice *pdev)
>      VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>      VFIODevice *vbasedev_iter;
>      VFIOGroup *group;
> -    char path[PATH_MAX], iommu_group_path[PATH_MAX], *group_name;
> +    char *tmp, group_path[PATH_MAX], *group_name;
>      ssize_t len;
>      struct stat st;
>      int groupid;
>      int ret;
>  
> -    /* Check that the host device exists */
> -    snprintf(path, sizeof(path),
> -             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/",
> -             vdev->host.domain, vdev->host.bus, vdev->host.slot,
> -             vdev->host.function);
> -    if (stat(path, &st) < 0) {
> -        error_report("vfio: error: no such host device: %s", path);
> +    if (!vdev->vbasedev.sysfsdev) {
> +        vdev->vbasedev.sysfsdev =
> +            g_strdup_printf("/sys/bus/pci/devices/%04x:%02x:%02x.%01x",
> +                            vdev->host.domain, vdev->host.bus,
> +                            vdev->host.slot, vdev->host.function);
> +    }
> +
> +    if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
> +        error_report("vfio: error: no such host device: %s",
> +                     vdev->vbasedev.sysfsdev);
>          return -errno;
>      }
>  
> +    vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
>      vdev->vbasedev.ops = &vfio_pci_ops;
> -
>      vdev->vbasedev.type = VFIO_DEVICE_TYPE_PCI;
> -    vdev->vbasedev.name = g_strdup_printf("%04x:%02x:%02x.%01x",
> -                                          vdev->host.domain, vdev->host.bus,
> -                                          vdev->host.slot, vdev->host.function);
>  
> -    strncat(path, "iommu_group", sizeof(path) - strlen(path) - 1);
> +    tmp = g_strdup_printf("%s/iommu_group", vdev->vbasedev.sysfsdev);
> +    len = readlink(tmp, group_path, sizeof(group_path));
> +    g_free(tmp);
>  
> -    len = readlink(path, iommu_group_path, sizeof(path));
> -    if (len <= 0 || len >= sizeof(path)) {
> +    if (len <= 0 || len >= sizeof(group_path)) {
>          error_report("vfio: error no iommu_group for device");
>          return len < 0 ? -errno : -ENAMETOOLONG;
>      }
>  
> -    iommu_group_path[len] = 0;
> -    group_name = basename(iommu_group_path);
> +    group_path[len] = 0;
>  
> +    group_name = basename(group_path);
>      if (sscanf(group_name, "%d", &groupid) != 1) {
> -        error_report("vfio: error reading %s: %m", path);
> +        error_report("vfio: error reading %s: %m", group_path);
>          return -errno;
>      }
>  
> @@ -2391,21 +2369,18 @@ static int vfio_initfn(PCIDevice *pdev)
>          return -ENOENT;
>      }
>  
> -    snprintf(path, sizeof(path), "%04x:%02x:%02x.%01x",
> -            vdev->host.domain, vdev->host.bus, vdev->host.slot,
> -            vdev->host.function);
> -
>      QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
>          if (strcmp(vbasedev_iter->name, vdev->vbasedev.name) == 0) {
> -            error_report("vfio: error: device %s is already attached", path);
> +            error_report("vfio: error: device %s is already attached",
> +                         vdev->vbasedev.name);
>              vfio_put_group(group);
>              return -EBUSY;
>          }
>      }
>  
> -    ret = vfio_get_device(group, path, &vdev->vbasedev);
> +    ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev);
>      if (ret) {
> -        error_report("vfio: failed to get device %s", path);
> +        error_report("vfio: failed to get device %s", vdev->vbasedev.name);
>          vfio_put_group(group);
>          return ret;
>      }
> @@ -2622,6 +2597,7 @@ static void vfio_instance_init(Object *obj)
>  
>  static Property vfio_pci_dev_properties[] = {
>      DEFINE_PROP_PCI_HOST_DEVADDR("host", VFIOPCIDevice, host),
> +    DEFINE_PROP_STRING("sysfsdev", VFIOPCIDevice, vbasedev.sysfsdev),
>      DEFINE_PROP_UINT32("x-intx-mmap-timeout-ms", VFIOPCIDevice,
>                         intx.mmap_timeout, 1100),
>      DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index 289b498..99f0642 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -559,38 +559,45 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
>  {
>      VFIOGroup *group;
>      VFIODevice *vbasedev_iter;
> -    char path[PATH_MAX], iommu_group_path[PATH_MAX], *group_name;
> +    char *tmp, group_path[PATH_MAX], *group_name;
>      ssize_t len;
>      struct stat st;
>      int groupid;
>      int ret;
>  
> -    /* name must be set prior to the call */
> -    if (!vbasedev->name || strchr(vbasedev->name, '/')) {
> -        return -EINVAL;
> -    }
> +    /* @sysfsdev takes precedence over @host */
> +    if (vbasedev->sysfsdev) {
> +        g_free(vbasedev->name);
> +        vbasedev->name = g_strdup(basename(vbasedev->sysfsdev));
do we need the g_strdup here?
> +    } else {
> +        if (!vbasedev->name || strchr(vbasedev->name, '/')) {
> +            return -EINVAL;
> +        }
>  
> -    /* Check that the host device exists */
> -    g_snprintf(path, sizeof(path), "/sys/bus/platform/devices/%s/",
> -               vbasedev->name);
> +        vbasedev->sysfsdev = g_strdup_printf("/sys/bus/platform/devices/%s",
> +                                             vbasedev->name);
> +    }
>  
> -    if (stat(path, &st) < 0) {
> -        error_report("vfio: error: no such host device: %s", path);
> +    if (stat(vbasedev->sysfsdev, &st) < 0) {
> +        error_report("vfio: error: no such host device: %s",
> +                     vbasedev->sysfsdev);
>          return -errno;
>      }
>  
> -    g_strlcat(path, "iommu_group", sizeof(path));
> -    len = readlink(path, iommu_group_path, sizeof(iommu_group_path));
> -    if (len < 0 || len >= sizeof(iommu_group_path)) {
> +    tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev);
> +    len = readlink(tmp, group_path, sizeof(group_path));
> +    g_free(tmp);
> +
> +    if (len < 0 || len >= sizeof(group_path)) {
>          error_report("vfio: error no iommu_group for device");
>          return len < 0 ? -errno : -ENAMETOOLONG;
>      }
>  
> -    iommu_group_path[len] = 0;
> -    group_name = basename(iommu_group_path);
> +    group_path[len] = 0;
>  
> +    group_name = basename(group_path);
>      if (sscanf(group_name, "%d", &groupid) != 1) {
> -        error_report("vfio: error reading %s: %m", path);
> +        error_report("vfio: error reading %s: %m", group_path);
>          return -errno;
>      }
>  
> @@ -602,25 +609,24 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
>          return -ENOENT;
>      }
>  
> -    g_snprintf(path, sizeof(path), "%s", vbasedev->name);
> -
>      QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
>          if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) {
> -            error_report("vfio: error: device %s is already attached", path);
> +            error_report("vfio: error: device %s is already attached",
> +                         vbasedev->name);
>              vfio_put_group(group);
>              return -EBUSY;
>          }
>      }
> -    ret = vfio_get_device(group, path, vbasedev);
> +    ret = vfio_get_device(group, vbasedev->name, vbasedev);
>      if (ret) {
> -        error_report("vfio: failed to get device %s", path);
> +        error_report("vfio: failed to get device %s", vbasedev->name);
>          vfio_put_group(group);
>          return ret;
>      }
>  
>      ret = vfio_populate_device(vbasedev);
>      if (ret) {
> -        error_report("vfio: failed to populate device %s", path);
> +        error_report("vfio: failed to populate device %s", vbasedev->name);
>          vfio_put_group(group);
>      }
>  
> @@ -680,7 +686,9 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
>      vbasedev->type = VFIO_DEVICE_TYPE_PLATFORM;
>      vbasedev->ops = &vfio_platform_ops;
>  
> -    trace_vfio_platform_realize(vbasedev->name, vdev->compat);
> +    trace_vfio_platform_realize(vbasedev->sysfsdev ?
> +                                vbasedev->sysfsdev : vbasedev->name,
> +                                vdev->compat);
>  
>      ret = vfio_base_device_init(vbasedev);
>      if (ret) {
> @@ -702,6 +710,7 @@ static const VMStateDescription vfio_platform_vmstate = {
>  
>  static Property vfio_platform_dev_properties[] = {
>      DEFINE_PROP_STRING("host", VFIOPlatformDevice, vbasedev.name),
> +    DEFINE_PROP_STRING("sysfsdev", VFIOPlatformDevice, vbasedev.sysfsdev),
>      DEFINE_PROP_BOOL("x-no-mmap", VFIOPlatformDevice, vbasedev.no_mmap, false),
>      DEFINE_PROP_UINT32("mmap-timeout-ms", VFIOPlatformDevice,
>                         mmap_timeout, 1100),
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index f037f3c..7e00ffc 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -89,6 +89,7 @@ typedef struct VFIODeviceOps VFIODeviceOps;
>  typedef struct VFIODevice {
>      QLIST_ENTRY(VFIODevice) next;
>      struct VFIOGroup *group;
> +    char *sysfsdev;
>      char *name;
>      int fd;
>      int type;
> 

Re: [Qemu-devel] [RFC PATCH] vfio: Add sysfsdev property for pci & platform

[Alex Williamson]

On Tue, 2016-01-26 at 16:03 +0100, Eric Auger wrote:
> 
> Hi Alex,
> 
> I did a try with both legacy cmd line and new one and it works fine for
> vfio platform too:
> -device vfio-calxeda-xgmac,host="fff51000.ethernet"
> -device
> vfio-calxeda-xgmac,sysfsdev="/sys/bus/platform/devices/fff51000.ethernet"
> 
> Tested-by: Eric Auger <eric.auger@linaro.org>
> Reviewed-by: Eric Auger <eric.auger@linaro.org>

Thanks!

> just 1 question below.
...
> > diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> > index 289b498..99f0642 100644
> > --- a/hw/vfio/platform.c
> > +++ b/hw/vfio/platform.c
> > @@ -559,38 +559,45 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
> >  {
> >      VFIOGroup *group;
> >      VFIODevice *vbasedev_iter;
> > -    char path[PATH_MAX], iommu_group_path[PATH_MAX], *group_name;
> > +    char *tmp, group_path[PATH_MAX], *group_name;
> >      ssize_t len;
> >      struct stat st;
> >      int groupid;
> >      int ret;
> >  
> > -    /* name must be set prior to the call */
> > -    if (!vbasedev->name || strchr(vbasedev->name, '/')) {
> > -        return -EINVAL;
> > -    }
> > +    /* @sysfsdev takes precedence over @host */
> > +    if (vbasedev->sysfsdev) {
> > +        g_free(vbasedev->name);
> > +        vbasedev->name = g_strdup(basename(vbasedev->sysfsdev));
> do we need the g_strdup here?


Versus pointing ->name to the offset within sysfsdev where the name
starts?  My concern was that both @sysfsdev and @name are allocated via
device properties and presumably automatically collected when the
device is destroyed.  If I set one within the buffer of another, I'd
likely get a double free.  So creating a new string buffer seemed like
the safest approach.  Agree?  Thanks,

Alex

Re: [Qemu-devel] [RFC PATCH] vfio: Add sysfsdev property for pci & platform

[Eric Auger]

On 01/26/2016 06:08 PM, Alex Williamson wrote:
> On Tue, 2016-01-26 at 16:03 +0100, Eric Auger wrote:
>>
>> Hi Alex,
>>
>> I did a try with both legacy cmd line and new one and it works fine for
>> vfio platform too:
>> -device vfio-calxeda-xgmac,host="fff51000.ethernet"
>> -device
>> vfio-calxeda-xgmac,sysfsdev="/sys/bus/platform/devices/fff51000.ethernet"
>>
>> Tested-by: Eric Auger <eric.auger@linaro.org>
>> Reviewed-by: Eric Auger <eric.auger@linaro.org>
> 
> Thanks!
> 
>> just 1 question below.
> ...
>>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
>>> index 289b498..99f0642 100644
>>> --- a/hw/vfio/platform.c
>>> +++ b/hw/vfio/platform.c
>>> @@ -559,38 +559,45 @@ static int vfio_base_device_init(VFIODevice *vbasedev)
>>>  {
>>>      VFIOGroup *group;
>>>      VFIODevice *vbasedev_iter;
>>> -    char path[PATH_MAX], iommu_group_path[PATH_MAX], *group_name;
>>> +    char *tmp, group_path[PATH_MAX], *group_name;
>>>      ssize_t len;
>>>      struct stat st;
>>>      int groupid;
>>>      int ret;
>>>  
>>> -    /* name must be set prior to the call */
>>> -    if (!vbasedev->name || strchr(vbasedev->name, '/')) {
>>> -        return -EINVAL;
>>> -    }
>>> +    /* @sysfsdev takes precedence over @host */
>>> +    if (vbasedev->sysfsdev) {
>>> +        g_free(vbasedev->name);
>>> +        vbasedev->name = g_strdup(basename(vbasedev->sysfsdev));
>> do we need the g_strdup here?
> 
> 
> Versus pointing ->name to the offset within sysfsdev where the name
> starts?  My concern was that both @sysfsdev and @name are allocated via
> device properties and presumably automatically collected when the
> device is destroyed.  If I set one within the buffer of another, I'd
> likely get a double free.  So creating a new string buffer seemed like
> the safest approach.  Agree?  Thanks,
Yes I agree

Best Regards

Eric
> 
> Alex
>