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

[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