Re: [RFC v2 7/7] drm/xe/svm: Correct memory attribute reset for partial unmap

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

On Wed, Apr 29, 2026 at 10:02:48PM -0700, Matthew Brost wrote:

Sorry double reply...

> On Mon, Apr 06, 2026 at 02:28:30PM +0530, Arvind Yadav wrote:
> > From: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > 
> > When performing a partial unmap of an SVM range, the memory attributes
> > were being reset for the entire range instead of just the portion
> > being unmapped. This could lead to unintended side effects and behaviour.
> > 
> > Fix this by restricting the attribute reset to only the affected subrange
> > that is being unmapped.
> > 
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > Signed-off-by: Arvind Yadav <arvind.yadav@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_svm.c | 56 +++++++++++++++++++++++++++----------
> >  drivers/gpu/drm/xe/xe_svm.h | 10 +++++++
> >  2 files changed, 52 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
> > index 89668ada38ca..f533cddf4d2b 100644
> > --- a/drivers/gpu/drm/xe/xe_svm.c
> > +++ b/drivers/gpu/drm/xe/xe_svm.c
> > @@ -58,6 +58,8 @@ void *xe_svm_private_page_owner(struct xe_vm *vm, bool force_smem)
> >  	return force_smem ? NULL : vm->svm.peer.owner;
> >  }
> >  
> > +#define XE_SVM_ATTR_RETRY_MAX 3
> > +
> >  static bool xe_svm_range_in_vram(struct xe_svm_range *range)
> >  {
> >  	/*
> > @@ -127,15 +129,23 @@ static void xe_svm_range_free(struct drm_gpusvm_range *range)
> >  	kfree(range);
> >  }
> >  
> > +static void xe_svm_range_set_unmapped(struct xe_svm_range *range,
> > +				      const struct mmu_notifier_range *mmu_range)
> > +{
> > +	drm_gpusvm_range_set_unmapped(&range->base, mmu_range);
> > +	if (range->base.pages.flags.partial_unmap) {
> > +		range->partial_unmap.start = max(xe_svm_range_start(range), mmu_range->start);
> > +		range->partial_unmap.end = min(xe_svm_range_end(range), mmu_range->end);
> > +	}
> 
> Can you blindly set range->partial_unmap.start/end here? More below?
> 
> > +}
> > +
> >  static void
> >  xe_svm_garbage_collector_add_range(struct xe_vm *vm, struct xe_svm_range *range,
> >  				   const struct mmu_notifier_range *mmu_range)
> >  {
> >  	struct xe_device *xe = vm->xe;
> >  
> > -	range_debug(range, "GARBAGE COLLECTOR ADD");
> 
> I don't think you should delete the above debug statement.
> 
> > -
> > -	drm_gpusvm_range_set_unmapped(&range->base, mmu_range);
> > +	xe_svm_range_set_unmapped(range, mmu_range);
> >  
> >  	spin_lock(&vm->svm.garbage_collector.lock);
> >  	if (list_empty(&range->garbage_collector_link))
> > @@ -380,9 +390,10 @@ static int xe_svm_range_set_default_attr(struct xe_vm *vm, u64 start, u64 end)
> >  static int xe_svm_garbage_collector(struct xe_vm *vm)
> >  {
> >  	struct xe_svm_range *range;
> > -	u64 range_start;
> > -	u64 range_end;
> > +	u64 unmap_start;
> > +	u64 unmap_end;
> >  	int err, ret = 0;
> > +	int retry_count;
> >  
> >  	lockdep_assert_held_write(&vm->lock);
> >  
> > @@ -397,8 +408,13 @@ static int xe_svm_garbage_collector(struct xe_vm *vm)
> >  		if (!range)
> >  			break;
> >  
> > -		range_start = xe_svm_range_start(range);
> > -		range_end = xe_svm_range_end(range);
> > +		if (range->base.pages.flags.partial_unmap) {
> > +			unmap_start = range->partial_unmap.start;
> > +			unmap_end = range->partial_unmap.end;
> 
> Then here blindly use range->partial_unmap.start/end?
> 
> Matt
> 
> > +		} else {
> > +			unmap_start = xe_svm_range_start(range);
> > +			unmap_end = xe_svm_range_end(range);
> > +		}
> >  
> >  		list_del(&range->garbage_collector_link);
> >  		spin_unlock(&vm->svm.garbage_collector.lock);
> > @@ -412,13 +428,25 @@ static int xe_svm_garbage_collector(struct xe_vm *vm)
> >  			return err;
> >  		}
> >  
> > -		err = xe_svm_range_set_default_attr(vm, range_start, range_end);
> > -		if (err) {
> > -			if (err == -EAGAIN)
> > -				ret = -EAGAIN;
> > -			else
> > -				return err;
> > -		}
> > +		/*
> > +		 * Retry set_default_attr on -EAGAIN (VMA was recreated).
> > +		 * Limit retries to prevent infinite loop.
> > +		 */
> > +		retry_count = 0;
> > +
> > +		do {
> > +			err = xe_svm_range_set_default_attr(vm, unmap_start, unmap_end);
> > +			if (err == -EAGAIN && ++retry_count > XE_SVM_ATTR_RETRY_MAX) {
> > +				drm_err(&vm->xe->drm,
> > +					"SET_ATTR retry limit exceeded for [0x%llx-0x%llx]\n",
> > +					unmap_start, unmap_end);
> > +				xe_vm_kill(vm, true);
> > +				return -EIO;
> > +			}
> > +		} while (err == -EAGAIN);
> > +
> > +		if (err)
> > +			return err;
> >  	}
> >  	spin_unlock(&vm->svm.garbage_collector.lock);
> >  
> > diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h
> > index b7b8eeacf196..4651e044cf53 100644
> > --- a/drivers/gpu/drm/xe/xe_svm.h
> > +++ b/drivers/gpu/drm/xe/xe_svm.h
> > @@ -46,6 +46,16 @@ struct xe_svm_range {
> >  	 * range. Protected by GPU SVM notifier lock.
> >  	 */
> >  	u8 tile_invalidated;
> > +	/**
> > +	 * @partial_unmap: Structure to hold partial unmap range info.
> > +	 * Valid only if partial unmap is in effect.
> > +	 */
> > +	struct {
> > +		/** @start: Start address of the partial unmap range */
> > +		u64 start;
> > +		/** @end: End address of the partial unmap range */
> > +		u64 end;
> > +	} partial_unmap;

Also it is a bit of shame to add 2 extra QWs in storage here...

We likely can find some bits in drm_gpusvm_pages, drm_gpusvm_range which
are no longer relavent after unmap to create a union with start / end.

Below are two candiates which I believe after
'drm_gpusvm_range_set_unmapped' would be unused.

147 struct drm_gpusvm_pages {
148         struct drm_pagemap_addr *dma_addr;
149         struct drm_pagemap *dpagemap;

This is micro-optimization thing so could be deferred but also perhaps
makes sense to normalize partial unmaps now at gpusvm level.

Matt

> >  };
> >  
> >  /**
> > -- 
> > 2.43.0
> >