Re: [PATCH v4 08/20] drm/xe/svm: Add xe_svm_ranges_zap_ptes_in_range() for PTE zapping

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 23-06-2025 10:26, Matthew Brost wrote:
> On Fri, Jun 13, 2025 at 06:25:46PM +0530, Himal Prasad Ghimiray wrote:
>> Introduce xe_svm_ranges_zap_ptes_in_range(), a function to zap page table
>> entries (PTEs) for all SVM ranges within a user-specified address range.
>>
>> -v2 (Matthew Brost)
>> Lock should be called even for tlb_invalidation
>>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_pt.c  | 14 ++++++++++++-
>>   drivers/gpu/drm/xe/xe_svm.c | 42 +++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_svm.h |  7 +++++++
>>   3 files changed, 62 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
>> index 9177c571689e..9a390ef10852 100644
>> --- a/drivers/gpu/drm/xe/xe_pt.c
>> +++ b/drivers/gpu/drm/xe/xe_pt.c
>> @@ -950,7 +950,19 @@ bool xe_pt_zap_ptes_range(struct xe_tile *tile, struct xe_vm *vm,
>>   	struct xe_pt *pt = vm->pt_root[tile->id];
>>   	u8 pt_mask = (range->tile_present & ~range->tile_invalidated);
>>   
>> -	xe_svm_assert_in_notifier(vm);
>> +	/*
>> +	 * Locking rules:
>> +	 *
>> +	 * - notifier_lock (write): full protection against page table changes
>> +	 *   and MMU notifier invalidations.
>> +	 *
>> +	 * - notifier_lock (read) + vm_lock (write): combined protection against
>> +	 *   invalidations and concurrent page table modifications. (e.g., madvise)
>> +	 *
>> +	 */
>> +	lockdep_assert(lockdep_is_held_type(&vm->svm.gpusvm.notifier_lock, 0) ||
>> +		       (lockdep_is_held_type(&vm->svm.gpusvm.notifier_lock, 1) &&
>> +		       lockdep_is_held_type(&vm->lock, 0)));
>>   
>>   	if (!(pt_mask & BIT(tile->id)))
>>   		return false;
>> diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
>> index 2fbbd6a604ea..19420635f1fa 100644
>> --- a/drivers/gpu/drm/xe/xe_svm.c
>> +++ b/drivers/gpu/drm/xe/xe_svm.c
>> @@ -999,6 +999,48 @@ int xe_svm_range_get_pages(struct xe_vm *vm, struct xe_svm_range *range,
>>   	return err;
>>   }
>>   
>> +/**
>> + * xe_svm_ranges_zap_ptes_in_range - clear ptes of svm ranges in input range
>> + * @vm: Pointer to the xe_vm structure
>> + * @start: Start of the input range
>> + * @end: End of the input range
>> + *
>> + * This function removes the page table entries (PTEs) associated
>> + * with the svm ranges within the given input start and end
>> + *
>> + * Return: tile_mask for which gt's need to be tlb invalidated.
>> + */
>> +u8 xe_svm_ranges_zap_ptes_in_range(struct xe_vm *vm, u64 start, u64 end)
>> +{
>> +	struct drm_gpusvm_notifier *notifier;
>> +	struct xe_svm_range *range;
>> +	u64 adj_start, adj_end;
>> +	struct xe_tile *tile;
>> +	u8 tile_mask = 0;
>> +	u8 id;
>> +
>> +	lockdep_assert(lockdep_is_held_type(&vm->svm.gpusvm.notifier_lock, 1) &&
>> +		       lockdep_is_held_type(&vm->lock, 0));
>> +
>> +	drm_gpusvm_for_each_notifier(notifier, &vm->svm.gpusvm, start, end) {
>> +		struct drm_gpusvm_range *r = NULL;
>> +
>> +		adj_start = max(start, notifier->itree.start);
> 
> s/notifier->itree.start/drm_gpusvm_notifier_start
> 
>> +		adj_end = min(end, notifier->itree.last + 1);
> 
> s/notifier->itree.last + 1/drm_gpusvm_notifier_end
> 
>> +		drm_gpusvm_for_each_range(r, notifier, adj_start, adj_end) {
>> +			range = to_xe_range(r);
>> +			for_each_tile(tile, vm->xe, id) {
>> +				if (xe_pt_zap_ptes_range(tile, vm, range)) {
>> +					tile_mask |= BIT(id);
>> +					range->tile_invalidated |= BIT(id);
> 
> /* WRITE_ONCE pairs with READ_ONCE in xe_vm_has_valid_gpu_mapping() */
> WRITE_ONCE(range->tile_invalidated, range->tile_invalidated | BIT(id));

Sure

> 
> Also, we need to be careful here. If we can fail after this point but
> before the TLB invalidation completes, we could break the notifier, as
> the notifier would skip the TLB invalidation. The code, as written, can
> only fail if the CT channel is down — in that case, all bets are off and
> we are issuing a GT reset. So, I think the code as written is okay, but
> I’d add a comment here indicating that there must be no failure points
> between setting tile_invalidated and issuing the TLB invalidation.

Will add a comment.

> 
> Matt
> 
>> +				}
>> +			}
>> +		}
>> +	}
>> +
>> +	return tile_mask;
>> +}
>> +
>>   #if IS_ENABLED(CONFIG_DRM_XE_DEVMEM_MIRROR)
>>   
>>   static struct drm_pagemap_device_addr
>> diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h
>> index 19ce4f2754a7..af8f285b6caa 100644
>> --- a/drivers/gpu/drm/xe/xe_svm.h
>> +++ b/drivers/gpu/drm/xe/xe_svm.h
>> @@ -91,6 +91,7 @@ bool xe_svm_range_validate(struct xe_vm *vm,
>>   
>>   u64 xe_svm_find_vma_start(struct xe_vm *vm, u64 addr, u64 end,  struct xe_vma *vma);
>>   
>> +u8 xe_svm_ranges_zap_ptes_in_range(struct xe_vm *vm, u64 start, u64 end);
>>   /**
>>    * xe_svm_range_has_dma_mapping() - SVM range has DMA mapping
>>    * @range: SVM range
>> @@ -305,6 +306,12 @@ u64 xe_svm_find_vma_start(struct xe_vm *vm, u64 addr, u64 end, struct xe_vma *vm
>>   	return ULONG_MAX;
>>   }
>>   
>> +static inline
>> +u8 xe_svm_ranges_zap_ptes_in_range(struct xe_vm *vm, u64 start, u64 end)
>> +{
>> +	return 0;
>> +}
>> +
>>   #define xe_svm_assert_in_notifier(...) do {} while (0)
>>   #define xe_svm_range_has_dma_mapping(...) false
>>   
>> -- 
>> 2.34.1
>>