Re: [RFC PATCH v4 1/3] vfio: platform: add device properties skeleton and user API

On Wed, Sep 9, 2015 at 10:48 PM, Alex Williamson <alex.williamson@redhat.com

wrote:

On Wed, 2015-09-09 at 11:17 +0200, Baptiste Reynal wrote:

quoted

From: Antonios Motakis <redacted>

This patch introduces an API that allows to return device properties (OF
or ACPI) of a device bound to the vfio-platform/vfio-amba driver and the
skeleton of the implementation for VFIO_PLATFORM. Information about any
device node bound by VFIO_PLATFORM should be queried via the introduced
ioctl VFIO_DEVICE_GET_DEV_PROPERTY.

The user needs to know the name and the data type of the property he is
accessing.

Signed-off-by: Antonios Motakis <redacted>
Signed-off-by: Baptiste Reynal <redacted>

---
v3 -> v4:
 - added flags placeholder in vfio_dev_properties
 - ioctl returns -E2BIG if the buffer is too small
 - details VFIO_DEVICE_GET_DEV_PROPERTY documentation
---
 drivers/vfio/platform/Makefile                |  3 +-
 drivers/vfio/platform/properties.c            | 77

+++++++++++++++++++++++++++

quoted

 drivers/vfio/platform/vfio_platform_common.c  | 35 ++++++++++++
 drivers/vfio/platform/vfio_platform_private.h |  7 +++
 include/uapi/linux/vfio.h                     | 31 +++++++++++
 5 files changed, 152 insertions(+), 1 deletion(-)
 create mode 100644 drivers/vfio/platform/properties.c

diff --git a/drivers/vfio/platform/Makefile

b/drivers/vfio/platform/Makefile

quoted

index 9ce8afe..37cf5ed 100644

--- a/drivers/vfio/platform/Makefile
+++ b/drivers/vfio/platform/Makefile

@@ -1,5 +1,6 @@

-vfio-platform-y := vfio_platform.o vfio_platform_common.o

vfio_platform_irq.o

quoted

+vfio-platform-y := vfio_platform.o vfio_platform_common.o

vfio_platform_irq.o \

quoted

+                properties.o

 obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
 obj-$(CONFIG_VFIO_PLATFORM) += reset/

diff --git a/drivers/vfio/platform/properties.c

b/drivers/vfio/platform/properties.c

quoted

new file mode 100644
index 0000000..98754c2

--- /dev/null
+++ b/drivers/vfio/platform/properties.c

@@ -0,0 +1,77 @@
+/*
+ * Copyright (C) 2015 - Virtual Open Systems
+ * Authors: Antonios Motakis <a.motakis@virtualopensystems.com>
+ *          Baptiste Reynal <b.reynal@virtualopensystems.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/slab.h>
+#include <linux/vfio.h>
+#include <linux/property.h>
+#include "vfio_platform_private.h"
+
+static int dev_property_get_strings(struct device *dev, uint32_t *flags,
+                                 char *name, unsigned *lenp,
+                                 void __user *datap, unsigned long

datasz)

quoted

+{
+     return -EINVAL;
+}
+
+static int dev_property_get_uint(struct device *dev, uint32_t *flags,
+                              char *name, uint32_t type, unsigned *lenp,
+                              void __user *datap, unsigned long datasz)
+{
+     return -EINVAL;
+}
+
+int vfio_platform_dev_properties(struct device *dev, uint32_t *flags,
+                              uint32_t type, unsigned *lenp,
+                              void __user *datap, unsigned long datasz)
+{
+     char *name;
+     long namesz;
+     int ret;
+
+     namesz = strnlen_user(datap, datasz);
+     if (!namesz)
+             return -EFAULT;
+     if (namesz > datasz)
+             return -EINVAL;
+
+     name = kzalloc(namesz, GFP_KERNEL);

What prevents the user from passing an arbitrarily large string here?

Nothing, should I put an arbitrary limit ?

quoted

+     if (!name)
+             return -ENOMEM;
+     if (strncpy_from_user(name, datap, namesz) <= 0) {
+             kfree(name);
+             return -EFAULT;
+     }
+
+     switch (type) {
+     case VFIO_DEV_PROPERTY_TYPE_STRINGS:
+             ret = dev_property_get_strings(dev, flags, name, lenp,
+                             datap, datasz);
+             break;
+
+     case VFIO_DEV_PROPERTY_TYPE_U64:
+     case VFIO_DEV_PROPERTY_TYPE_U32:
+     case VFIO_DEV_PROPERTY_TYPE_U16:
+     case VFIO_DEV_PROPERTY_TYPE_U8:
+             ret = dev_property_get_uint(dev, flags, name, type, lenp,
+                                         datap, datasz);
+             break;
+
+     default:
+             ret = -EINVAL;
+     }
+
+     kfree(name);
+     return ret;
+}

diff --git a/drivers/vfio/platform/vfio_platform_common.c

b/drivers/vfio/platform/vfio_platform_common.c

quoted

index e43efb5..44ba22c 100644

--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c

@@ -20,6 +20,7 @@
 #include <linux/types.h>
 #include <linux/uaccess.h>
 #include <linux/vfio.h>
+#include <linux/property.h>

 #include "vfio_platform_private.h"

@@ -302,6 +303,34 @@ static long vfio_platform_ioctl(void *device_data,
                      return vdev->reset(vdev);
              else
                      return -EINVAL;
+     } else if (cmd == VFIO_DEVICE_GET_DEV_PROPERTY) {
+             struct vfio_dev_property info;
+             void __user *datap;
+             unsigned long datasz;
+             int ret;
+
+             if (!vdev->dev)
+                     return -EINVAL;
+
+             minsz = offsetofend(struct vfio_dev_property, length);
+
+             if (copy_from_user(&info, (void __user *)arg, minsz))
+                     return -EFAULT;
+
+             if (info.argsz < minsz)
+                     return -EINVAL;
+
+             datap = (void __user *) arg + minsz;
+             datasz = info.argsz - minsz;
+
+             ret = vfio_platform_dev_properties(vdev->dev, &info.flags,

Why the address of flags, I don't see anyone modifying it and there's
nothing defined in the ioctl description for flags being modified on
return.  In fact, there are no flags defined yet, it's just a
future-proofing mechanism of the ioctl, so why propagate it at all right
now?

I tried to use them to retrieve some informations about the property, then
forgot to remove the propagation. Will be removed on next release.

quoted

+                             info.type, &info.length, datap, datasz);
+
+             if (copy_to_user((void __user *)arg, &info, minsz))
+                     ret = -EFAULT;
+
+             return ret;
+
      }

      return -ENOTTY;

@@ -553,6 +582,12 @@ int vfio_platform_probe_common(struct

vfio_platform_device *vdev,

quoted

      vfio_platform_get_reset(vdev, dev);

+     /* add device properties flag */
+     if (device_property_present(dev, "name")) {
+             vdev->dev = dev;

It seems precarious to leave ->dev dangling in the else case.  This is a
future bug waiting to happen.

Ok, wil be fixed on next release.

quoted

+             vdev->flags |= VFIO_DEVICE_FLAGS_DEV_PROPERTIES;
+     }
+
      mutex_init(&vdev->igate);

      return 0;

