[RFC PATCH v4 0/3] vfio: platform: return device properties for a platform device

[RFC PATCH v4 0/3] vfio: platform: return device properties for a platform device

[Baptiste Reynal]

This RFC's intention is to show what an interface to access device
properties for the VFIO platform driver can look like. These properties
can be retrieved from the device tree node describing the device, or ACPI 
properties.

If a device property corresponding to a platform device bound to VFIO PLATFORM
or VFIO AMBA is available, this patch series will allow the user to query
for this information. This can be useful for userspace drivers to automatically
query parameters related to the device.

Specifically for QEMU, reading the "compatible" property of the device tree
node could be of use to find out what device is being assigned to the guest and
handle appropriately a wider range of devices in the future, and to generate an
appropriate device tree for the guest.

Older versions of this series were specifically targeted at device tree
properties. The v3 has been reworked on top of Rafael J. Wysocki's
uniform device properties API for device tree and ACPI devices. This will allow
us to use the API in the future with devices described via ACPI.

The API to get the list of available properties and the device tree full_name
have been removed. These probably don't serve an useful purpose, as the user
of this API need to know anyway what properties are specific to the device he
wants to access with VFIO. If we decide to reintroduce the list of properties
in the future, the generic device properties API in the kernel will have to
be extended accordingly.

A kernel with this series and all the dependencies applied can be pulled from
branch vfio-device-properties-v4 from the repository:
https://git.virtualopensystems.com/dev/linux.git

Changes since v3:
 - Rebased on top on v4.2
 - Rework VFIO_DEVICE_GET_DEV_PROPERTY ioctl
 - Rework code according to Eric Auger's comments
Changes since v2:
 - Reworked on top of Rafael J. Wysocki's uniform device properties API for
   device tree and ACPI
 - Support for u64 array properties
 - Removed API to get list of available properties and device tree full_name
Changes since v1:
 - Updated for VFIO platform patch series v8:
   VFIO AMBA devices now supported in addition to VFIO PLATFORM devices
 - Refactored and cleaned up the code

Antonios Motakis (3):
  vfio: platform: add device properties skeleton and user API
  vfio: platform: access device property as a list of strings
  vfio: platform: return device properties as arrays of unsigned
    integers

 drivers/vfio/platform/Makefile                |   3 +-
 drivers/vfio/platform/properties.c            | 178 ++++++++++++++++++++++++++
 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, 253 insertions(+), 1 deletion(-)
 create mode 100644 drivers/vfio/platform/properties.c

-- 
2.5.1

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

[Baptiste Reynal]

From: Antonios Motakis <a.motakis@virtualopensystems.com>

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 <a.motakis@virtualopensystems.com>
Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>

---
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 +++++++++++++++++++++++++++
 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
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
+vfio-platform-y := vfio_platform.o vfio_platform_common.o vfio_platform_irq.o \
+		   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
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)
+{
+	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);
+	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
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,
+				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,
 
 	vfio_platform_get_reset(vdev, dev);
 
+	/* add device properties flag */
+	if (device_property_present(dev, "name")) {
+		vdev->dev = dev;
+		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
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,
 					unsigned start, unsigned count,
 					void *data);
 
+/* device properties support in properties.c */
+extern int vfio_platform_dev_properties(struct device *dev, uint32_t *flags,
+					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 */
 #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 */
 	__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)
+ * 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
+ * or an array of u32 values is expected. On success, data[] will be filled
+ * with the requested information, either as an array of u32, or with a list
+ * 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
+ *          size is then written into the length field).
+ */
+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)
  *
-- 
2.5.1

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

[Alex Williamson]

On Wed, 2015-09-09 at 11:17 +0200, Baptiste Reynal wrote:
> From: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> 
> 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 <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> Signed-off-by: Baptiste Reynal <b.reynal-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> 
> ---
> 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 +++++++++++++++++++++++++++
>  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
> 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
> +vfio-platform-y := vfio_platform.o vfio_platform_common.o vfio_platform_irq.o \
> +		   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
> 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-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> + *          Baptiste Reynal <b.reynal-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> + *
> + * 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)
> +{
> +	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?

> +	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
> 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?

> +				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,
>  
>  	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.

> +		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
> 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,
>  					unsigned start, unsigned count,
>  					void *data);
>  
> +/* device properties support in properties.c */
> +extern int vfio_platform_dev_properties(struct device *dev, uint32_t *flags,
> +					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 */
>  #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 */
>  	__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)
> + * 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
> + * or an array of u32 values is expected. On success, data[] will be filled
> + * with the requested information, either as an array of u32, or with a list
> + * 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
> + *          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.

