Messages in this thread | | | Subject | Re: [PATCH v2 5/9] vfio/fsl-mc: Allow userspace to MMAP fsl-mc device MMIO regions | From | Diana Craciun OSS <> | Date | Thu, 4 Jun 2020 21:41:26 +0300 |
| |
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;
| |