Re: [PATCH 1/7] drm/gpusvm: fix hmm_pfn_to_map_order() usage

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Thu, 2025-03-20 at 17:29 +0000, Matthew Auld wrote:
> Handle the case where the hmm range partially covers a huge page
> (like
> 2M), otherwise we can potentially end up doing something nasty like
> mapping memory which potentially is outside the range, and maybe not
> even mapped by the mm. Fix is based on the xe userptr code, which in
> a
> future patch will directly use gpusvm, so needs alignment here.
> 
> Reported-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/drm_gpusvm.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gpusvm.c
> b/drivers/gpu/drm/drm_gpusvm.c
> index 2451c816edd5..48993cef4a74 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
> @@ -817,6 +817,27 @@ drm_gpusvm_range_alloc(struct drm_gpusvm
> *gpusvm,
>  	return range;
>  }
>  
> +/*
> + * To allow skipping PFNs with the same flags (like when they belong
> to
> + * the same huge PTE) when looping over the pfn array, take a given
> a hmm_pfn,
> + * and return the largest order that will fit inside the PTE, but
> also crucially
> + * accounting for the original hmm range boundaries.
> + */
> +static unsigned int drm_gpusvm_hmm_pfn_to_order(unsigned long
> hmm_pfn,
> +						unsigned long
> hmm_pfn_index,
> +						unsigned long
> npages)
> +{
> +	unsigned long size;
> +
> +	size = 1UL << hmm_pfn_to_map_order(hmm_pfn);
> +	size -= (hmm_pfn & ~HMM_PFN_FLAGS) & (size - 1);
> +	hmm_pfn_index += size;
> +	if (hmm_pfn_index > npages)
> +		size -= (hmm_pfn_index - npages);
> +
> +	return fls(size) - 1;

ilog2() for readability?

> +}
> +
>  /**
>   * drm_gpusvm_check_pages() - Check pages
>   * @gpusvm: Pointer to the GPU SVM structure
> @@ -875,7 +896,7 @@ static bool drm_gpusvm_check_pages(struct
> drm_gpusvm *gpusvm,
>  			err = -EFAULT;
>  			goto err_free;
>  		}
> -		i += 0x1 << hmm_pfn_to_map_order(pfns[i]);
> +		i += 0x1 << drm_gpusvm_hmm_pfn_to_order(pfns[i], i,
> npages);
>  	}
>  
>  err_free:
> @@ -1408,7 +1429,7 @@ int drm_gpusvm_range_get_pages(struct
> drm_gpusvm *gpusvm,
>  	for (i = 0, j = 0; i < npages; ++j) {
>  		struct page *page = hmm_pfn_to_page(pfns[i]);
>  
> -		order = hmm_pfn_to_map_order(pfns[i]);
> +		order = drm_gpusvm_hmm_pfn_to_order(pfns[i], i,
> npages);
>  		if (is_device_private_page(page) ||
>  		    is_device_coherent_page(page)) {
>  			if (zdd != page->zone_device_data && i > 0)
> {
Either way
Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>