Re: [PATCH] drm/xe: Invalidate userptr VMA on page pin fault

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

On Mon, Mar 11, 2024 at 02:29:26PM +0100, Thomas Hellström wrote:
> On Mon, 2024-03-11 at 11:55 +0100, Thomas Hellström wrote:
> > Hi, Matthew
> > 
> > On Fri, 2024-03-08 at 13:37 -0800, Matthew Brost wrote:
> > > Rather than return an error to the user or ban the VM when userptr
> > > VMA
> > > page pin fails with -EFAULT, invalidate VMA mappings. This supports
> > > the
> > > UMD use case of freeing userptr while still having bindings.
> > > 
> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_gt_pagefault.c |  4 ++--
> > >  drivers/gpu/drm/xe/xe_trace.h        |  2 +-
> > >  drivers/gpu/drm/xe/xe_vm.c           | 20 +++++++++++++-------
> > >  drivers/gpu/drm/xe/xe_vm_types.h     |  7 ++-----
> > >  4 files changed, 18 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > index 73c535193a98..241c294270d9 100644
> > > --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > @@ -69,7 +69,7 @@ static bool access_is_atomic(enum access_type
> > > access_type)
> > >  static bool vma_is_valid(struct xe_tile *tile, struct xe_vma *vma)
> > >  {
> > >  	return BIT(tile->id) & vma->tile_present &&
> > > -		!(BIT(tile->id) & vma->usm.tile_invalidated);
> > > +		!(BIT(tile->id) & vma->tile_invalidated);
> > >  }
> > >  
> > >  static bool vma_matches(struct xe_vma *vma, u64 page_addr)
> > > @@ -226,7 +226,7 @@ static int handle_pagefault(struct xe_gt *gt,
> > > struct pagefault *pf)
> > >  
> > >  	if (xe_vma_is_userptr(vma))
> > >  		ret =
> > > xe_vma_userptr_check_repin(to_userptr_vma(vma));
> > > -	vma->usm.tile_invalidated &= ~BIT(tile->id);
> > > +	vma->tile_invalidated &= ~BIT(tile->id);
> > >  
> > >  unlock_dma_resv:
> > >  	drm_exec_fini(&exec);
> > > diff --git a/drivers/gpu/drm/xe/xe_trace.h
> > > b/drivers/gpu/drm/xe/xe_trace.h
> > > index 4ddc55527f9a..846f14507d5f 100644
> > > --- a/drivers/gpu/drm/xe/xe_trace.h
> > > +++ b/drivers/gpu/drm/xe/xe_trace.h
> > > @@ -468,7 +468,7 @@ DEFINE_EVENT(xe_vma, xe_vma_userptr_invalidate,
> > >  	     TP_ARGS(vma)
> > >  );
> > >  
> > > -DEFINE_EVENT(xe_vma, xe_vma_usm_invalidate,
> > > +DEFINE_EVENT(xe_vma, xe_vma_invalidate,
> > >  	     TP_PROTO(struct xe_vma *vma),
> > >  	     TP_ARGS(vma)
> > >  );
> > > diff --git a/drivers/gpu/drm/xe/xe_vm.c
> > > b/drivers/gpu/drm/xe/xe_vm.c
> > > index 643b3701a738..9a19044f7ef6 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > @@ -724,11 +724,18 @@ int xe_vm_userptr_pin(struct xe_vm *vm)
> > >  	list_for_each_entry_safe(uvma, next, &vm-
> > > > userptr.repin_list,
> > >  				 userptr.repin_link) {
> > >  		err = xe_vma_userptr_pin_pages(uvma);
> > > -		if (err < 0)
> > > -			return err;
> > > -
> > >  		list_del_init(&uvma->userptr.repin_link);
> > > -		list_move_tail(&uvma->vma.combined_links.rebind,
> > > &vm->rebind_list);
> > > +		if (err == -EFAULT) {
> > > +			err = xe_vm_invalidate_vma(&uvma->vma);
> > 
> > I think we need to check for FAULT_MODE here. If we hit this path in
> > FAULT_MODE, we already have an invalid gpu access and can kill the
> > VM.
> > 

Agree, will fix.

> > In preempt-fence mode, we should probably be calling
> > xe_vm_unbind_vma(), because xe_vm_invalidate_vma() isn't safe to call
> > outside of the mmu_notifier, and if there are still BOOKKEEP fences
> > pending- see the asserts in that function.
> 
> 
> Actually, xe_vm_invalidate_vma() would probably work if we grabbed the
> vm resv and waited for bookkeep fences first, and updated the asserts. 
>

Yes, I think that will work but is that even needed? We have vm->lock
here in write mode which should prevent any further updates to the page
tables. What are we trying prevent a race a against? An in-flight bind
job which touches the same page tables? I guess that is possible.
 
> But then xe_vm_unbind_vma() might still be better since we also clean
> up the page-tables.
> 

I think we do not want to mess with the VMA state as that should only be
changed by user IOCTLs. An invalidation seems to be the right call here.

> /Thomas
> 
> 
> > 
> > > +			if (err)
> > > +				return err;
> > > +		} else {
> > > +			if (err < 0)
> > > +				return err;
> > > +
> > > +			list_move_tail(&uvma-
> > > > vma.combined_links.rebind,
> > > +				       &vm->rebind_list);
> > > +		}
> > >  	}
> > >  
> > >  	return 0;
> > > @@ -3214,9 +3221,8 @@ int xe_vm_invalidate_vma(struct xe_vma *vma)
> > >  	u8 id;
> > >  	int ret;
> > >  
> > > -	xe_assert(xe, xe_vm_in_fault_mode(xe_vma_vm(vma)));
> > >  	xe_assert(xe, !xe_vma_is_null(vma));
> > > -	trace_xe_vma_usm_invalidate(vma);
> > > +	trace_xe_vma_invalidate(vma);
> > >  
> > >  	/* Check that we don't race with page-table updates */
> > >  	if (IS_ENABLED(CONFIG_PROVE_LOCKING)) {
> > > @@ -3254,7 +3260,7 @@ int xe_vm_invalidate_vma(struct xe_vma *vma)
> > >  		}
> > >  	}
> > >  
> > > -	vma->usm.tile_invalidated = vma->tile_mask;
> > > +	vma->tile_invalidated = vma->tile_mask;
> > >  
> > >  	return 0;
> > >  }
> > > diff --git a/drivers/gpu/drm/xe/xe_vm_types.h
> > > b/drivers/gpu/drm/xe/xe_vm_types.h
> > > index 79b5cab57711..ae5fb565f6bf 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> > > @@ -84,11 +84,8 @@ struct xe_vma {
> > >  		struct work_struct destroy_work;
> > >  	};
> > >  
> > > -	/** @usm: unified shared memory state */
> > > -	struct {
> > > -		/** @tile_invalidated: VMA has been invalidated */
> > > -		u8 tile_invalidated;
> > > -	} usm;
> > > +	/** @tile_invalidated: VMA has been invalidated */
> > > +	u8 tile_invalidated;
> > 
> > Add a comment in the commit message about removing the usm struct?

Will add.

Matt

> > /Thomas
> > 
> > 
> > >  
> > >  	/** @tile_mask: Tile mask of where to create binding for
> > > this VMA */
> > >  	u8 tile_mask;
> > 
>