> + */
> +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)
>   *

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

[Baptiste Reynal]

[-- Attachment #1.1: Type: text/plain, Size: 11670 bytes --]

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:
> > From: Antonios Motakis <a.motakis@virtualopensystems.com>
> >
> > 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 <a.motakis@virtualopensystems.com>
> > Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> >
> > ---
> > 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
> +++++++++++++++++++++++++++
> >  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
> > 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
> > +vfio-platform-y := vfio_platform.o vfio_platform_common.o
> vfio_platform_irq.o \
> > +                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
> > 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)
> > +{
> > +     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 ?


>
> > +     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
> > 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.


>
> > +                             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,
> >
> >       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.


>
> > +             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
> > 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,
> >                                       unsigned start, unsigned count,
> >                                       void *data);
> >
> > +/* device properties support in properties.c */
> > +extern int vfio_platform_dev_properties(struct device *dev, uint32_t
> *flags,
> > +                                     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
> */
> >  #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
> */
> >       __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)
> > + * 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
> > + * or an array of u32 values is expected. On success, data[] will be
> filled
> > + * with the requested information, either as an array of u32, or with a
> list
> > + * 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
> > + *          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.


>
> > + */
> > +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)
> >   *
>
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 15732 bytes --]

[-- Attachment #2: Type: text/plain, Size: 151 bytes --]

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[RFC PATCH v4 2/3] vfio: platform: access device property as a list of strings

[Baptiste Reynal]

From: Antonios Motakis <a.motakis@virtualopensystems.com>

Certain device properties (e.g. the device node name, the compatible
string), are available as a list of strings (separated by the null
terminating character). Let the VFIO user query this type of properties.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>

---
v3 -> v4:
 - The list length is computed before strings copy. If the entire list
   doesn't fit, no strings are copied to the user.
---
 drivers/vfio/platform/properties.c | 43 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/platform/properties.c b/drivers/vfio/platform/properties.c
index 98754c2..8bf9c8f 100644
--- a/drivers/vfio/platform/properties.c
+++ b/drivers/vfio/platform/properties.c
@@ -22,7 +22,48 @@ static int dev_property_get_strings(struct device *dev, uint32_t *flags,
 				    char *name, unsigned *lenp,
 				    void __user *datap, unsigned long datasz)
 {
-	return -EINVAL;
+	const char **val;
+	int n, i, ret;
+
+	if (lenp == NULL)
+		return -EFAULT;
+
+	*lenp = 0;
+
+	n = device_property_read_string_array(dev, name, NULL, 0);
+	if (n < 0)
+		return n;
+
+	val = kcalloc(n, sizeof(char *), GFP_KERNEL);
+	if (!val)
+		return -ENOMEM;
+
+	ret = device_property_read_string_array(dev, name, val, n);
+	if (ret < 0)
+		goto out;
+
+	for (i = 0; i < n; i++)
+		*lenp += strlen(val[i]) + 1;
+
+	if (datasz < *lenp) {
+		ret = -E2BIG;
+		goto out;
+	}
+
+	for (i = 0; i < n; i++) {
+		size_t len = strlen(val[i]) + 1;
+
+		if (copy_to_user(datap, val[i], strlen(val[i]) + 1)) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		datap += len;
+	}
+
+out:
+	kfree(val);
+	return ret;
 }
 
 static int dev_property_get_uint(struct device *dev, uint32_t *flags,
-- 
2.5.1

Re: [RFC PATCH v4 2/3] vfio: platform: access device property as a list of strings

[Alex Williamson]

On Wed, 2015-09-09 at 11:17 +0200, Baptiste Reynal wrote:
> From: Antonios Motakis <a.motakis@virtualopensystems.com>
> 
> Certain device properties (e.g. the device node name, the compatible
> string), are available as a list of strings (separated by the null
> terminating character). Let the VFIO user query this type of properties.
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> 
> ---
> v3 -> v4:
>  - The list length is computed before strings copy. If the entire list
>    doesn't fit, no strings are copied to the user.
> ---
>  drivers/vfio/platform/properties.c | 43 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/platform/properties.c b/drivers/vfio/platform/properties.c
> index 98754c2..8bf9c8f 100644
> --- a/drivers/vfio/platform/properties.c
> +++ b/drivers/vfio/platform/properties.c
> @@ -22,7 +22,48 @@ static int dev_property_get_strings(struct device *dev, uint32_t *flags,
>  				    char *name, unsigned *lenp,
>  				    void __user *datap, unsigned long datasz)
>  {
> -	return -EINVAL;
> +	const char **val;
> +	int n, i, ret;
> +
> +	if (lenp == NULL)
> +		return -EFAULT;

Paranoia?

> +
> +	*lenp = 0;
> +
> +	n = device_property_read_string_array(dev, name, NULL, 0);
> +	if (n < 0)
> +		return n;
> +
> +	val = kcalloc(n, sizeof(char *), GFP_KERNEL);
> +	if (!val)
> +		return -ENOMEM;
> +
> +	ret = device_property_read_string_array(dev, name, val, n);
> +	if (ret < 0)
> +		goto out;
> +
> +	for (i = 0; i < n; i++)
> +		*lenp += strlen(val[i]) + 1;
> +
> +	if (datasz < *lenp) {
> +		ret = -E2BIG;
> +		goto out;
> +	}
> +
> +	for (i = 0; i < n; i++) {
> +		size_t len = strlen(val[i]) + 1;
> +
> +		if (copy_to_user(datap, val[i], strlen(val[i]) + 1)) {

No need to call strlen() again here

> +			ret = -EFAULT;
> +			goto out;
> +		}
> +
> +		datap += len;
> +	}
> +
> +out:
> +	kfree(val);
> +	return ret;
>  }
>  
>  static int dev_property_get_uint(struct device *dev, uint32_t *flags,

Re: [RFC PATCH v4 2/3] vfio: platform: access device property as a list of strings

[Baptiste Reynal]

[-- Attachment #1.1: Type: text/plain, Size: 2682 bytes --]

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:
> > From: Antonios Motakis <a.motakis@virtualopensystems.com>
> >
> > Certain device properties (e.g. the device node name, the compatible
> > string), are available as a list of strings (separated by the null
> > terminating character). Let the VFIO user query this type of properties.
> >
> > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> > Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> >
> > ---
> > v3 -> v4:
> >  - The list length is computed before strings copy. If the entire list
> >    doesn't fit, no strings are copied to the user.
> > ---
> >  drivers/vfio/platform/properties.c | 43
> +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 42 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vfio/platform/properties.c
> b/drivers/vfio/platform/properties.c
> > index 98754c2..8bf9c8f 100644
> > --- a/drivers/vfio/platform/properties.c
> > +++ b/drivers/vfio/platform/properties.c
> > @@ -22,7 +22,48 @@ static int dev_property_get_strings(struct device
> *dev, uint32_t *flags,
> >                                   char *name, unsigned *lenp,
> >                                   void __user *datap, unsigned long
> datasz)
> >  {
> > -     return -EINVAL;
> > +     const char **val;
> > +     int n, i, ret;
> > +
> > +     if (lenp == NULL)
> > +             return -EFAULT;
>
> Paranoia?
>

Kind of, automatic reflex.


>
> > +
> > +     *lenp = 0;
> > +
> > +     n = device_property_read_string_array(dev, name, NULL, 0);
> > +     if (n < 0)
> > +             return n;
> > +
> > +     val = kcalloc(n, sizeof(char *), GFP_KERNEL);
> > +     if (!val)
> > +             return -ENOMEM;
> > +
> > +     ret = device_property_read_string_array(dev, name, val, n);
> > +     if (ret < 0)
> > +             goto out;
> > +
> > +     for (i = 0; i < n; i++)
> > +             *lenp += strlen(val[i]) + 1;
> > +
> > +     if (datasz < *lenp) {
> > +             ret = -E2BIG;
> > +             goto out;
> > +     }
> > +
> > +     for (i = 0; i < n; i++) {
> > +             size_t len = strlen(val[i]) + 1;
> > +
> > +             if (copy_to_user(datap, val[i], strlen(val[i]) + 1)) {
>
> No need to call strlen() again here
>

Thanks, will be fixed.


>
> > +                     ret = -EFAULT;
> > +                     goto out;
> > +             }
> > +
> > +             datap += len;
> > +     }
> > +
> > +out:
> > +     kfree(val);
> > +     return ret;
> >  }
> >
> >  static int dev_property_get_uint(struct device *dev, uint32_t *flags,
>
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 4272 bytes --]

[-- Attachment #2: Type: text/plain, Size: 151 bytes --]

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[RFC PATCH v4 3/3] vfio: platform: return device properties as arrays of unsigned integers

[Baptiste Reynal]

From: Antonios Motakis <a.motakis@virtualopensystems.com>

Certain properties of a device are accessible as an array of unsigned
integers, either u64, u32, u16, or u8. Let the VFIO user query this
type of device properties.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
---
 drivers/vfio/platform/properties.c | 62 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/platform/properties.c b/drivers/vfio/platform/properties.c
index 8bf9c8f..625e2d3 100644
--- a/drivers/vfio/platform/properties.c
+++ b/drivers/vfio/platform/properties.c
@@ -70,7 +70,67 @@ 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 ret, n;
+	u8 *out;
+	size_t sz;
+	int (*func)(const struct device *, const char *, void *, size_t)
+		= NULL;
+
+	switch (type) {
+	case VFIO_DEV_PROPERTY_TYPE_U64:
+		sz = sizeof(u64);
+		func = (int (*)(const struct device *,
+				const char *, void *, size_t))
+			device_property_read_u64_array;
+		break;
+	case VFIO_DEV_PROPERTY_TYPE_U32:
+		sz = sizeof(u32);
+		func = (int (*)(const struct device *,
+				const char *, void *, size_t))
+			device_property_read_u32_array;
+		break;
+	case VFIO_DEV_PROPERTY_TYPE_U16:
+		sz = sizeof(u16);
+		func = (int (*)(const struct device *,
+				const char *, void *, size_t))
+			device_property_read_u16_array;
+		break;
+	case VFIO_DEV_PROPERTY_TYPE_U8:
+		sz = sizeof(u8);
+		func = (int (*)(const struct device *,
+				const char *, void *, size_t))
+			device_property_read_u8_array;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	/* get size of array */
+	n = func(dev, name, NULL, 0);
+	if (n < 0)
+		return n;
+
+	if (lenp)
+		*lenp = n * sz;
+
+	if (n * sz > datasz)
+		return -EOVERFLOW;
+
+	out = kcalloc(n, sz, GFP_KERNEL);
+	if (!out)
+		return -ENOMEM;
+
+	ret = func(dev, name, out, n);
+	if (ret)
+		goto out;
+
+	if (copy_to_user(datap, out, n * sz))
+		ret = -EFAULT;
+
+out:
+	kfree(out);
+	return ret;
 }
 
 int vfio_platform_dev_properties(struct device *dev, uint32_t *flags,
-- 
2.5.1

Re: [RFC PATCH v4 3/3] vfio: platform: return device properties as arrays of unsigned integers

[Alex Williamson]

On Wed, 2015-09-09 at 11:17 +0200, Baptiste Reynal wrote:
> From: Antonios Motakis <a.motakis@virtualopensystems.com>
> 
> Certain properties of a device are accessible as an array of unsigned
> integers, either u64, u32, u16, or u8. Let the VFIO user query this
> type of device properties.
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> ---
>  drivers/vfio/platform/properties.c | 62 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 61 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/platform/properties.c b/drivers/vfio/platform/properties.c
> index 8bf9c8f..625e2d3 100644
> --- a/drivers/vfio/platform/properties.c
> +++ b/drivers/vfio/platform/properties.c
> @@ -70,7 +70,67 @@ 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 ret, n;
> +	u8 *out;
> +	size_t sz;
> +	int (*func)(const struct device *, const char *, void *, size_t)
> +		= NULL;
> +
> +	switch (type) {
> +	case VFIO_DEV_PROPERTY_TYPE_U64:
> +		sz = sizeof(u64);
> +		func = (int (*)(const struct device *,
> +				const char *, void *, size_t))
> +			device_property_read_u64_array;
> +		break;
> +	case VFIO_DEV_PROPERTY_TYPE_U32:
> +		sz = sizeof(u32);
> +		func = (int (*)(const struct device *,
> +				const char *, void *, size_t))
> +			device_property_read_u32_array;
> +		break;
> +	case VFIO_DEV_PROPERTY_TYPE_U16:
> +		sz = sizeof(u16);
> +		func = (int (*)(const struct device *,
> +				const char *, void *, size_t))
> +			device_property_read_u16_array;
> +		break;
> +	case VFIO_DEV_PROPERTY_TYPE_U8:
> +		sz = sizeof(u8);
> +		func = (int (*)(const struct device *,
> +				const char *, void *, size_t))
> +			device_property_read_u8_array;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* get size of array */
> +	n = func(dev, name, NULL, 0);
> +	if (n < 0)
> +		return n;
> +
> +	if (lenp)
> +		*lenp = n * sz;

Why is this conditional?

> +
> +	if (n * sz > datasz)
> +		return -EOVERFLOW;

Ugh, this isn't E2BIG or ENOSPC...

> +
> +	out = kcalloc(n, sz, GFP_KERNEL);
> +	if (!out)
> +		return -ENOMEM;
> +
> +	ret = func(dev, name, out, n);
> +	if (ret)
> +		goto out;
> +
> +	if (copy_to_user(datap, out, n * sz))
> +		ret = -EFAULT;
> +
> +out:
> +	kfree(out);
> +	return ret;
>  }
>  
>  int vfio_platform_dev_properties(struct device *dev, uint32_t *flags,

Re: [RFC PATCH v4 3/3] vfio: platform: return device properties as arrays of unsigned integers

[Baptiste Reynal]

[-- Attachment #1.1: Type: text/plain, Size: 3484 bytes --]

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:
> > From: Antonios Motakis <a.motakis@virtualopensystems.com>
> >
> > Certain properties of a device are accessible as an array of unsigned
> > integers, either u64, u32, u16, or u8. Let the VFIO user query this
> > type of device properties.
> >
> > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> > Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> > ---
> >  drivers/vfio/platform/properties.c | 62
> +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 61 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vfio/platform/properties.c
> b/drivers/vfio/platform/properties.c
> > index 8bf9c8f..625e2d3 100644
> > --- a/drivers/vfio/platform/properties.c
> > +++ b/drivers/vfio/platform/properties.c
> > @@ -70,7 +70,67 @@ 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 ret, n;
> > +     u8 *out;
> > +     size_t sz;
> > +     int (*func)(const struct device *, const char *, void *, size_t)
> > +             = NULL;
> > +
> > +     switch (type) {
> > +     case VFIO_DEV_PROPERTY_TYPE_U64:
> > +             sz = sizeof(u64);
> > +             func = (int (*)(const struct device *,
> > +                             const char *, void *, size_t))
> > +                     device_property_read_u64_array;
> > +             break;
> > +     case VFIO_DEV_PROPERTY_TYPE_U32:
> > +             sz = sizeof(u32);
> > +             func = (int (*)(const struct device *,
> > +                             const char *, void *, size_t))
> > +                     device_property_read_u32_array;
> > +             break;
> > +     case VFIO_DEV_PROPERTY_TYPE_U16:
> > +             sz = sizeof(u16);
> > +             func = (int (*)(const struct device *,
> > +                             const char *, void *, size_t))
> > +                     device_property_read_u16_array;
> > +             break;
> > +     case VFIO_DEV_PROPERTY_TYPE_U8:
> > +             sz = sizeof(u8);
> > +             func = (int (*)(const struct device *,
> > +                             const char *, void *, size_t))
> > +                     device_property_read_u8_array;
> > +             break;
> > +
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     /* get size of array */
> > +     n = func(dev, name, NULL, 0);
> > +     if (n < 0)
> > +             return n;
> > +
> > +     if (lenp)
> > +             *lenp = n * sz;
>
> Why is this conditional?
>

Same paranoia as before. If lenp is not allocated, the function behaviour
is the same.


>
> > +
> > +     if (n * sz > datasz)
> > +             return -EOVERFLOW;
>
> Ugh, this isn't E2BIG or ENOSPC...
>

Ok, will be changed to ENOSPC.


>
> > +
> > +     out = kcalloc(n, sz, GFP_KERNEL);
> > +     if (!out)
> > +             return -ENOMEM;
> > +
> > +     ret = func(dev, name, out, n);
> > +     if (ret)
> > +             goto out;
> > +
> > +     if (copy_to_user(datap, out, n * sz))
> > +             ret = -EFAULT;
> > +
> > +out:
> > +     kfree(out);
> > +     return ret;
> >  }
> >
> >  int vfio_platform_dev_properties(struct device *dev, uint32_t *flags,
>
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 5286 bytes --]

[-- Attachment #2: Type: text/plain, Size: 151 bytes --]

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH v4 0/3] vfio: platform: return device properties for a platform device

[Alex Williamson]

On Wed, 2015-09-09 at 11:17 +0200, Baptiste Reynal wrote:
> This RFC's intention is to show what an interface to access device
> properties for the VFIO platform driver can look like. These properties
> can be retrieved from the device tree node describing the device, or ACPI 
> properties.
> 
> If a device property corresponding to a platform device bound to VFIO PLATFORM
> or VFIO AMBA is available, this patch series will allow the user to query
> for this information. This can be useful for userspace drivers to automatically
> query parameters related to the device.
> 
> Specifically for QEMU, reading the "compatible" property of the device tree
> node could be of use to find out what device is being assigned to the guest and
> handle appropriately a wider range of devices in the future, and to generate an
> appropriate device tree for the guest.
> 
> Older versions of this series were specifically targeted at device tree
> properties. The v3 has been reworked on top of Rafael J. Wysocki's
> uniform device properties API for device tree and ACPI devices. This will allow
> us to use the API in the future with devices described via ACPI.
> 
> The API to get the list of available properties and the device tree full_name
> have been removed. These probably don't serve an useful purpose, as the user
> of this API need to know anyway what properties are specific to the device he
> wants to access with VFIO. If we decide to reintroduce the list of properties
> in the future, the generic device properties API in the kernel will have to
> be extended accordingly.
> 
> A kernel with this series and all the dependencies applied can be pulled from
> branch vfio-device-properties-v4 from the repository:
> https://git.virtualopensystems.com/dev/linux.git


Did we ever get past whether this is a necessary interface and why it's
vfio's job to provide it?  Support for sysfs is a requirement for QEMU
to determine the correct IOMMU group number for a device, and it doesn't
seem unreasonable for sysfs to provide retrieval of device property
information for all users, not just vfio.  So I haven't fully bought
into this interface yet, but I reviewed it nonetheless.  Thanks,

Alex

> Changes since v3:
>  - Rebased on top on v4.2
>  - Rework VFIO_DEVICE_GET_DEV_PROPERTY ioctl
>  - Rework code according to Eric Auger's comments
> Changes since v2:
>  - Reworked on top of Rafael J. Wysocki's uniform device properties API for
>    device tree and ACPI
>  - Support for u64 array properties
>  - Removed API to get list of available properties and device tree full_name
> Changes since v1:
>  - Updated for VFIO platform patch series v8:
>    VFIO AMBA devices now supported in addition to VFIO PLATFORM devices
>  - Refactored and cleaned up the code
> 
> Antonios Motakis (3):
>   vfio: platform: add device properties skeleton and user API
>   vfio: platform: access device property as a list of strings
>   vfio: platform: return device properties as arrays of unsigned
>     integers
> 
>  drivers/vfio/platform/Makefile                |   3 +-
>  drivers/vfio/platform/properties.c            | 178 ++++++++++++++++++++++++++
>  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, 253 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/vfio/platform/properties.c
>