Allow userspace to mmap device regions for direct access of
fsl-mc devices.
Signed-off-by: Bharat Bhushan <[email protected]>
Signed-off-by: Diana Craciun <[email protected]>
---
drivers/vfio/fsl-mc/vfio_fsl_mc.c | 60 ++++++++++++++++++++++-
drivers/vfio/fsl-mc/vfio_fsl_mc_private.h | 2 +
2 files changed, 60 insertions(+), 2 deletions(-)
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
index c162fa27c02c..a92c6c97c29a 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
@@ -33,7 +33,11 @@ static int vfio_fsl_mc_regions_init(struct vfio_fsl_mc_device *vdev)
vdev->regions[i].addr = res->start;
vdev->regions[i].size = PAGE_ALIGN((resource_size(res)));
- vdev->regions[i].flags = 0;
+ vdev->regions[i].flags = VFIO_REGION_INFO_FLAG_MMAP;
+ vdev->regions[i].flags |= VFIO_REGION_INFO_FLAG_READ;
+ if (!(mc_dev->regions[i].flags & IORESOURCE_READONLY))
+ vdev->regions[i].flags |= VFIO_REGION_INFO_FLAG_WRITE;
+ vdev->regions[i].type = mc_dev->regions[i].flags & IORESOURCE_BITS;
}
vdev->num_regions = mc_dev->obj_desc.region_count;
@@ -164,9 +168,61 @@ static ssize_t vfio_fsl_mc_write(void *device_data, const char __user *buf,
return -EINVAL;
}
+static int vfio_fsl_mc_mmap_mmio(struct vfio_fsl_mc_region region,
+ struct vm_area_struct *vma)
+{
+ u64 size = vma->vm_end - vma->vm_start;
+ u64 pgoff, base;
+
+ pgoff = vma->vm_pgoff &
+ ((1U << (VFIO_FSL_MC_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
+ base = pgoff << PAGE_SHIFT;
+
+ if (region.size < PAGE_SIZE || base + size > region.size)
+ return -EINVAL;
+
+ if (!(region.type & VFIO_DPRC_REGION_CACHEABLE))
+ vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+
+ vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;
+
+ return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
+ size, vma->vm_page_prot);
+}
+
static int vfio_fsl_mc_mmap(void *device_data, struct vm_area_struct *vma)
{
- return -EINVAL;
+ struct vfio_fsl_mc_device *vdev = device_data;
+ struct fsl_mc_device *mc_dev = vdev->mc_dev;
+ int index;
+
+ index = vma->vm_pgoff >> (VFIO_FSL_MC_OFFSET_SHIFT - PAGE_SHIFT);
+
+ if (vma->vm_end < vma->vm_start)
+ return -EINVAL;
+ if (vma->vm_start & ~PAGE_MASK)
+ return -EINVAL;
+ if (vma->vm_end & ~PAGE_MASK)
+ return -EINVAL;
+ if (!(vma->vm_flags & VM_SHARED))
+ return -EINVAL;
+ if (index >= vdev->num_regions)
+ return -EINVAL;
+
+ if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_MMAP))
+ return -EINVAL;
+
+ if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_READ)
+ && (vma->vm_flags & VM_READ))
+ return -EINVAL;
+
+ if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_WRITE)
+ && (vma->vm_flags & VM_WRITE))
+ return -EINVAL;
+
+ vma->vm_private_data = mc_dev;
+
+ return vfio_fsl_mc_mmap_mmio(vdev->regions[index], vma);
}
static const struct vfio_device_ops vfio_fsl_mc_ops = {
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
index 818dfd3df4db..89d2e2a602d8 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
@@ -15,6 +15,8 @@
#define VFIO_FSL_MC_INDEX_TO_OFFSET(index) \
((u64)(index) << VFIO_FSL_MC_OFFSET_SHIFT)
+#define VFIO_DPRC_REGION_CACHEABLE 0x00000001
+
struct vfio_fsl_mc_region {
u32 flags;
u32 type;
--
2.17.1
On Fri, 8 May 2020 10:20:35 +0300
Diana Craciun <[email protected]> wrote:
> Allow userspace to mmap device regions for direct access of
> fsl-mc devices.
>
> Signed-off-by: Bharat Bhushan <[email protected]>
> Signed-off-by: Diana Craciun <[email protected]>
> ---
> drivers/vfio/fsl-mc/vfio_fsl_mc.c | 60 ++++++++++++++++++++++-
> drivers/vfio/fsl-mc/vfio_fsl_mc_private.h | 2 +
> 2 files changed, 60 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> index c162fa27c02c..a92c6c97c29a 100644
> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> @@ -33,7 +33,11 @@ static int vfio_fsl_mc_regions_init(struct vfio_fsl_mc_device *vdev)
>
> vdev->regions[i].addr = res->start;
> vdev->regions[i].size = PAGE_ALIGN((resource_size(res)));
> - vdev->regions[i].flags = 0;
> + vdev->regions[i].flags = VFIO_REGION_INFO_FLAG_MMAP;
> + vdev->regions[i].flags |= VFIO_REGION_INFO_FLAG_READ;
> + if (!(mc_dev->regions[i].flags & IORESOURCE_READONLY))
> + vdev->regions[i].flags |= VFIO_REGION_INFO_FLAG_WRITE;
I'm a little confused that we advertise read and write here, but it's
only relative to the mmap and even later in the series where we add
read and write callback support, it's only for the dprc and dpmcp
devices. Doesn't this leave dpaa2 accelerator devices with only mmap
access? vfio doesn't really have a way to specify that a device only
has mmap access and the read/write interfaces can be quite useful when
debugging or tracing.
> + vdev->regions[i].type = mc_dev->regions[i].flags & IORESOURCE_BITS;
> }
>
> vdev->num_regions = mc_dev->obj_desc.region_count;
> @@ -164,9 +168,61 @@ static ssize_t vfio_fsl_mc_write(void *device_data, const char __user *buf,
> return -EINVAL;
> }
>
> +static int vfio_fsl_mc_mmap_mmio(struct vfio_fsl_mc_region region,
> + struct vm_area_struct *vma)
> +{
> + u64 size = vma->vm_end - vma->vm_start;
> + u64 pgoff, base;
> +
> + pgoff = vma->vm_pgoff &
> + ((1U << (VFIO_FSL_MC_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
> + base = pgoff << PAGE_SHIFT;
> +
> + if (region.size < PAGE_SIZE || base + size > region.size)
We've already aligned region.size up to PAGE_SIZE, so that test can't
be true. Whether it was a good idea to do that alignment, I'm not so
sure.
> + return -EINVAL;
> +
> + if (!(region.type & VFIO_DPRC_REGION_CACHEABLE))
> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> +
> + vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;
> +
> + return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
> + size, vma->vm_page_prot);
> +}
> +
> static int vfio_fsl_mc_mmap(void *device_data, struct vm_area_struct *vma)
> {
> - return -EINVAL;
> + struct vfio_fsl_mc_device *vdev = device_data;
> + struct fsl_mc_device *mc_dev = vdev->mc_dev;
> + int index;
> +
> + index = vma->vm_pgoff >> (VFIO_FSL_MC_OFFSET_SHIFT - PAGE_SHIFT);
> +
> + if (vma->vm_end < vma->vm_start)
> + return -EINVAL;
> + if (vma->vm_start & ~PAGE_MASK)
> + return -EINVAL;
> + if (vma->vm_end & ~PAGE_MASK)
> + return -EINVAL;
> + if (!(vma->vm_flags & VM_SHARED))
> + return -EINVAL;
> + if (index >= vdev->num_regions)
> + return -EINVAL;
> +
> + if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_MMAP))
> + return -EINVAL;
> +
> + if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_READ)
> + && (vma->vm_flags & VM_READ))
> + return -EINVAL;
> +
> + if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_WRITE)
> + && (vma->vm_flags & VM_WRITE))
> + return -EINVAL;
> +
> + vma->vm_private_data = mc_dev;
> +
> + return vfio_fsl_mc_mmap_mmio(vdev->regions[index], vma);
> }
>
> static const struct vfio_device_ops vfio_fsl_mc_ops = {
> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
> index 818dfd3df4db..89d2e2a602d8 100644
> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
> @@ -15,6 +15,8 @@
> #define VFIO_FSL_MC_INDEX_TO_OFFSET(index) \
> ((u64)(index) << VFIO_FSL_MC_OFFSET_SHIFT)
>
> +#define VFIO_DPRC_REGION_CACHEABLE 0x00000001
There appears to be some sort of magic mapping of this to bus specific
bits in the IORESOURCE_BITS range. If the bus specific bits get
shifted we'll be subtly broken here. Can't we use the bus #define so
that we can't get out of sync? Thanks,
Alex
> +
> struct vfio_fsl_mc_region {
> u32 flags;
> u32 type;
On 6/2/2020 7:12 AM, Alex Williamson wrote:
> On Fri, 8 May 2020 10:20:35 +0300
> Diana Craciun <[email protected]> wrote:
>
>> Allow userspace to mmap device regions for direct access of
>> fsl-mc devices.
>>
>> Signed-off-by: Bharat Bhushan <[email protected]>
>> Signed-off-by: Diana Craciun <[email protected]>
>> ---
>> drivers/vfio/fsl-mc/vfio_fsl_mc.c | 60 ++++++++++++++++++++++-
>> drivers/vfio/fsl-mc/vfio_fsl_mc_private.h | 2 +
>> 2 files changed, 60 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>> index c162fa27c02c..a92c6c97c29a 100644
>> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>> @@ -33,7 +33,11 @@ static int vfio_fsl_mc_regions_init(struct vfio_fsl_mc_device *vdev)
>>
>> vdev->regions[i].addr = res->start;
>> vdev->regions[i].size = PAGE_ALIGN((resource_size(res)));
>> - vdev->regions[i].flags = 0;
>> + vdev->regions[i].flags = VFIO_REGION_INFO_FLAG_MMAP;
>> + vdev->regions[i].flags |= VFIO_REGION_INFO_FLAG_READ;
>> + if (!(mc_dev->regions[i].flags & IORESOURCE_READONLY))
>> + vdev->regions[i].flags |= VFIO_REGION_INFO_FLAG_WRITE;
>
> I'm a little confused that we advertise read and write here, but it's
> only relative to the mmap
OK, I will fix that.
> and even later in the series where we add
> read and write callback support, it's only for the dprc and dpmcp
> devices. Doesn't this leave dpaa2 accelerator devices with only mmap
> access? vfio doesn't really have a way to specify that a device only
> has mmap access and the read/write interfaces can be quite useful when
> debugging or tracing.
I do not see any reason of not implementing read/write interface for all
the dpaa2 accelerator devices. I will do that in the next version.
>
>> + vdev->regions[i].type = mc_dev->regions[i].flags & IORESOURCE_BITS;
>> }
>>
>> vdev->num_regions = mc_dev->obj_desc.region_count;
>> @@ -164,9 +168,61 @@ static ssize_t vfio_fsl_mc_write(void *device_data, const char __user *buf,
>> return -EINVAL;
>> }
>>
>> +static int vfio_fsl_mc_mmap_mmio(struct vfio_fsl_mc_region region,
>> + struct vm_area_struct *vma)
>> +{
>> + u64 size = vma->vm_end - vma->vm_start;
>> + u64 pgoff, base;
>> +
>> + pgoff = vma->vm_pgoff &
>> + ((1U << (VFIO_FSL_MC_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
>> + base = pgoff << PAGE_SHIFT;
>> +
>> + if (region.size < PAGE_SIZE || base + size > region.size)
> We've already aligned region.size up to PAGE_SIZE, so that test can't
> be true. Whether it was a good idea to do that alignment, I'm not so
OK, I will come back with a resolution on this matter.
> sure.
>
>> + return -EINVAL;
>> +
>> + if (!(region.type & VFIO_DPRC_REGION_CACHEABLE))
>> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>> +
>> + vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;
>> +
>> + return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
>> + size, vma->vm_page_prot);
>> +}
>> +
>> static int vfio_fsl_mc_mmap(void *device_data, struct vm_area_struct *vma)
>> {
>> - return -EINVAL;
>> + struct vfio_fsl_mc_device *vdev = device_data;
>> + struct fsl_mc_device *mc_dev = vdev->mc_dev;
>> + int index;
>> +
>> + index = vma->vm_pgoff >> (VFIO_FSL_MC_OFFSET_SHIFT - PAGE_SHIFT);
>> +
>> + if (vma->vm_end < vma->vm_start)
>> + return -EINVAL;
>> + if (vma->vm_start & ~PAGE_MASK)
>> + return -EINVAL;
>> + if (vma->vm_end & ~PAGE_MASK)
>> + return -EINVAL;
>> + if (!(vma->vm_flags & VM_SHARED))
>> + return -EINVAL;
>> + if (index >= vdev->num_regions)
>> + return -EINVAL;
>> +
>> + if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_MMAP))
>> + return -EINVAL;
>> +
>> + if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_READ)
>> + && (vma->vm_flags & VM_READ))
>> + return -EINVAL;
>> +
>> + if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_WRITE)
>> + && (vma->vm_flags & VM_WRITE))
>> + return -EINVAL;
>> +
>> + vma->vm_private_data = mc_dev;
>> +
>> + return vfio_fsl_mc_mmap_mmio(vdev->regions[index], vma);
>> }
>>
>> static const struct vfio_device_ops vfio_fsl_mc_ops = {
>> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>> index 818dfd3df4db..89d2e2a602d8 100644
>> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
>> @@ -15,6 +15,8 @@
>> #define VFIO_FSL_MC_INDEX_TO_OFFSET(index) \
>> ((u64)(index) << VFIO_FSL_MC_OFFSET_SHIFT)
>>
>> +#define VFIO_DPRC_REGION_CACHEABLE 0x00000001
>
> There appears to be some sort of magic mapping of this to bus specific
> bits in the IORESOURCE_BITS range. If the bus specific bits get
> shifted we'll be subtly broken here. Can't we use the bus #define so
> that we can't get out of sync? Thanks,
OK, I will use the bus define for these bits.
Thanks,
Diana
>
> Alex
>
>
>> +
>> struct vfio_fsl_mc_region {
>> u32 flags;
>> u32 type;