Re: [PATCH v2 5/9] vfio/fsl-mc: Allow userspace to MMAP fsl-mc device MMIO regions

[Diana Craciun OSS <diana.craciun@oss.nxp.com>]

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;