diff --git a/drivers/vfio/platform/vfio_platform_private.h

b/drivers/vfio/platform/vfio_platform_private.h

quoted

index 1c9b3d5..c75b089 100644

--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h

@@ -56,6 +56,7 @@ struct vfio_platform_device {
      u32                             num_irqs;
      int                             refcnt;
      struct mutex                    igate;
+     struct device                   *dev;

      /*
       * These fields should be filled by the bus specific binder

@@ -89,4 +90,10 @@ extern int vfio_platform_set_irqs_ioctl(struct

vfio_platform_device *vdev,

quoted

                                      unsigned start, unsigned count,
                                      void *data);

+/* device properties support in properties.c */
+extern int vfio_platform_dev_properties(struct device *dev, uint32_t

*flags,

quoted

+                                     uint32_t type, unsigned *lenp,
+                                     void __user *datap,
+                                     unsigned long datasz);
+
 #endif /* VFIO_PLATFORM_PRIVATE_H */

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 9fd7b5d..a1a0eaa 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h

@@ -164,12 +164,43 @@ struct vfio_device_info {
 #define VFIO_DEVICE_FLAGS_PCI        (1 << 1)        /* vfio-pci device

*/

quoted

 #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)  /* vfio-platform device */
 #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)     /* vfio-amba device */
+#define VFIO_DEVICE_FLAGS_DEV_PROPERTIES (1 << 4) /* Device properties

*/

quoted

      __u32   num_regions;    /* Max region index + 1 */
      __u32   num_irqs;       /* Max IRQ index + 1 */
 };
 #define VFIO_DEVICE_GET_INFO         _IO(VFIO_TYPE, VFIO_BASE + 7)

 /**
+ * VFIO_DEVICE_GET_DEV_PROPERTY - _IOR(VFIO_TYPE, VFIO_BASE + 17,
+ *                                           struct vfio_devtree_info)
+ *
+ * Retrieve a device property, e.g. from a HW description (device tree

or ACPI)

quoted

+ * if available.
+ * Caller will initialize data[] with a single string with the requested
+ * devicetree property name, and type depending on whether an array of

strings

quoted

+ * or an array of u32 values is expected. On success, data[] will be

filled

quoted

+ * with the requested information, either as an array of u32, or with a

list

quoted

+ * of strings separated by the NULL terminating character.
+ * Return: 0 on success, -errno on failure.
+ * Errors:
+ *   E2BIG: the property is too big to fit in the data[] buffer (the

required

quoted

+ *          size is then written into the length field).

I don't know which is more correct, but the
VFIO_DEVICE_GET_PCI_HOT_RESET_INFO uses ENOSPC when the buffer size is
too small.  If there's no compelling reason to be different, let's use
it here too.

I found E2BIG in others ioctls (KVM_GET_MSR_INDEX_LIST), but let's use
ENOSPC.

quoted

+ */
+struct vfio_dev_property {
+     __u32   argsz;
+     __u32   flags;  /* Placeholder for future extension */
+     __u32   type;
+#define VFIO_DEV_PROPERTY_TYPE_STRINGS       0
+#define VFIO_DEV_PROPERTY_TYPE_U8    1
+#define VFIO_DEV_PROPERTY_TYPE_U16   2
+#define VFIO_DEV_PROPERTY_TYPE_U32   3
+#define VFIO_DEV_PROPERTY_TYPE_U64   4
+     __u32   length; /* Size of data[] */
+     __u8    data[];
+};
+#define VFIO_DEVICE_GET_DEV_PROPERTY _IO(VFIO_TYPE, VFIO_BASE + 17)
+
+/**
  * VFIO_DEVICE_GET_REGION_INFO - _IOWR(VFIO_TYPE, VFIO_BASE + 8,
  *                                  struct vfio_region_info)
  *