This patch enables the IOCTLs VFIO_DEVICE_GET_REGION_INFO ioctl call,
which allows the user to learn about the available MMIO resources of
a device.
Signed-off-by: Antonios Motakis <[email protected]>
---
drivers/vfio/platform/vfio_platform_common.c | 93 +++++++++++++++++++++++++--
drivers/vfio/platform/vfio_platform_private.h | 22 +++++++
2 files changed, 111 insertions(+), 4 deletions(-)
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 1e4073f..8a7e474 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -27,17 +27,84 @@
#include "vfio_platform_private.h"
+static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
+{
+ int cnt = 0, i;
+
+ while (vdev->get_resource(vdev, cnt))
+ cnt++;
+
+ vdev->regions = kcalloc(cnt, sizeof(struct vfio_platform_region),
+ GFP_KERNEL);
+ if (!vdev->regions)
+ return -ENOMEM;
+
+ for (i = 0; i < cnt; i++) {
+ struct resource *res =
+ vdev->get_resource(vdev, i);
+
+ if (!res)
+ goto err;
+
+ vdev->regions[i].addr = res->start;
+ vdev->regions[i].size = resource_size(res);
+ vdev->regions[i].flags = 0;
+
+ switch (resource_type(res)) {
+ case IORESOURCE_MEM:
+ vdev->regions[i].type = VFIO_PLATFORM_REGION_TYPE_MMIO;
+ break;
+ case IORESOURCE_IO:
+ vdev->regions[i].type = VFIO_PLATFORM_REGION_TYPE_PIO;
+ break;
+ default:
+ goto err;
+ }
+ }
+
+ vdev->num_regions = cnt;
+
+ return 0;
+err:
+ kfree(vdev->regions);
+ return -EINVAL;
+}
+
+static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
+{
+ vdev->num_regions = 0;
+ kfree(vdev->regions);
+}
+
static void vfio_platform_release(void *device_data)
{
+ struct vfio_platform_device *vdev = device_data;
+
+ if (atomic_dec_and_test(&vdev->refcnt))
+ vfio_platform_regions_cleanup(vdev);
+
module_put(THIS_MODULE);
}
static int vfio_platform_open(void *device_data)
{
+ struct vfio_platform_device *vdev = device_data;
+ int ret;
+
if (!try_module_get(THIS_MODULE))
return -ENODEV;
+ if (atomic_inc_return(&vdev->refcnt) == 1) {
+ ret = vfio_platform_regions_init(vdev);
+ if (ret)
+ goto err_reg;
+ }
+
return 0;
+
+err_reg:
+ module_put(THIS_MODULE);
+ return ret;
}
static long vfio_platform_ioctl(void *device_data,
@@ -58,15 +125,33 @@ static long vfio_platform_ioctl(void *device_data,
return -EINVAL;
info.flags = vdev->flags;
- info.num_regions = 0;
+ info.num_regions = vdev->num_regions;
info.num_irqs = 0;
return copy_to_user((void __user *)arg, &info, minsz);
- } else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
- return -EINVAL;
+ } else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
+ struct vfio_region_info info;
+
+ minsz = offsetofend(struct vfio_region_info, offset);
+
+ if (copy_from_user(&info, (void __user *)arg, minsz))
+ return -EFAULT;
+
+ if (info.argsz < minsz)
+ return -EINVAL;
+
+ if (info.index >= vdev->num_regions)
+ return -EINVAL;
+
+ /* map offset to the physical address */
+ info.offset = VFIO_PLATFORM_INDEX_TO_OFFSET(info.index);
+ info.size = vdev->regions[info.index].size;
+ info.flags = vdev->regions[info.index].flags;
+
+ return copy_to_user((void __user *)arg, &info, minsz);
- else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
+ } else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
return -EINVAL;
else if (cmd == VFIO_DEVICE_SET_IRQS)
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index ef76737..2a06035 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -15,7 +15,29 @@
#ifndef VFIO_PLATFORM_PRIVATE_H
#define VFIO_PLATFORM_PRIVATE_H
+#define VFIO_PLATFORM_OFFSET_SHIFT 40
+#define VFIO_PLATFORM_OFFSET_MASK (((u64)(1) << VFIO_PLATFORM_OFFSET_SHIFT) - 1)
+
+#define VFIO_PLATFORM_OFFSET_TO_INDEX(off) \
+ (off >> VFIO_PLATFORM_OFFSET_SHIFT)
+
+#define VFIO_PLATFORM_INDEX_TO_OFFSET(index) \
+ ((u64)(index) << VFIO_PLATFORM_OFFSET_SHIFT)
+
+struct vfio_platform_region {
+ u64 addr;
+ resource_size_t size;
+ u32 flags;
+ u32 type;
+#define VFIO_PLATFORM_REGION_TYPE_MMIO 1
+#define VFIO_PLATFORM_REGION_TYPE_PIO 2
+};
+
struct vfio_platform_device {
+ struct vfio_platform_region *regions;
+ u32 num_regions;
+ atomic_t refcnt;
+
/*
* These fields should be filled by the bus driver at binding time
*/
--
2.1.1
On Mon, 2014-10-13 at 15:10 +0200, Antonios Motakis wrote:
> This patch enables the IOCTLs VFIO_DEVICE_GET_REGION_INFO ioctl call,
> which allows the user to learn about the available MMIO resources of
> a device.
>
> Signed-off-by: Antonios Motakis <[email protected]>
> ---
> drivers/vfio/platform/vfio_platform_common.c | 93 +++++++++++++++++++++++++--
> drivers/vfio/platform/vfio_platform_private.h | 22 +++++++
> 2 files changed, 111 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index 1e4073f..8a7e474 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -27,17 +27,84 @@
>
> #include "vfio_platform_private.h"
>
> +static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
> +{
> + int cnt = 0, i;
> +
> + while (vdev->get_resource(vdev, cnt))
> + cnt++;
> +
> + vdev->regions = kcalloc(cnt, sizeof(struct vfio_platform_region),
> + GFP_KERNEL);
> + if (!vdev->regions)
> + return -ENOMEM;
> +
> + for (i = 0; i < cnt; i++) {
> + struct resource *res =
> + vdev->get_resource(vdev, i);
> +
> + if (!res)
> + goto err;
> +
> + vdev->regions[i].addr = res->start;
> + vdev->regions[i].size = resource_size(res);
> + vdev->regions[i].flags = 0;
> +
> + switch (resource_type(res)) {
> + case IORESOURCE_MEM:
> + vdev->regions[i].type = VFIO_PLATFORM_REGION_TYPE_MMIO;
> + break;
> + case IORESOURCE_IO:
> + vdev->regions[i].type = VFIO_PLATFORM_REGION_TYPE_PIO;
> + break;
Ok, we have support for PIO in platform now (thanks!), does the user
know what type of region they're dealing with? Do they care? For PCI
the user tests the PCI BAR in config space to determine which type it
is. I'm guessing that platform would do something similar against the
device tree or ACPI, right?
> + default:
> + goto err;
> + }
> + }
> +
> + vdev->num_regions = cnt;
> +
> + return 0;
> +err:
> + kfree(vdev->regions);
> + return -EINVAL;
> +}
> +
> +static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
> +{
> + vdev->num_regions = 0;
> + kfree(vdev->regions);
> +}
> +
> static void vfio_platform_release(void *device_data)
> {
> + struct vfio_platform_device *vdev = device_data;
> +
> + if (atomic_dec_and_test(&vdev->refcnt))
> + vfio_platform_regions_cleanup(vdev);
> +
> module_put(THIS_MODULE);
> }
>
> static int vfio_platform_open(void *device_data)
> {
> + struct vfio_platform_device *vdev = device_data;
> + int ret;
> +
> if (!try_module_get(THIS_MODULE))
> return -ENODEV;
>
> + if (atomic_inc_return(&vdev->refcnt) == 1) {
> + ret = vfio_platform_regions_init(vdev);
> + if (ret)
> + goto err_reg;
> + }
> +
> return 0;
> +
> +err_reg:
> + module_put(THIS_MODULE);
> + return ret;
Note that if vfio_platform_regions_init() fails then your refcnt is
wrong. We switched to a mutex in vfio-pci to avoid this. See
61d792562b53.
> }
>
> static long vfio_platform_ioctl(void *device_data,
> @@ -58,15 +125,33 @@ static long vfio_platform_ioctl(void *device_data,
> return -EINVAL;
>
> info.flags = vdev->flags;
> - info.num_regions = 0;
> + info.num_regions = vdev->num_regions;
> info.num_irqs = 0;
>
> return copy_to_user((void __user *)arg, &info, minsz);
>
> - } else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
> - return -EINVAL;
> + } else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
> + struct vfio_region_info info;
> +
> + minsz = offsetofend(struct vfio_region_info, offset);
> +
> + if (copy_from_user(&info, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (info.argsz < minsz)
> + return -EINVAL;
> +
> + if (info.index >= vdev->num_regions)
> + return -EINVAL;
> +
> + /* map offset to the physical address */
> + info.offset = VFIO_PLATFORM_INDEX_TO_OFFSET(info.index);
> + info.size = vdev->regions[info.index].size;
> + info.flags = vdev->regions[info.index].flags;
> +
> + return copy_to_user((void __user *)arg, &info, minsz);
>
> - else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
> + } else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
> return -EINVAL;
>
> else if (cmd == VFIO_DEVICE_SET_IRQS)
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index ef76737..2a06035 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -15,7 +15,29 @@
> #ifndef VFIO_PLATFORM_PRIVATE_H
> #define VFIO_PLATFORM_PRIVATE_H
>
> +#define VFIO_PLATFORM_OFFSET_SHIFT 40
> +#define VFIO_PLATFORM_OFFSET_MASK (((u64)(1) << VFIO_PLATFORM_OFFSET_SHIFT) - 1)
> +
> +#define VFIO_PLATFORM_OFFSET_TO_INDEX(off) \
> + (off >> VFIO_PLATFORM_OFFSET_SHIFT)
> +
> +#define VFIO_PLATFORM_INDEX_TO_OFFSET(index) \
> + ((u64)(index) << VFIO_PLATFORM_OFFSET_SHIFT)
> +
> +struct vfio_platform_region {
> + u64 addr;
> + resource_size_t size;
> + u32 flags;
> + u32 type;
> +#define VFIO_PLATFORM_REGION_TYPE_MMIO 1
> +#define VFIO_PLATFORM_REGION_TYPE_PIO 2
> +};
> +
> struct vfio_platform_device {
> + struct vfio_platform_region *regions;
> + u32 num_regions;
> + atomic_t refcnt;
> +
> /*
> * These fields should be filled by the bus driver at binding time
> */
> -----Original Message-----
> From: [email protected] [mailto:iommu-
> [email protected]] On Behalf Of Alex Williamson
> Sent: Tuesday, October 21, 2014 10:05 PM
> To: Antonios Motakis
> Cc: open list:VFIO DRIVER; [email protected]; [email protected];
> [email protected]; open list; [email protected];
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v8 07/18] vfio/platform: return info for device memory
> mapped IO regions
>
> On Mon, 2014-10-13 at 15:10 +0200, Antonios Motakis wrote:
> > This patch enables the IOCTLs VFIO_DEVICE_GET_REGION_INFO ioctl call,
> > which allows the user to learn about the available MMIO resources of a
> > device.
> >
> > Signed-off-by: Antonios Motakis <[email protected]>
> > ---
> > drivers/vfio/platform/vfio_platform_common.c | 93
> > +++++++++++++++++++++++++--
> > drivers/vfio/platform/vfio_platform_private.h | 22 +++++++
> > 2 files changed, 111 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/vfio/platform/vfio_platform_common.c
> > b/drivers/vfio/platform/vfio_platform_common.c
> > index 1e4073f..8a7e474 100644
> > --- a/drivers/vfio/platform/vfio_platform_common.c
> > +++ b/drivers/vfio/platform/vfio_platform_common.c
> > @@ -27,17 +27,84 @@
> >
> > #include "vfio_platform_private.h"
> >
> > +static int vfio_platform_regions_init(struct vfio_platform_device
> > +*vdev) {
> > + int cnt = 0, i;
> > +
> > + while (vdev->get_resource(vdev, cnt))
> > + cnt++;
> > +
> > + vdev->regions = kcalloc(cnt, sizeof(struct vfio_platform_region),
> > + GFP_KERNEL);
> > + if (!vdev->regions)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < cnt; i++) {
> > + struct resource *res =
> > + vdev->get_resource(vdev, i);
> > +
> > + if (!res)
> > + goto err;
> > +
> > + vdev->regions[i].addr = res->start;
> > + vdev->regions[i].size = resource_size(res);
> > + vdev->regions[i].flags = 0;
> > +
> > + switch (resource_type(res)) {
> > + case IORESOURCE_MEM:
> > + vdev->regions[i].type = VFIO_PLATFORM_REGION_TYPE_MMIO;
> > + break;
> > + case IORESOURCE_IO:
> > + vdev->regions[i].type = VFIO_PLATFORM_REGION_TYPE_PIO;
> > + break;
>
> Ok, we have support for PIO in platform now (thanks!), does the user know what
> type of region they're dealing with? Do they care? For PCI the user tests the
> PCI BAR in config space to determine which type it is. I'm guessing that
> platform would do something similar against the device tree or ACPI, right?
Interested in knowing what type of PIO region in a platform device, is this for I2C/SPI type of device?
Thanks
-Bharat
>
> > + default:
> > + goto err;
> > + }
> > + }
> > +
> > + vdev->num_regions = cnt;
> > +
> > + return 0;
> > +err:
> > + kfree(vdev->regions);
> > + return -EINVAL;
> > +}
> > +
> > +static void vfio_platform_regions_cleanup(struct vfio_platform_device
> > +*vdev) {
> > + vdev->num_regions = 0;
> > + kfree(vdev->regions);
> > +}
> > +
> > static void vfio_platform_release(void *device_data) {
> > + struct vfio_platform_device *vdev = device_data;
> > +
> > + if (atomic_dec_and_test(&vdev->refcnt))
> > + vfio_platform_regions_cleanup(vdev);
> > +
> > module_put(THIS_MODULE);
> > }
> >
> > static int vfio_platform_open(void *device_data) {
> > + struct vfio_platform_device *vdev = device_data;
> > + int ret;
> > +
> > if (!try_module_get(THIS_MODULE))
> > return -ENODEV;
> >
> > + if (atomic_inc_return(&vdev->refcnt) == 1) {
> > + ret = vfio_platform_regions_init(vdev);
> > + if (ret)
> > + goto err_reg;
> > + }
> > +
> > return 0;
> > +
> > +err_reg:
> > + module_put(THIS_MODULE);
> > + return ret;
>
> Note that if vfio_platform_regions_init() fails then your refcnt is wrong. We
> switched to a mutex in vfio-pci to avoid this. See 61d792562b53.
>
> > }
> >
> > static long vfio_platform_ioctl(void *device_data, @@ -58,15 +125,33
> > @@ static long vfio_platform_ioctl(void *device_data,
> > return -EINVAL;
> >
> > info.flags = vdev->flags;
> > - info.num_regions = 0;
> > + info.num_regions = vdev->num_regions;
> > info.num_irqs = 0;
> >
> > return copy_to_user((void __user *)arg, &info, minsz);
> >
> > - } else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
> > - return -EINVAL;
> > + } else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
> > + struct vfio_region_info info;
> > +
> > + minsz = offsetofend(struct vfio_region_info, offset);
> > +
> > + if (copy_from_user(&info, (void __user *)arg, minsz))
> > + return -EFAULT;
> > +
> > + if (info.argsz < minsz)
> > + return -EINVAL;
> > +
> > + if (info.index >= vdev->num_regions)
> > + return -EINVAL;
> > +
> > + /* map offset to the physical address */
> > + info.offset = VFIO_PLATFORM_INDEX_TO_OFFSET(info.index);
> > + info.size = vdev->regions[info.index].size;
> > + info.flags = vdev->regions[info.index].flags;
> > +
> > + return copy_to_user((void __user *)arg, &info, minsz);
> >
> > - else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
> > + } else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
> > return -EINVAL;
> >
> > else if (cmd == VFIO_DEVICE_SET_IRQS) diff --git
> > a/drivers/vfio/platform/vfio_platform_private.h
> > b/drivers/vfio/platform/vfio_platform_private.h
> > index ef76737..2a06035 100644
> > --- a/drivers/vfio/platform/vfio_platform_private.h
> > +++ b/drivers/vfio/platform/vfio_platform_private.h
> > @@ -15,7 +15,29 @@
> > #ifndef VFIO_PLATFORM_PRIVATE_H
> > #define VFIO_PLATFORM_PRIVATE_H
> >
> > +#define VFIO_PLATFORM_OFFSET_SHIFT 40
> > +#define VFIO_PLATFORM_OFFSET_MASK (((u64)(1) <<
> > +VFIO_PLATFORM_OFFSET_SHIFT) - 1)
> > +
> > +#define VFIO_PLATFORM_OFFSET_TO_INDEX(off) \
> > + (off >> VFIO_PLATFORM_OFFSET_SHIFT)
> > +
> > +#define VFIO_PLATFORM_INDEX_TO_OFFSET(index) \
> > + ((u64)(index) << VFIO_PLATFORM_OFFSET_SHIFT)
> > +
> > +struct vfio_platform_region {
> > + u64 addr;
> > + resource_size_t size;
> > + u32 flags;
> > + u32 type;
> > +#define VFIO_PLATFORM_REGION_TYPE_MMIO 1
> > +#define VFIO_PLATFORM_REGION_TYPE_PIO 2
> > +};
> > +
> > struct vfio_platform_device {
> > + struct vfio_platform_region *regions;
> > + u32 num_regions;
> > + atomic_t refcnt;
> > +
> > /*
> > * These fields should be filled by the bus driver at binding time
> > */
>
>
>
> _______________________________________________
> iommu mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Tue, 2014-10-21 at 18:18 +0000, [email protected] wrote:
>
> > -----Original Message-----
> > From: [email protected] [mailto:iommu-
> > [email protected]] On Behalf Of Alex Williamson
> > Sent: Tuesday, October 21, 2014 10:05 PM
> > To: Antonios Motakis
> > Cc: open list:VFIO DRIVER; [email protected]; [email protected];
> > [email protected]; open list; [email protected];
> > [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH v8 07/18] vfio/platform: return info for device memory
> > mapped IO regions
> >
> > On Mon, 2014-10-13 at 15:10 +0200, Antonios Motakis wrote:
> > > This patch enables the IOCTLs VFIO_DEVICE_GET_REGION_INFO ioctl call,
> > > which allows the user to learn about the available MMIO resources of a
> > > device.
> > >
> > > Signed-off-by: Antonios Motakis <[email protected]>
> > > ---
> > > drivers/vfio/platform/vfio_platform_common.c | 93
> > > +++++++++++++++++++++++++--
> > > drivers/vfio/platform/vfio_platform_private.h | 22 +++++++
> > > 2 files changed, 111 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/vfio/platform/vfio_platform_common.c
> > > b/drivers/vfio/platform/vfio_platform_common.c
> > > index 1e4073f..8a7e474 100644
> > > --- a/drivers/vfio/platform/vfio_platform_common.c
> > > +++ b/drivers/vfio/platform/vfio_platform_common.c
> > > @@ -27,17 +27,84 @@
> > >
> > > #include "vfio_platform_private.h"
> > >
> > > +static int vfio_platform_regions_init(struct vfio_platform_device
> > > +*vdev) {
> > > + int cnt = 0, i;
> > > +
> > > + while (vdev->get_resource(vdev, cnt))
> > > + cnt++;
> > > +
> > > + vdev->regions = kcalloc(cnt, sizeof(struct vfio_platform_region),
> > > + GFP_KERNEL);
> > > + if (!vdev->regions)
> > > + return -ENOMEM;
> > > +
> > > + for (i = 0; i < cnt; i++) {
> > > + struct resource *res =
> > > + vdev->get_resource(vdev, i);
> > > +
> > > + if (!res)
> > > + goto err;
> > > +
> > > + vdev->regions[i].addr = res->start;
> > > + vdev->regions[i].size = resource_size(res);
> > > + vdev->regions[i].flags = 0;
> > > +
> > > + switch (resource_type(res)) {
> > > + case IORESOURCE_MEM:
> > > + vdev->regions[i].type = VFIO_PLATFORM_REGION_TYPE_MMIO;
> > > + break;
> > > + case IORESOURCE_IO:
> > > + vdev->regions[i].type = VFIO_PLATFORM_REGION_TYPE_PIO;
> > > + break;
> >
> > Ok, we have support for PIO in platform now (thanks!), does the user know what
> > type of region they're dealing with? Do they care? For PCI the user tests the
> > PCI BAR in config space to determine which type it is. I'm guessing that
> > platform would do something similar against the device tree or ACPI, right?
>
> Interested in knowing what type of PIO region in a platform device, is this for I2C/SPI type of device?
I don't think we have any specific users, I had noted in a previous
version that we were searching only for MMIO regions and we at least
needed to figure out how PIO regions would be exposed so we don't have
compatibility issues when adding them (ie. how to match a vfio resource
index to a sysfs/dt/acpi description when we're only reporting a subset
of resources). Antonios has done a good job in this version in adding
all but the accessor functions for PIO. x86 could theoretically have
ACPI defined devices that are exposed as platform devices that could be
exposed using this code. Thanks,
Alex
> > > + default:
> > > + goto err;
> > > + }
> > > + }
> > > +
> > > + vdev->num_regions = cnt;
> > > +
> > > + return 0;
> > > +err:
> > > + kfree(vdev->regions);
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static void vfio_platform_regions_cleanup(struct vfio_platform_device
> > > +*vdev) {
> > > + vdev->num_regions = 0;
> > > + kfree(vdev->regions);
> > > +}
> > > +
> > > static void vfio_platform_release(void *device_data) {
> > > + struct vfio_platform_device *vdev = device_data;
> > > +
> > > + if (atomic_dec_and_test(&vdev->refcnt))
> > > + vfio_platform_regions_cleanup(vdev);
> > > +
> > > module_put(THIS_MODULE);
> > > }
> > >
> > > static int vfio_platform_open(void *device_data) {
> > > + struct vfio_platform_device *vdev = device_data;
> > > + int ret;
> > > +
> > > if (!try_module_get(THIS_MODULE))
> > > return -ENODEV;
> > >
> > > + if (atomic_inc_return(&vdev->refcnt) == 1) {
> > > + ret = vfio_platform_regions_init(vdev);
> > > + if (ret)
> > > + goto err_reg;
> > > + }
> > > +
> > > return 0;
> > > +
> > > +err_reg:
> > > + module_put(THIS_MODULE);
> > > + return ret;
> >
> > Note that if vfio_platform_regions_init() fails then your refcnt is wrong. We
> > switched to a mutex in vfio-pci to avoid this. See 61d792562b53.
> >
> > > }
> > >
> > > static long vfio_platform_ioctl(void *device_data, @@ -58,15 +125,33
> > > @@ static long vfio_platform_ioctl(void *device_data,
> > > return -EINVAL;
> > >
> > > info.flags = vdev->flags;
> > > - info.num_regions = 0;
> > > + info.num_regions = vdev->num_regions;
> > > info.num_irqs = 0;
> > >
> > > return copy_to_user((void __user *)arg, &info, minsz);
> > >
> > > - } else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
> > > - return -EINVAL;
> > > + } else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
> > > + struct vfio_region_info info;
> > > +
> > > + minsz = offsetofend(struct vfio_region_info, offset);
> > > +
> > > + if (copy_from_user(&info, (void __user *)arg, minsz))
> > > + return -EFAULT;
> > > +
> > > + if (info.argsz < minsz)
> > > + return -EINVAL;
> > > +
> > > + if (info.index >= vdev->num_regions)
> > > + return -EINVAL;
> > > +
> > > + /* map offset to the physical address */
> > > + info.offset = VFIO_PLATFORM_INDEX_TO_OFFSET(info.index);
> > > + info.size = vdev->regions[info.index].size;
> > > + info.flags = vdev->regions[info.index].flags;
> > > +
> > > + return copy_to_user((void __user *)arg, &info, minsz);
> > >
> > > - else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
> > > + } else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
> > > return -EINVAL;
> > >
> > > else if (cmd == VFIO_DEVICE_SET_IRQS) diff --git
> > > a/drivers/vfio/platform/vfio_platform_private.h
> > > b/drivers/vfio/platform/vfio_platform_private.h
> > > index ef76737..2a06035 100644
> > > --- a/drivers/vfio/platform/vfio_platform_private.h
> > > +++ b/drivers/vfio/platform/vfio_platform_private.h
> > > @@ -15,7 +15,29 @@
> > > #ifndef VFIO_PLATFORM_PRIVATE_H
> > > #define VFIO_PLATFORM_PRIVATE_H
> > >
> > > +#define VFIO_PLATFORM_OFFSET_SHIFT 40
> > > +#define VFIO_PLATFORM_OFFSET_MASK (((u64)(1) <<
> > > +VFIO_PLATFORM_OFFSET_SHIFT) - 1)
> > > +
> > > +#define VFIO_PLATFORM_OFFSET_TO_INDEX(off) \
> > > + (off >> VFIO_PLATFORM_OFFSET_SHIFT)
> > > +
> > > +#define VFIO_PLATFORM_INDEX_TO_OFFSET(index) \
> > > + ((u64)(index) << VFIO_PLATFORM_OFFSET_SHIFT)
> > > +
> > > +struct vfio_platform_region {
> > > + u64 addr;
> > > + resource_size_t size;
> > > + u32 flags;
> > > + u32 type;
> > > +#define VFIO_PLATFORM_REGION_TYPE_MMIO 1
> > > +#define VFIO_PLATFORM_REGION_TYPE_PIO 2
> > > +};
> > > +
> > > struct vfio_platform_device {
> > > + struct vfio_platform_region *regions;
> > > + u32 num_regions;
> > > + atomic_t refcnt;
> > > +
> > > /*
> > > * These fields should be filled by the bus driver at binding time
> > > */
> >
> >
> >
> > _______________________________________________
> > iommu mailing list
> > [email protected]
> > https://lists.linuxfoundation.org/mailman/listinfo/iommu
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Tue, Oct 21, 2014 at 6:34 PM, Alex Williamson
<[email protected]> wrote:
> On Mon, 2014-10-13 at 15:10 +0200, Antonios Motakis wrote:
>> This patch enables the IOCTLs VFIO_DEVICE_GET_REGION_INFO ioctl call,
>> which allows the user to learn about the available MMIO resources of
>> a device.
>>
>> Signed-off-by: Antonios Motakis <[email protected]>
>> ---
>> drivers/vfio/platform/vfio_platform_common.c | 93 +++++++++++++++++++++++++--
>> drivers/vfio/platform/vfio_platform_private.h | 22 +++++++
>> 2 files changed, 111 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index 1e4073f..8a7e474 100644
>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> @@ -27,17 +27,84 @@
>>
>> #include "vfio_platform_private.h"
>>
>> +static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>> +{
>> + int cnt = 0, i;
>> +
>> + while (vdev->get_resource(vdev, cnt))
>> + cnt++;
>> +
>> + vdev->regions = kcalloc(cnt, sizeof(struct vfio_platform_region),
>> + GFP_KERNEL);
>> + if (!vdev->regions)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < cnt; i++) {
>> + struct resource *res =
>> + vdev->get_resource(vdev, i);
>> +
>> + if (!res)
>> + goto err;
>> +
>> + vdev->regions[i].addr = res->start;
>> + vdev->regions[i].size = resource_size(res);
>> + vdev->regions[i].flags = 0;
>> +
>> + switch (resource_type(res)) {
>> + case IORESOURCE_MEM:
>> + vdev->regions[i].type = VFIO_PLATFORM_REGION_TYPE_MMIO;
>> + break;
>> + case IORESOURCE_IO:
>> + vdev->regions[i].type = VFIO_PLATFORM_REGION_TYPE_PIO;
>> + break;
>
> Ok, we have support for PIO in platform now (thanks!), does the user
> know what type of region they're dealing with? Do they care? For PCI
> the user tests the PCI BAR in config space to determine which type it
> is. I'm guessing that platform would do something similar against the
> device tree or ACPI, right?
Maybe is worthwhile to add an explicit flag in
VFIO_DEVICE_GET_REGION_INFO for PIO regions. For platform devices I
don't know if we can always rely on DT or ACPI info to be available.
For VFIO PCI the BAR is always implemented, and while I have proposed
an RFC to return DT information, I don't think we can assume how a
device is described in the host, whether DT, ACPI, or dark magic.
>
>> + default:
>> + goto err;
>> + }
>> + }
>> +
>> + vdev->num_regions = cnt;
>> +
>> + return 0;
>> +err:
>> + kfree(vdev->regions);
>> + return -EINVAL;
>> +}
>> +
>> +static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
>> +{
>> + vdev->num_regions = 0;
>> + kfree(vdev->regions);
>> +}
>> +
>> static void vfio_platform_release(void *device_data)
>> {
>> + struct vfio_platform_device *vdev = device_data;
>> +
>> + if (atomic_dec_and_test(&vdev->refcnt))
>> + vfio_platform_regions_cleanup(vdev);
>> +
>> module_put(THIS_MODULE);
>> }
>>
>> static int vfio_platform_open(void *device_data)
>> {
>> + struct vfio_platform_device *vdev = device_data;
>> + int ret;
>> +
>> if (!try_module_get(THIS_MODULE))
>> return -ENODEV;
>>
>> + if (atomic_inc_return(&vdev->refcnt) == 1) {
>> + ret = vfio_platform_regions_init(vdev);
>> + if (ret)
>> + goto err_reg;
>> + }
>> +
>> return 0;
>> +
>> +err_reg:
>> + module_put(THIS_MODULE);
>> + return ret;
>
> Note that if vfio_platform_regions_init() fails then your refcnt is
> wrong. We switched to a mutex in vfio-pci to avoid this. See
> 61d792562b53.
Ack.
>
>> }
>>
>> static long vfio_platform_ioctl(void *device_data,
>> @@ -58,15 +125,33 @@ static long vfio_platform_ioctl(void *device_data,
>> return -EINVAL;
>>
>> info.flags = vdev->flags;
>> - info.num_regions = 0;
>> + info.num_regions = vdev->num_regions;
>> info.num_irqs = 0;
>>
>> return copy_to_user((void __user *)arg, &info, minsz);
>>
>> - } else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
>> - return -EINVAL;
>> + } else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
>> + struct vfio_region_info info;
>> +
>> + minsz = offsetofend(struct vfio_region_info, offset);
>> +
>> + if (copy_from_user(&info, (void __user *)arg, minsz))
>> + return -EFAULT;
>> +
>> + if (info.argsz < minsz)
>> + return -EINVAL;
>> +
>> + if (info.index >= vdev->num_regions)
>> + return -EINVAL;
>> +
>> + /* map offset to the physical address */
>> + info.offset = VFIO_PLATFORM_INDEX_TO_OFFSET(info.index);
>> + info.size = vdev->regions[info.index].size;
>> + info.flags = vdev->regions[info.index].flags;
>> +
>> + return copy_to_user((void __user *)arg, &info, minsz);
>>
>> - else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
>> + } else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
>> return -EINVAL;
>>
>> else if (cmd == VFIO_DEVICE_SET_IRQS)
>> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
>> index ef76737..2a06035 100644
>> --- a/drivers/vfio/platform/vfio_platform_private.h
>> +++ b/drivers/vfio/platform/vfio_platform_private.h
>> @@ -15,7 +15,29 @@
>> #ifndef VFIO_PLATFORM_PRIVATE_H
>> #define VFIO_PLATFORM_PRIVATE_H
>>
>> +#define VFIO_PLATFORM_OFFSET_SHIFT 40
>> +#define VFIO_PLATFORM_OFFSET_MASK (((u64)(1) << VFIO_PLATFORM_OFFSET_SHIFT) - 1)
>> +
>> +#define VFIO_PLATFORM_OFFSET_TO_INDEX(off) \
>> + (off >> VFIO_PLATFORM_OFFSET_SHIFT)
>> +
>> +#define VFIO_PLATFORM_INDEX_TO_OFFSET(index) \
>> + ((u64)(index) << VFIO_PLATFORM_OFFSET_SHIFT)
>> +
>> +struct vfio_platform_region {
>> + u64 addr;
>> + resource_size_t size;
>> + u32 flags;
>> + u32 type;
>> +#define VFIO_PLATFORM_REGION_TYPE_MMIO 1
>> +#define VFIO_PLATFORM_REGION_TYPE_PIO 2
>> +};
>> +
>> struct vfio_platform_device {
>> + struct vfio_platform_region *regions;
>> + u32 num_regions;
>> + atomic_t refcnt;
>> +
>> /*
>> * These fields should be filled by the bus driver at binding time
>> */
>
>
>
--
Antonios Motakis
Virtual Open Systems
On Wed, 2014-10-22 at 15:54 +0200, Antonios Motakis wrote:
> On Tue, Oct 21, 2014 at 6:34 PM, Alex Williamson
> <[email protected]> wrote:
> > On Mon, 2014-10-13 at 15:10 +0200, Antonios Motakis wrote:
> >> This patch enables the IOCTLs VFIO_DEVICE_GET_REGION_INFO ioctl call,
> >> which allows the user to learn about the available MMIO resources of
> >> a device.
> >>
> >> Signed-off-by: Antonios Motakis <[email protected]>
> >> ---
> >> drivers/vfio/platform/vfio_platform_common.c | 93 +++++++++++++++++++++++++--
> >> drivers/vfio/platform/vfio_platform_private.h | 22 +++++++
> >> 2 files changed, 111 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> >> index 1e4073f..8a7e474 100644
> >> --- a/drivers/vfio/platform/vfio_platform_common.c
> >> +++ b/drivers/vfio/platform/vfio_platform_common.c
> >> @@ -27,17 +27,84 @@
> >>
> >> #include "vfio_platform_private.h"
> >>
> >> +static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
> >> +{
> >> + int cnt = 0, i;
> >> +
> >> + while (vdev->get_resource(vdev, cnt))
> >> + cnt++;
> >> +
> >> + vdev->regions = kcalloc(cnt, sizeof(struct vfio_platform_region),
> >> + GFP_KERNEL);
> >> + if (!vdev->regions)
> >> + return -ENOMEM;
> >> +
> >> + for (i = 0; i < cnt; i++) {
> >> + struct resource *res =
> >> + vdev->get_resource(vdev, i);
> >> +
> >> + if (!res)
> >> + goto err;
> >> +
> >> + vdev->regions[i].addr = res->start;
> >> + vdev->regions[i].size = resource_size(res);
> >> + vdev->regions[i].flags = 0;
> >> +
> >> + switch (resource_type(res)) {
> >> + case IORESOURCE_MEM:
> >> + vdev->regions[i].type = VFIO_PLATFORM_REGION_TYPE_MMIO;
> >> + break;
> >> + case IORESOURCE_IO:
> >> + vdev->regions[i].type = VFIO_PLATFORM_REGION_TYPE_PIO;
> >> + break;
> >
> > Ok, we have support for PIO in platform now (thanks!), does the user
> > know what type of region they're dealing with? Do they care? For PCI
> > the user tests the PCI BAR in config space to determine which type it
> > is. I'm guessing that platform would do something similar against the
> > device tree or ACPI, right?
>
> Maybe is worthwhile to add an explicit flag in
> VFIO_DEVICE_GET_REGION_INFO for PIO regions. For platform devices I
> don't know if we can always rely on DT or ACPI info to be available.
> For VFIO PCI the BAR is always implemented, and while I have proposed
> an RFC to return DT information, I don't think we can assume how a
> device is described in the host, whether DT, ACPI, or dark magic.
Is this already handled by the fact that vfio-platform is not meant to
be a generic meta driver to the same extent as vfio-pci? There is no
self-describing config space on platform devices like there is on PCI,
so the user will need to know in advance somehow what the device is and
what resources/irqs it uses. We do need to make sure though that we
provide a userspace ABI that allows them to match VFIO indexes to the
device in a predictable way. It's not fully clear to me how that works.
Thanks,
Alex
On Wed, Oct 22, 2014 at 6:46 PM, Alex Williamson
<[email protected]> wrote:
> On Wed, 2014-10-22 at 15:54 +0200, Antonios Motakis wrote:
>> On Tue, Oct 21, 2014 at 6:34 PM, Alex Williamson
>> <[email protected]> wrote:
>> > On Mon, 2014-10-13 at 15:10 +0200, Antonios Motakis wrote:
>> >> This patch enables the IOCTLs VFIO_DEVICE_GET_REGION_INFO ioctl call,
>> >> which allows the user to learn about the available MMIO resources of
>> >> a device.
>> >>
>> >> Signed-off-by: Antonios Motakis <[email protected]>
>> >> ---
>> >> drivers/vfio/platform/vfio_platform_common.c | 93 +++++++++++++++++++++++++--
>> >> drivers/vfio/platform/vfio_platform_private.h | 22 +++++++
>> >> 2 files changed, 111 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> >> index 1e4073f..8a7e474 100644
>> >> --- a/drivers/vfio/platform/vfio_platform_common.c
>> >> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> >> @@ -27,17 +27,84 @@
>> >>
>> >> #include "vfio_platform_private.h"
>> >>
>> >> +static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
>> >> +{
>> >> + int cnt = 0, i;
>> >> +
>> >> + while (vdev->get_resource(vdev, cnt))
>> >> + cnt++;
>> >> +
>> >> + vdev->regions = kcalloc(cnt, sizeof(struct vfio_platform_region),
>> >> + GFP_KERNEL);
>> >> + if (!vdev->regions)
>> >> + return -ENOMEM;
>> >> +
>> >> + for (i = 0; i < cnt; i++) {
>> >> + struct resource *res =
>> >> + vdev->get_resource(vdev, i);
>> >> +
>> >> + if (!res)
>> >> + goto err;
>> >> +
>> >> + vdev->regions[i].addr = res->start;
>> >> + vdev->regions[i].size = resource_size(res);
>> >> + vdev->regions[i].flags = 0;
>> >> +
>> >> + switch (resource_type(res)) {
>> >> + case IORESOURCE_MEM:
>> >> + vdev->regions[i].type = VFIO_PLATFORM_REGION_TYPE_MMIO;
>> >> + break;
>> >> + case IORESOURCE_IO:
>> >> + vdev->regions[i].type = VFIO_PLATFORM_REGION_TYPE_PIO;
>> >> + break;
>> >
>> > Ok, we have support for PIO in platform now (thanks!), does the user
>> > know what type of region they're dealing with? Do they care? For PCI
>> > the user tests the PCI BAR in config space to determine which type it
>> > is. I'm guessing that platform would do something similar against the
>> > device tree or ACPI, right?
>>
>> Maybe is worthwhile to add an explicit flag in
>> VFIO_DEVICE_GET_REGION_INFO for PIO regions. For platform devices I
>> don't know if we can always rely on DT or ACPI info to be available.
>> For VFIO PCI the BAR is always implemented, and while I have proposed
>> an RFC to return DT information, I don't think we can assume how a
>> device is described in the host, whether DT, ACPI, or dark magic.
>
> Is this already handled by the fact that vfio-platform is not meant to
> be a generic meta driver to the same extent as vfio-pci? There is no
> self-describing config space on platform devices like there is on PCI,
> so the user will need to know in advance somehow what the device is and
> what resources/irqs it uses. We do need to make sure though that we
> provide a userspace ABI that allows them to match VFIO indexes to the
> device in a predictable way. It's not fully clear to me how that works.
> Thanks,
Yeah, it is a fact of life that the user needs to know what regions he
is accessing, just from their ordering. Even with the extension for
accessing device tree data, the user still needs to know what each
region is and what info he is looking for from the device tree. In
that respect we could delegate the responsibility to the user to just
"know" what kind of device he is accessing and what kind of regions it
features (and in what order).
>
> Alex
>
--
Antonios Motakis
Virtual Open Systems