linux-kernel - Re: [PATCH v2 1/1] drm/amdkfd: Fix unaligned doorbell absolute offset for gfx8

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <e33eb526-c0b7-c404-72d2-390ce7fb0108@amd.com>
Date:   Fri, 29 Sep 2023 10:39:28 +0200
From:   Shashank Sharma <shashank.sharma@....com>
To:     Felix Kuehling <felix.kuehling@....com>,
        "Joshi, Mukul" <Mukul.Joshi@....com>,
        "Yadav, Arvind" <Arvind.Yadav@....com>,
        "Koenig, Christian" <Christian.Koenig@....com>,
        "Deucher, Alexander" <Alexander.Deucher@....com>,
        "Pan, Xinhui" <Xinhui.Pan@....com>,
        "airlied@...il.com" <airlied@...il.com>,
        "daniel@...ll.ch" <daniel@...ll.ch>
Cc:     "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "amd-gfx@...ts.freedesktop.org" <amd-gfx@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/1] drm/amdkfd: Fix unaligned doorbell absolute offset
 for gfx8


On 28/09/2023 20:53, Felix Kuehling wrote:
> On 2023-09-28 11:38, Shashank Sharma wrote:
>> Hello Felix, Mukul,
>>
>> On 28/09/2023 17:30, Felix Kuehling wrote:
>>> On 2023-09-28 10:30, Joshi, Mukul wrote:
>>>> [AMD Official Use Only - General]
>>>>
>>>>> -----Original Message-----
>>>>> From: Yadav, Arvind <Arvind.Yadav@....com>
>>>>> Sent: Thursday, September 28, 2023 5:54 AM
>>>>> To: Koenig, Christian <Christian.Koenig@....com>; Deucher, Alexander
>>>>> <Alexander.Deucher@....com>; Sharma, Shashank
>>>>> <Shashank.Sharma@....com>; Kuehling, Felix <Felix.Kuehling@....com>;
>>>>> Joshi, Mukul <Mukul.Joshi@....com>; Pan, Xinhui <Xinhui.Pan@....com>;
>>>>> airlied@...il.com; daniel@...ll.ch
>>>>> Cc: amd-gfx@...ts.freedesktop.org; 
>>>>> dri-devel@...ts.freedesktop.org; linux-
>>>>> kernel@...r.kernel.org; Yadav, Arvind <Arvind.Yadav@....com>; Koenig,
>>>>> Christian <Christian.Koenig@....com>
>>>>> Subject: [PATCH v2 1/1] drm/amdkfd: Fix unaligned doorbell 
>>>>> absolute offset
>>>>> for gfx8
>>>>>
>>>>> This patch is to adjust the absolute doorbell offset against the 
>>>>> doorbell id
>>>>> considering the doorbell size of 32/64 bit.
>>>>>
>>>>> v2:
>>>>> - Addressed the review comment from Felix.
>>>>>
>>>>> Cc: Christian Koenig <christian.koenig@....com>
>>>>> Cc: Alex Deucher <alexander.deucher@....com>
>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@....com>
>>>>> Signed-off-by: Arvind Yadav <Arvind.Yadav@....com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 9 ++++++++-
>>>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>>>> index 0d3d538b64eb..c54c4392d26e 100644
>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>>>> @@ -407,7 +407,14 @@ static int allocate_doorbell(struct
>>>>> qcm_process_device *qpd,
>>>>>
>>>>>        q->properties.doorbell_off = amdgpu_doorbell_index_on_bar(dev-
>>>>>> adev,
>>>>>                                                                  qpd-
>>>>>> proc_doorbells,
>>>>> -                                                               q-
>>>>>> doorbell_id);
>>>>> +                                                               0);
>>>>> +
>>>> It looks like amdgpu_doorbell_index_on_bar() works only for 64-bit 
>>>> doorbells.
>>>> Shouldn't it work for both 32-bit and 64-bit doorbells considering 
>>>> this is common
>>>> doorbell manager code?
>>
>>
>> Yes, You are right that the calculations to find a particular 
>> doorbell in the doorbell page considers a doorbell width of 64-bit.
>>
>>>
>>> I could see this argument going either way. KFD is the only one that 
>>> cares about managing doorbells for user mode queues on GFXv8 GPUs. 
>>> This is not a use case that amdgpu cares about. So I'm OK with KFD 
>>> doing its own address calculations to make sure doorbells continue 
>>> to work on GFXv8.
>>>
>>> It may not be worth adding complexity to the common doorbell manager 
>>> code to support legacy GPUs with 32-bit doorbells.
>>
>>
>> I was thinking about adding an additional input parameter which will 
>> indicate if the doorbell width is 32-bit vs 64-bit (like 
>> is_doorbell_64_bit), and doorbell manager can alter the multiplier 
>> while calculating the final offset. Please let me know if that will 
>> work for both the cases.
>
> Yes, that would work for KFD because we already have the doorbell size 
> in our device-info structure. Instead of making it a boolean flag, you 
> could make it a doorbell_size parameter, in byte or dword units to 
> simplify the pointer math.


Sounds good, will do that and send an update.

- Shashank

>
> Regards,
>   Felix
>
>
>>
>> - Shashank
>>
>>>
>>>
>>> Regards,
>>>   Felix
>>>
>>>
>>>>
>>>> Thanks,
>>>> Mukul
>>>>
>>>>> +     /* Adjust the absolute doorbell offset against the doorbell id
>>>>> considering
>>>>> +      * the doorbell size of 32/64 bit.
>>>>> +      */
>>>>> +     q->properties.doorbell_off += q->doorbell_id *
>>>>> + dev->kfd->device_info.doorbell_size / 4;
>>>>> +
>>>>>        return 0;
>>>>>   }
>>>>>
>>>>> -- 
>>>>> 2.34.1