Re: [PATCH v6 2/8] drm/ttm: Add ttm_bo_access

["Christian König" <christian.koenig@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 3748 bytes --]

Am 06.11.24 um 18:00 schrieb Matthew Brost:
> [SNIP]
> This is not a generic interface that anyone can freely access. The same
> permissions used by ptrace are checked when opening such an interface.
> See [1] [2].
>
> [1]https://patchwork.freedesktop.org/patch/617470/?series=136572&rev=2
> [2]https://patchwork.freedesktop.org/patch/617471/?series=136572&rev=2

Thanks a lot for those pointers, that is exactly what I was looking for.

And yeah, it is what I feared. You are re-implementing existing 
functionality, but see below.

>>> kmap/vmap are used everywhere in the DRM subsystem to access BOs, so I’m
>>> failing to see the problem with adding a simple helper based on existing
>>> code.
>> What#s possible and often done is to do kmap/vmap if you need to implement a
>> CPU copy for scanout for example or for copying/validating command buffers.
>> But that usually requires accessing the whole BO and has separate security
>> checks.
>>
>> When you want to access only a few bytes of a BO that sounds massively like
>> a peek/poke like interface and we have already rejected that more than once.
>> There even used to be standardized GEM IOCTLs for that which have been
>> removed by now.
>>
>> If you need to access BOs which are placed in not CPU accessible memory then
>> implement the access callback for ptrace, see amdgpu_ttm_access_memory for
>> an example how to do this.
>>
> Ptrace access via vm_operations_struct.access → ttm_bo_vm_access.
>
> This series renames ttm_bo_vm_access to ttm_bo_access, with no code changes.
>
> The above function accesses a BO via kmap if it is in SYSTEM / TT,
> which is existing code.
>
> This function is only exposed to user space via ptrace permissions.
> In this series, we implement a function [3] similar to
> amdgpu_ttm_access_memory for the TTM vfunc access_memory. What is
> missing is non-visible CPU memory access, similar to
> amdgpu_ttm_access_memory_sdma. This will be addressed in a follow-up and
> was omitted in this series given its complexity.
>
> So, this looks more or less identical to AMD's ptrace implementation,
> but in GPU address space. Again, I fail to see what the problem is here.
> What am I missing?

The main question is why can't you use the existing interfaces directly?

Additional to the peek/poke interface of ptrace Linux has the 
pidfd_getfd system call, see here 
https://man7.org/linux/man-pages/man2/pidfd_getfd.2.html.

The pidfd_getfd() allows to dup() the render node file descriptor into 
your gdb process. That in turn gives you all the access you need from 
gdb, including mapping BOs and command submission on behalf of the 
application.

As far as I can see that allows for the same functionality as the 
eudebug interface, just without any driver specific code messing with 
ptrace permissions and peek/poke interfaces.

So the question is still why do you need the whole eudebug interface in 
the first place? I might be missing something, but that seems to be 
superfluous from a high level view.

It's true that the AMD KFD part has still similar functionality, but 
that is because of the broken KFD design of tying driver state to the 
CPU process (which makes it inaccessible for gdb even with imported 
render node fd).

Both Sima and I (and partially Dave as well) have pushed back on the KFD 
approach. And the long term plan is to get rid of such device driver 
specific interface which re-implement existing functionality just 
differently.

So you need to have a really really good explanation why the eudebug 
interface is actually necessary.

Regards,
Christian.

>
> Matt
>
> [3]https://patchwork.freedesktop.org/patch/622520/?series=140200&rev=6
>
>> Regards,
>> Christian.
>>
>>> Matt
>>>
>>>> Regards,
>>>> Christian.

[-- Attachment #2: Type: text/html, Size: 5707 bytes --]