Re: [PATCH v4 05/10] vfio/fsl-mc: Allow userspace to MMAP fsl-mc device MMIO regions

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

Hi Eric,

On 9/3/2020 7:05 PM, Auger Eric wrote:
> Hi Diana,
> 
> On 8/26/20 11:33 AM, Diana Craciun 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 +++++++++++++++++++++++++++++--
>>   1 file changed, 58 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>> index 093b8d68496c..64d5c1fff51f 100644
>> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>> @@ -33,7 +33,8 @@ static int vfio_fsl_mc_regions_init(struct vfio_fsl_mc_device *vdev)
>>   
>>   		vdev->regions[i].addr = res->start;
>>   		vdev->regions[i].size = resource_size(res);
>> -		vdev->regions[i].flags = 0;
>> +		vdev->regions[i].flags = VFIO_REGION_INFO_FLAG_MMAP;
> Is the region always mmappable or does it depend on the
> mc_dev->regions[i].flags. Also on VFIO platform we checked some
> alignment addr/size constraints.

The region is mmappable regardless of the region flags. However, I see 
that there are some questions regarding the fact that the regions are 
always mmappable in the following patches, so I'll try to clarify here.

The way the userspace communicates with the hardware is through some 
commands (special data written in the device region). The commands can 
be issued using special channels (devices): dprc and dpmcp. Most of the 
commands can be passthrough, the command that configures the interrupts 
is the most important example. In order to reduce the complexity, the 
command which configures the interrupts was restricted from the firmware 
to be issued only on a single type of device (dprc). However, in the 
current implementation the memory region corresponding to the dpcr can 
be passthorugh as well. The reason is that it can be used (for example) 
by a DPDK application in the userspace. The DPDK application configures 
the interrupts using the VFIO_DEVICE_SET_IRQS system call. When it comes 
to virtual machines the dprc and the dpmc will be emulated in QEMU.

Regarding the alignemnet/size constraints I agree, I will add some checks.

>> +		vdev->regions[i].type = mc_dev->regions[i].flags & IORESOURCE_BITS;
>>   	}
>>   
>>   	vdev->num_regions = mc_dev->obj_desc.region_count;
>> @@ -164,9 +165,64 @@ 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;
>> +	u8 region_cacheable;
>> +
>> +	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;
>> +
>> +	region_cacheable = (region.type & FSL_MC_REGION_CACHEABLE) &&
>> +			   (region.type & FSL_MC_REGION_SHAREABLE);
> I see in fsl-mc-bus.c that IORESOURCE_CACHEABLE and IORESOURCE_MEM are
> set on the regions flag?

Yes, initially the two flags were set (IORESOURCE_CACHEABLE and 
IORESOURCE_MEM). However, I have changed the behaviour a little bit (it 
was wrong in the past) and IORESOURCE_MEM is still present, but also 
FSL_MC_REGION_CACHEABLE and FSL_MC_REGION_SHAREABLE which are retrieved 
from the firmware through commands.

>> +	if (!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 = {
>>
> Thanks
> 
> Eric
> 

Thanks,
Diana