[Intel-gfx] [PATCH 4/4] drm/i915/perf: Map OA buffer to user space for gen12 performance query

[Chris Wilson]

Quoting Umesh Nerlige Ramappa (2020-07-18 01:04:37)
> +static vm_fault_t vm_fault_oa(struct vm_fault *vmf)
> +{
> +       struct vm_area_struct *vma = vmf->vma;
> +       struct i915_perf_stream *stream = vma->vm_private_data;
> +       struct drm_i915_gem_object *obj = stream->oa_buffer.vma->obj;
> +       int err;
> +
> +       err = i915_gem_object_pin_pages(obj);
> +       if (err)
> +               goto out;
> +
> +       err = remap_io_sg(vma,
> +                         vma->vm_start, vma->vm_end - vma->vm_start,
> +                         obj->mm.pages->sgl, -1);
> +
> +       i915_gem_object_unpin_pages(obj);

Ok, basics look good (will handle any vma for which we have valid DMA
addresses, and since we control the construction of the vma we know we
can limit it to objects that work via CPU PTE). There is just one small
catch we have to be wary off -- the CPU pte are not holding a reference
to the pages themselves, and so we need to refault if we have to evict.

However! The oa_buffer is perma-pinned so the pages cannot just disappear,
but the mmap itself may outlive the perf-fd. So we need some way to
determine that the vm_fault_oa() is called on an old stream (e..g if
(stream->closed) return VM_FAULT_SIGBUS;), and during stream close to
call unmap_mmaping_range(perf_file->f_mapping, 0, -1);

Since the mmap may live longer than expected, the vm_ops should on
open() take a reference to the stream, and on close() release that
reference.

Now would be a good time to mmap and close the perf-fd in an igt and
verify the mmap is no longer accessible after the close. (To add
complications, you can fork after opening the mmap(perf-fd).)

> +out:
> +       return i915_error_to_vmf_fault(err);
> +}
> +
> +static const struct vm_operations_struct vm_ops_oa = {
> +       .fault = vm_fault_oa,
> +};
> +
> +int i915_perf_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +       struct i915_perf_stream *stream = file->private_data;
> +       int len;
> +
> +       if (!IS_ALIGNED(vma->vm_start, PAGE_SIZE))
> +               return -EINVAL;
> +
> +       if (vma->vm_end < vma->vm_start)
> +               return -EINVAL;

These strike me as being redundant.

> +       len = vma->vm_end - vma->vm_start;
> +       if (!IS_ALIGNED(len, PAGE_SIZE) || len > OA_BUFFER_SIZE)
> +               return -EINVAL;
> +
> +       if (vma->vm_flags & VM_WRITE)
> +               return -EINVAL;
> +
> +       if (vma->vm_pgoff != I915_PERF_OA_BUFFER_MMAP_OFFSET)
> +               return -EINVAL;

I'd throw this is into a switch and do it earlier, so the len and
writable checks can be based on the oa_vma. (Having a switch makes it
clearer that this doesn't have to be a one-off.)
-Chris