Re: [PATCH] drm/amdkfd: preserve the byte address when removing retry faults

[Kuehling, Felix]

On 2026-11-08 04:15, Aveline Noir wrote:
> svm_range_restore_pages() receives a CPU page number, but passes it
> unchanged to amdgpu_gmc_filter_faults_remove() when recovery is deferred
> or returns -EAGAIN. The software fault filter uses a byte address and
> PASID as its key, so this can leave the fault filtered until the entry
> ages out.
>
> Pass the original byte address to svm_range_restore_pages() and derive
> the CPU page number locally for range operations. Use the original
> address for both filter removals. This also preserves the GPU subpage
> offset when CPU pages are larger than the 4 KiB GPU page size.

Good catch. All the addresses in page fault handlers and the filter 
should all be be 4KB page aligned (GPU page size). But 
svm_range_restore_pages works with page numbers based on the CPU page 
size, which may be larger on some architectures. So passing in the byte 
address is the most sane way to keep it straight. The patch is

Reviewed-by: Felix Kuehling <felix.kuehling@xxxxxxx>

I'm applying it to amd-staging-drm-next.


> Fixes: b3dc91f97317 ("drm/amdkfd: enable subsequent retry fault")
> Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> Closes: https://lore.kernel.org/r/sashiko-outbox-203586@xxxxxxxxxx
> Signed-off-by: Aveline Noir <jm5905938@xxxxxxxxx>
> ---
> No testing on AMD hardware. Any test will be appreciated.
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c   | 7 ++++---
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.h   | 4 ++--
>   3 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index c41e9c0916b7..86ef849097fa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -3092,7 +3092,7 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
>   		drm_exec_fini(&exec);
>   
>   		if (!svm_range_restore_pages(adev, pasid, vmid,
> -					     node_id, addr >> PAGE_SHIFT, ts, write_fault))
> +					     node_id, addr, ts, write_fault))
>   			return true;
>   
>   		/* Re-acquire the VM lock, could be that the VM was freed in between. */
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 79455e23a030..c85d6bb73629 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -3130,8 +3130,9 @@ svm_fault_allowed(struct vm_area_struct *vma, bool write_fault)
>   int
>   svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
>   			uint32_t vmid, uint32_t node_id,
> -			uint64_t addr, uint64_t ts, bool write_fault)
> +			uint64_t fault_addr, uint64_t ts, bool write_fault)
>   {
> +	uint64_t addr = fault_addr >> PAGE_SHIFT;
>   	unsigned long start, last, size;
>   	struct mm_struct *mm = NULL;
>   	struct svm_range_list *svms;
> @@ -3249,7 +3250,7 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
>   	mutex_lock(&prange->migrate_mutex);
>   
>   	if (svm_range_skip_recover(prange)) {
> -		amdgpu_gmc_filter_faults_remove(node->adev, addr, pasid);
> +		amdgpu_gmc_filter_faults_remove(node->adev, fault_addr, pasid);
>   		r = 0;
>   		goto out_unlock_range;
>   	}
> @@ -3353,7 +3354,7 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
>   
>   	if (r == -EAGAIN) {
>   		pr_debug("recover vm fault later\n");
> -		amdgpu_gmc_filter_faults_remove(node->adev, addr, pasid);
> +		amdgpu_gmc_filter_faults_remove(node->adev, fault_addr, pasid);
>   		r = 0;
>   	}
>   	return r;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> index c7d7adae4476..38744e716f74 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
> @@ -178,7 +178,7 @@ int svm_range_vram_node_new(struct kfd_node *node, struct svm_range *prange,
>   			    bool clear);
>   void svm_range_vram_node_free(struct svm_range *prange);
>   int svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
> -			    uint32_t vmid, uint32_t node_id, uint64_t addr, uint64_t ts,
> +			    uint32_t vmid, uint32_t node_id, uint64_t fault_addr, uint64_t ts,
>   			    bool write_fault);
>   int svm_range_evict_svm_bo(struct amdgpu_bo *bo);
>   
> @@ -230,7 +230,7 @@ static inline void svm_range_list_fini(struct kfd_process *p)
>   static inline int svm_range_restore_pages(struct amdgpu_device *adev,
>   					  unsigned int pasid,
>   					  uint32_t client_id, uint32_t node_id,
> -					  uint64_t addr, uint64_t ts, bool write_fault)
> +					  uint64_t fault_addr, uint64_t ts, bool write_fault)
>   {
>   	return -EFAULT;
>   }
>
> ---
> base-commit: 403b3d68346d7ba25f474f0686ad1d05feb65b7c
> change-id: 20261108-kfd-fault-filter-address-20261108-4f096f2b2a1a
>
> Best regards,
> --
> Aveline Noir <jm5905938@xxxxxxxxx>