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

Mon Nov 11 11:27:07 UTC 2024

Hi Jonas,

Am 11.11.24 um 09:00 schrieb Joonas Lahtinen:
> Back from some time off and will try to answer below.

welcome back, good to have the designer of this at hand.

> Adding Dave and Sima as this topic has been previously discussed to some
> extent and will be good to reach common understanding about what the
> series is trying to do and what is the difference to the AMD debugging
> model.

Yeah, I was already wondering why that wasn't issued before.

> 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.

You can get the exact same functionality by requesting the render FD 
from the debugged process.

This also doesn't cause any security concerns since it uses the existing 
systemcall interfaces, especially see pidfd_getfd() and fdinfo for 
reference.

> 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.

I can understand why you don't want to use parsitic threads, but why 
don't you want to attach with GDB to the process?

I mean you somehow need to prevent that the debugging information you 
try to gather or modify change while you access them.

> 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.

That sounds extremely questionable and just re-implements existing 
functionality as far as I can see.

The fdinfo file under proc already provides the necessary information 
which file render nodes a pid uses and the pidfd_getfd() system call 
then gives you access to those render node file descriptors.

Why do you need that as separate and especially driver specific 
functionality?

> 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.

Is that not something you can do through the render node the PID uses as 
well?

If yes I think it would still be much more cleaner to expose that as 
IOCTL on the render node.

> 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.

Do you have pointers to this discussion?

> However, if this code can't be added, not sure how we would ever be able
> to implement core dumps for GPU threads/memory?

Exactly that's what I tried to point out before. Use cases like core 
dumps or even CPU copies are valid use cases.

We do that inside AMDGPU as well or at least have plans for it, but we 
already figured out that you can't use the TTM interfaces for that.

When you want to do a core dump the GPU is usually stuck executing and 
when you try to call kmap/vmap it is possible that those calls wait for 
the stuck GPU to finish whatever it is executing.
That's why drivers need to use hardware specific approaches to access 
data for crash dumps.
[SNIP]
>> 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.

I would rather say that you try to debug completely independent of the 
CPU process is a really bad idea.

> 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",

OH! Wait a second, you do WHAT? How do you guarantee dma_fence forward 
progress in that case?

See here: 
https://www.kernel.org/doc/html/latest/driver-api/dma-buf.html#indefinite-dma-fences

[SNIP]
>> So you need to have a really really good explanation why the eudebug interface
>> is actually necessary.
> TL;DR The main point is to decouple the debugging of the EU workloads from the
> debugging of the CPU process. This avoids the interference with the CPU process with
> parasitic thread injection. Further this also allows generating a core dump
> without any GDB connected. There are also many other smaller pros/cons
> which can be discussed but for the context of this patch, this is the
> main one.
>
> So unlike parasitic thread injection, we don't unlock any special IOCTL for
> the process under debug to be performed by the parasitic thread, but we
> allow the minimal set of operations to be performed by GDB as if those were
> done on the EUs themselves.
>
> One can think of it like the minimal subset of ptrace but for EU threads,
> not the CPU threads. And thus, building on this it's possible to extend
> the core kernel generated core dumps with DRM specific extension which
> would contain the EU thread/memory dump.

I can understand that you don't want to use complicated and hard to get 
right approaches like parasitic debugging threads, but this should also 
be absolutely not necessary.

The problem is that when you completely avoid ptrace and the existing 
interface you also have to implement a lot of stuff which is already 
more or less there. In other words debuggers like gdb already have the 
ability to interact with device drivers through their file descriptors. 
And that includes all IOCTLs, mmap() as well as things like command 
submission etc...

It could be that you need some addition IOCTL, e.g. like flushing caches 
etc..., but you certainly don't need a separate file descriptor gated by 
exporting ptrace access check functions. That's a really questionable 
design.

But taking a step back: When you stop GPU execution and insert break 
points you need to guarantee that this will never affect any dma_fence. 
Otherwise the core memory management can run into a deadlock.

Neither the preemption fence XE uses for it's threads nor the hardware 
fence used for end of submission indication can be delayed while things 
like a core dump is underway. That's why you also can't fully core dump 
in the case of a GPU hang.

What is possible is that you wait for the XE preemption fence to signal 
(which AFAIK is implemented XE internally as stopping all threads), but 
skimming over the code this absolutely doesn't seem what you do.

So at least of hand that looks like a classic indefinite DMA fence 
problem to me which will get you a whale big NAK from Sima and Dave.

While for the peek/poke interface is maybe a bit ugly, but more or less 
doable, stopping the GPU without signaling the dma_fences is really 
*not* something you can do.

Regards,
Christian.

>
> Regards, Joonas
>
>> Regards,
>> Christian.
>>
>>
>>
>>      Matt
>>
>>      [3]https://patchwork.freedesktop.org/patch/622520/?series=140200&rev=6
>>
>>
>>          Regards,
>>          Christian.
>>
>>
>>              Matt
>>
>>
>>                  Regards,
>>                  Christian.
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20241111/3a9a7e04/attachment-0001.htm>