lkml.org 
[lkml]   [2020]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 5/9] vfio/fsl-mc: Allow userspace to MMAP fsl-mc device MMIO regions
From
Date
On 6/2/2020 7:12 AM, Alex Williamson wrote:
> On Fri, 8 May 2020 10:20:35 +0300
> Diana Craciun <diana.craciun@oss.nxp.com> wrote:
>
>> Allow userspace to mmap device regions for direct access of
>> fsl-mc devices.
>>
>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@nxp.com>
>> Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com>
>> ---
>> 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;

\
 
 \ /
  Last update: 2020-06-04 20:42    [W:0.079 / U:0.432 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site