lkml.org 
[lkml]   [2020]   [Sep]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4 05/10] vfio/fsl-mc: Allow userspace to MMAP fsl-mc device MMIO regions
From
Date
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

\
 
 \ /
  Last update: 2020-09-07 14:52    [W:0.106 / U:0.344 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site