[PATCH] drm/amdgpu: Fix multi-level page table bugs for large BOs v2

[Felix Kuehling]

On 17-03-29 12:34 PM, Christian König wrote:
> Am 29.03.2017 um 17:48 schrieb Felix Kuehling:
>> Fix the start/end address calculation for address ranges that span
>> multiple page directories in amdgpu_vm_alloc_levels.
>>
>> Add WARN_ONs if page tables aren't found. Otherwise the page table
>> update would just fail silently.
>>
>> v2:
>>   * Change WARN_ON to WARN_ON_ONCE
>>   * Move masking of high address bits to caller
>>   * Add range-check for "from" and "to"
>>
>> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 20 +++++++++++++++-----
>>   1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 818747f..033852b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -275,13 +275,18 @@ static int amdgpu_vm_alloc_levels(struct
>> amdgpu_device *adev,
>>           memset(parent->entries, 0 , sizeof(struct amdgpu_vm_pt));
>>       }
>>   -    from = (saddr >> shift) % amdgpu_vm_num_entries(adev, level);
>> -    to = (eaddr >> shift) % amdgpu_vm_num_entries(adev, level);
>> +    from = saddr >> shift;
>> +    to = eaddr >> shift;
>> +    if (from >= amdgpu_vm_num_entries(adev, level) ||
>> +        to >= amdgpu_vm_num_entries(adev, level))
>> +        return -EINVAL;
>>         if (to > parent->last_entry_used)
>>           parent->last_entry_used = to;
>>         ++level;
>> +    saddr = saddr & ((1 << shift) - 1);
>> +    eaddr = eaddr & ((1 << shift) - 1);
>
> Why do you need this part? The modulo above should have handled that.

This adjusts the saddr and eaddr for the next recursion level. It
removes the high bits that don't matter in the next level. That's what
we talked about in the code review: moving the masking of high-bits to
the caller.

>
>>         /* walk over the address space and allocate the page tables */
>>       for (pt_idx = from; pt_idx <= to; ++pt_idx) {
>> @@ -312,8 +317,11 @@ static int amdgpu_vm_alloc_levels(struct
>> amdgpu_device *adev,
>>           }
>>             if (level < adev->vm_manager.num_level) {
>> -            r = amdgpu_vm_alloc_levels(adev, vm, entry, saddr,
>> -                           eaddr, level);
>> +            uint64_t sub_saddr = (pt_idx == from) ? saddr : 0;
>> +            uint64_t sub_eaddr = (pt_idx == to) ? eaddr :
>> +                ((1 << shift) - 1);
>> +            r = amdgpu_vm_alloc_levels(adev, vm, entry, sub_saddr,
>> +                           sub_eaddr, level);
>>               if (r)
>>                   return r;
>>           }
>> @@ -957,9 +965,11 @@ static struct amdgpu_bo *amdgpu_vm_get_pt(struct
>> amdgpu_pte_update_params *p,
>>           entry = &entry->entries[idx];
>>       }
>>   -    if (level)
>> +    if (WARN_ON_ONCE(level))
>>           return NULL;
>>   +    WARN_ON_ONCE(!entry->bo);
>> +
>
> Please drop those warnings. It is perfectly valid not to have the PT
> allocated here.
>
> Rather add a warning to the caller if it needs the BO.

This function is only called from two places in amdgpu_vm_update_ptes.
In both cases the PT is needed and a failure means that we're silently
not updating a page table that we expected to be there. I can add an
error message to the caller instead. But then it'll be in two places
instead of one.

Regards,
  Felix

>
>>       return entry->bo;
>>   }
>>   
>
>