Re: [PATCH v2 2/5] drm/xe/mmio_gem: fix fault handling for split VMA

[Matthew Auld <matthew.auld@intel.com>]

Hi,

On 26/05/2026 13:51, Ilia Levi wrote:
> The fault handler currently assumes it always operates on a VMA spanning
> the entire GEM object. This does not hold when the VMA has been split,
> e.g. by a partial munmap or mprotect. In that case the handler may map
> wrong physical pages or cause SIGBUS.
> 
> Change the fault handler to map only the GEM subrange corresponding to
> the VMA, and do not set vm_pgoff to zero. Many DRM drivers do this
> because helpers like dma_mmap_pages() interpret vm_pgoff as an
> intra-buffer page offset; leaving the DRM fake offset there would break
> these helpers. Those drivers can get away with zeroing it because they
> map eagerly -- all PTEs are established before mmap returns, so vm_pgoff
> is never consulted again. This driver does not use such helpers and
> defers mapping to the fault handler, where vm_pgoff must be preserved:
> when the kernel splits a VMA it adjusts vm_pgoff, and the fault handler
> subtracts the GEM object's fake mmap offset to recover the page offset
> within the object.
> 
> Fixes: 1ffcf8b8ae8a ("drm/xe: Support for mmap-ing mmio regions")
> Assisted-by: GitHub-Copilot:claude-opus-4.6
> Signed-off-by: Ilia Levi <ilia.levi@intel.com>

Would it work if we did something like:

+static int xe_mmio_gem_vm_may_split(struct vm_area_struct *vma, 
unsigned long addr)
+{
+       return -EINVAL;
+}
+
  static const struct vm_operations_struct vm_ops = {
         .open = drm_gem_vm_open,
         .close = drm_gem_vm_close,
         .fault = xe_mmio_gem_vm_fault,
+       .may_split = xe_mmio_gem_vm_may_split,
  };

?

I don't think partial unmap or similar is really a real use case for 
this type of special mapping. IMO if we can just reject that would be 
simplest? What do you think here?

> ---
>   drivers/gpu/drm/xe/xe_mmio_gem.c | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_mmio_gem.c b/drivers/gpu/drm/xe/xe_mmio_gem.c
> index c22a38e5616b..15e884ad3f1c 100644
> --- a/drivers/gpu/drm/xe/xe_mmio_gem.c
> +++ b/drivers/gpu/drm/xe/xe_mmio_gem.c
> @@ -37,6 +37,7 @@ static vm_fault_t xe_mmio_gem_vm_fault(struct vm_fault *);
>   struct xe_mmio_gem {
>   	struct drm_gem_object base;
>   	phys_addr_t phys_addr;
> +	unsigned long pgoff;
>   };
>   
>   static const struct vm_operations_struct vm_ops = {
> @@ -92,6 +93,8 @@ struct xe_mmio_gem *xe_mmio_gem_create(struct xe_device *xe, struct drm_file *fi
>   	if (err)
>   		goto free_gem;
>   
> +	obj->pgoff = drm_vma_node_start(&base->vma_node);
> +
>   	err = drm_vma_node_allow(&base->vma_node, file);
>   	if (err)
>   		goto free_gem;
> @@ -147,8 +150,6 @@ static int xe_mmio_gem_mmap(struct drm_gem_object *base, struct vm_area_struct *
>   	if ((vma->vm_flags & VM_SHARED) == 0)
>   		return -EINVAL;
>   
> -	/* Set vm_pgoff (used as a fake buffer offset by DRM) to 0 */
> -	vma->vm_pgoff = 0;
>   	vma->vm_page_prot = pgprot_noncached(vm_get_page_prot(vma->vm_flags));
>   	vm_flags_set(vma, VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP |
>   		     VM_DONTCOPY | VM_NORESERVE);
> @@ -190,7 +191,8 @@ static vm_fault_t xe_mmio_gem_vm_fault(struct vm_fault *vmf)
>   	struct xe_mmio_gem *obj = to_xe_mmio_gem(base);
>   	struct drm_device *dev = base->dev;
>   	vm_fault_t ret = VM_FAULT_NOPAGE;
> -	unsigned long i;
> +	unsigned long addr, pfn;
> +	unsigned long pgoff;
>   	int idx;
>   
>   	if (!drm_dev_enter(dev, &idx)) {
> @@ -203,13 +205,15 @@ static vm_fault_t xe_mmio_gem_vm_fault(struct vm_fault *vmf)
>   		return xe_mmio_gem_vm_fault_dummy_page(vmf);
>   	}
>   
> -	for (i = 0; i < base->size; i += PAGE_SIZE) {
> -		unsigned long addr = vma->vm_start + i;
> -		unsigned long phys_addr = obj->phys_addr + i;
> +	pgoff = vma->vm_pgoff - obj->pgoff;
> +	pfn = PHYS_PFN(obj->phys_addr) + pgoff;
>   
> -		ret = vmf_insert_pfn(vma, addr, PHYS_PFN(phys_addr));
> +	for (addr = vma->vm_start; addr < vma->vm_end; addr += PAGE_SIZE) {
> +		ret = vmf_insert_pfn(vma, addr, pfn);
>   		if (ret & VM_FAULT_ERROR)
>   			break;
> +
> +		pfn++;
>   	}
>   
>   	drm_dev_exit(idx);