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

[Diana Craciun OSS]

On 6/2/2020 7:12 AM, Alex Williamson wrote:
> On Fri,  8 May 2020 10:20:35 +0300
> Diana Craciun <diana.craciun@xxxxxxxxxxx> wrote:
>
> > Allow userspace to mmap device regions for direct access of
> > fsl-mc devices.
> >
> > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@xxxxxxx>
> > Signed-off-by: Diana Craciun <diana.craciun@xxxxxxxxxxx>
> > ---
> >   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;