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

[Matthew Auld <matthew.auld@intel.com>]

On 20/03/2025 20:40, Matthew Brost wrote:
> On Thu, Mar 20, 2025 at 05:29:58PM +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.
>> + */
> 
> I'd make this proper kernel doc given all of drm_gpusvm.c has proper kernel doc.

Ok, typically we don't add kernel-doc for static declarations so didn't 
bother. Will make this consistent. Thanks.

> 
> Otherwise LGTM.
> 
> Matt
> 
>> +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;
>> +}
>> +
>>   /**
>>    * 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) {
>> -- 
>> 2.48.1
>>