[PATCH 15/17] drm/xe/oa/uapi: OA buffer mmap

[Umesh Nerlige Ramappa]

On Fri, Jan 19, 2024 at 07:11:22PM -0800, Dixit, Ashutosh wrote:
>On Fri, 22 Dec 2023 18:39:14 -0800, Umesh Nerlige Ramappa wrote:
>>
>
>Hi Umesh,
>
>> On Thu, Dec 07, 2023 at 10:43:27PM -0800, Ashutosh Dixit wrote:
>> > Allow the OA buffer to be mmap'd to userspace. This is needed for the MMIO
>> > trigger use case. Even otherwise, with whitelisted OA head/tail ptr
>> > registers, userspace can receive/interpret OA data from the mmap'd buffer
>> > without issuing read()'s on the OA stream fd.
>> >
>> > Suggested-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
>> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
>> > ---
>> > drivers/gpu/drm/xe/xe_oa.c | 53 ++++++++++++++++++++++++++++++++++++++
>> > 1 file changed, 53 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
>> > index 42f32d4359f2c..97779cbb83ee8 100644
>> > --- a/drivers/gpu/drm/xe/xe_oa.c
>> > +++ b/drivers/gpu/drm/xe/xe_oa.c
>> > @@ -898,6 +898,8 @@ static int xe_oa_alloc_oa_buffer(struct xe_oa_stream *stream)
>> >		return PTR_ERR(bo);
>> >
>> >	stream->oa_buffer.bo = bo;
>> > +	/* mmap implementation requires OA buffer to be in system memory */
>> > +	xe_assert(stream->oa->xe, bo->vmap.is_iomem == 0);
>> >	stream->oa_buffer.vaddr = bo->vmap.vaddr;
>> >	return 0;
>> > }
>> > @@ -1174,6 +1176,9 @@ static int xe_oa_release(struct inode *inode, struct file *file)
>> >	struct xe_oa_stream *stream = file->private_data;
>> >	struct xe_gt *gt = stream->gt;
>> >
>> > +	/* Zap mmap's */
>> > +	unmap_mapping_range(file->f_mapping, 0, -1, 1);
>> > +
>> >	mutex_lock(&gt->oa.gt_lock);
>> >	xe_oa_destroy_locked(stream);
>> >	mutex_unlock(&gt->oa.gt_lock);
>> > @@ -1184,6 +1189,53 @@ static int xe_oa_release(struct inode *inode, struct file *file)
>> >	return 0;
>> > }
>> >
>> > +static int xe_oa_mmap(struct file *file, struct vm_area_struct *vma)
>> > +{
>> > +	struct xe_oa_stream *stream = file->private_data;
>> > +	struct xe_bo *bo = stream->oa_buffer.bo;
>> > +	unsigned long start = vma->vm_start;
>> > +	int i, ret;
>> > +
>> > +	if (xe_perf_stream_paranoid && !perfmon_capable()) {
>> > +		drm_dbg(&stream->oa->xe->drm, "Insufficient privilege to map OA buffer\n");
>> > +		return -EACCES;
>> > +	}
>> > +
>> > +	/* Can mmap the entire OA buffer or nothing (no partial OA buffer mmaps) */
>> > +	if (vma->vm_end - vma->vm_start != XE_OA_BUFFER_SIZE) {
>> > +		drm_dbg(&stream->oa->xe->drm, "Wrong mmap size, must be OA buffer size\n");
>> > +		return -EINVAL;
>> > +	}
>> > +
>> > +	/* Only support VM_READ, enforce MAP_PRIVATE by checking for VM_MAYSHARE */
>> > +	if (vma->vm_flags & (VM_WRITE | VM_EXEC | VM_SHARED | VM_MAYSHARE)) {
>> > +		drm_dbg(&stream->oa->xe->drm, "mmap must be read only\n");
>> > +		return -EINVAL;
>> > +	}
>> > +
>> > +	vm_flags_clear(vma, VM_MAYWRITE | VM_MAYEXEC);
>> > +
>> > +	/*
>> > +	 * If the privileged parent forks and child drops root privilege, we do not want
>> > +	 * the child to retain access to the mapped OA buffer. Explicitly set VM_DONTCOPY
>> > +	 * to avoid such cases.
>> > +	 */
>> > +	vm_flags_set(vma, vma->vm_flags | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_DONTCOPY);
>>
>> Would help to just use the vm_flags_mod where you can specify both set and
>> clear flags.
>
>Yes, good idea, done.
>
>>
>> And then just to be paranoid about it, maybe add an assert to check that
>> the flags applied correctly.
>
>Don't think this is needed, code in vm_flags_mod is pretty clear. So
>skipping this.
>
>> Assuming you ran the existing mmap tests for this.
>
>Yes, existing IGT's have been ported to Xe.
>
>> I think we should also add an mremap case. I think that should fail with
>> EINVAL since this is a private mapping.
>
>Ah, mremap description says "mremap() expands (or shrinks) an existing
>memory mapping" so the relevant flag seems to be VM_DONTEXPAND rather than
>VM_SHARED/VM_MAYSHARE. I will see about adding a mremap test.
>
>>
>> > +
>> > +	xe_assert(stream->oa->xe, bo->ttm.ttm->num_pages ==
>> > +		  (vma->vm_end - vma->vm_start) >> PAGE_SHIFT);
>> > +	for (i = 0; i < bo->ttm.ttm->num_pages; i++) {
>> > +		ret = remap_pfn_range(vma, start, page_to_pfn(bo->ttm.ttm->pages[i]),
>> > +				      PAGE_SIZE, vma->vm_page_prot);
>>
>> vma->vm_page_prot is set to the state of vm_flags that existed at the
>> mmap_region() level. We have modified those flags here and we must update
>> the vma_page_prot with vm_get_page_prot(vma->vm_flags).
>
>If we need to do this I think the call to use would be
>vma_set_page_prot(). But, looking at vm_get_page_prot, vm_flags which
>affect vma->vm_page_prot are (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED). And note
>that in this function we have not modified any of these, if
>(VM_WRITE|VM_EXEC|VM_SHARED) is set, we just return error
>(vma->vm_page_prot seems to determine what we write to the PTE's and the
>flags we are modifying here are more like "software" vm_flags).
>
>Also we would see some test failures if we didn't do this correctly, and we
>are not seeing test failures.
>
>So I am a little torn about this, vma_set_page_prot() doesn't seem to be
>really needed, but may be nice to have, but it is also rarely used in the
>kernel.

I think I misread this earlier. I agree with your description, so I am 
okay to leave it as is.

Thanks,
Umesh
>
>I am copying Thomas and let's see what he thinks. I'm skipping adding this
>for now but if Thomas or you say we should add it, I'll go ahead add it.
>
>Thanks.
>--
>Ashutosh