* [RFC PATCH v4 1/3] vfio: platform: add device properties skeleton and user API
[not found] <1441790231-22920-1-git-send-email-b.reynal@virtualopensystems.com>
@ 2015-09-09 9:17 ` Baptiste Reynal
[not found] ` <1441790231-22920-2-git-send-email-b.reynal-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
0 siblings, 1 reply; 3+ messages in thread
From: Baptiste Reynal @ 2015-09-09 9:17 UTC (permalink / raw)
To: kvmarm, iommu
Cc: open list:VFIO PLATFORM DRIVER, open list:ABI/API, open list,
alex.williamson, tech
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC PATCH v4 1/3] vfio: platform: add device properties skeleton and user API
[not found] ` <1441790231-22920-2-git-send-email-b.reynal-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
@ 2015-09-09 20:48 ` Alex Williamson
2015-09-10 6:35 ` Baptiste Reynal
0 siblings, 1 reply; 3+ messages in thread
From: Alex Williamson @ 2015-09-09 20:48 UTC (permalink / raw)
To: Baptiste Reynal
Cc: kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
christoffer.dall-QSEj5FYQhm4dnm+yROfE0A,
eric.auger-QSEj5FYQhm4dnm+yROfE0A,
tech-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J, Antonios Motakis,
open list, open list:VFIO PLATFORM DRIVER, open list:ABI/API
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)
> *
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC PATCH v4 1/3] vfio: platform: add device properties skeleton and user API
2015-09-09 20:48 ` Alex Williamson
@ 2015-09-10 6:35 ` Baptiste Reynal
0 siblings, 0 replies; 3+ messages in thread
From: Baptiste Reynal @ 2015-09-10 6:35 UTC (permalink / raw)
To: Alex Williamson
Cc: open list:VFIO PLATFORM DRIVER, open list:ABI/API, open list,
Linux IOMMU, VirtualOpenSystems Technical Team, kvm-arm
[-- 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-09-10 6:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1441790231-22920-1-git-send-email-b.reynal@virtualopensystems.com>
2015-09-09 9:17 ` [RFC PATCH v4 1/3] vfio: platform: add device properties skeleton and user API Baptiste Reynal
[not found] ` <1441790231-22920-2-git-send-email-b.reynal-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
2015-09-09 20:48 ` Alex Williamson
2015-09-10 6:35 ` Baptiste Reynal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).