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

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Jul 15, 2026 at 02:05:35PM +0100, Matthew Auld wrote:
> 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?
> 

+1 - I don't think split would really be a use case and most xe_mmio_gem
usages are likely exactly one page, right?

Matt 

> > ---
> >   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);
>