Quoting Christian König (2024-11-07 11:44:33)
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.
Could you elaborate on what this "existing functionality" exactly is?
I do not think this functionality exists at this time.
The EU debugging architecture for Xe specifically avoids the need for GDB
to attach with ptrace to the CPU process or interfere with the CPU process for
the debugging via parasitic threads or so.
Debugger connection is opened to the DRM driver for given PID (which uses the
ptrace may access check for now) after which the all DRM client of that
PID are exposed to the debugger process.
What we want to expose via that debugger connection is the ability for GDB to
read/write the different GPU VM address spaces (ppGTT for Intel GPUs) just like
the EU threads would see them. Note that the layout of the ppGTT is
completely up to the userspace driver to setup and is mostly only partially
equal to the CPU address space.
Specifically as part of reading/writing the ppGTT for debugging purposes,
there are deep flushes needed: for example flushing instruction cache
when adding/removing breakpoints.
Maybe that will explain the background. I elaborate on this at the end some more.
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.
Referring to the explanation at top: These IOCTL are not for the debugging target
process to issue. The peek/poke interface is specifically for GDB only
to facilitate the emulation of memory reads/writes on the GPU address
space as they were done by EUs themselves. And to recap: for modifying
instructions for example (add/remove breakpoint), extra level of cache flushing is
needed which is not available to regular userspace.
I specifically discussed with Sima on the difference before moving forward with this
design originally. If something has changed since then, I'm of course happy to rediscuss.
However, if this code can't be added, not sure how we would ever be able
to implement core dumps for GPU threads/memory?
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.
As also mentioned above, we don't work via ptrace at all when it comes
to debugging the EUs. The only thing used for now is the ptrace_may_access to
implement similar access restrictions as ptrace has. This can be changed
to something else if needed.
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.
Maybe this sentence is what caused the confusion.
Userspace is never exposed with peek/poke interface, only the debugger
connection which is its own FD.
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?
We're not working on the CPU address space or BOs. We're working
strictly on the GPU address space as would be seen by an EU thread if it
accessed address X.
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.
We're not operating on the CPU address space nor are we operating on BOs
(there is no concept of BO in the EU debug interface). Each VMA in the VM
could come from anywhere, only the start address and size matter. And
neither do we need to interfere with the command submission of the
process under debug.
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.
Recapping from above. It is to allow the debugging of EU threads per DRM
client, completely independent of the CPU process. If ptrace_may_acces
is the sore point, we could consider other permission checks, too. There
is no other connection to ptrace in this architecture as single
permission check to know if PID is fair game to access by debugger
process.
Why no parasitic thread or ptrace: Going forward, binding the EU debugging to
the DRM client would also pave way for being able to extend core kernel generated
core dump with each DRM client's EU thread/memory dump. We have similar
feature called "Offline core dump" enabled in the downstream public
trees for i915, where we currently attach the EU thread dump to i915 error state
and then later combine i915 error state with CPU core dump file with a
tool.
This is relatively little amount of extra code, as this baseline series
already introduces GDB the ability to perform the necessary actions.
It's just the matter of kernel driver calling: "stop all threads", then
copying the memory map and memory contents for GPU threads, just like is
done for CPU threads.
With parasitic thread injection, not sure if there is such way forward,
as it would seem to require to inject quite abit more logic to core kernel?
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.
Recapping, this series is not adding it back. The debugger connection
is a separate FD from the DRM one, with separate IOCTL set. We don't allow
the DRM FD any new operations based on ptrace is attached or not. We
don't ever do that check even.
We only restrict the opening of the debugger connection to given PID with
ptrace_may_access check for now. That can be changed to something else,
if necessary.
