[PATCH 14/26] drm/xe/eudebug: implement userptr_vma access

[Joonas Lahtinen]

Quoting Christian König (2024-12-10 12:00:48)
> Am 10.12.24 um 10:33 schrieb Joonas Lahtinen:
> 
>     Quoting Christian König (2024-12-09 17:42:32)
> 
>         Am 09.12.24 um 16:31 schrieb Simona Vetter:
> 
>             On Mon, Dec 09, 2024 at 03:03:04PM +0100, Christian König wrote:
> 
>                 Am 09.12.24 um 14:33 schrieb Mika Kuoppala:
> 
>                     From: Andrzej Hajda <andrzej.hajda at intel.com>
> 
>                     Debugger needs to read/write program's vmas including userptr_vma.
>                     Since hmm_range_fault is used to pin userptr vmas, it is possible
>                     to map those vmas from debugger context.
> 
>                 Oh, this implementation is extremely questionable as well. Adding the LKML
>                 and the MM list as well.
> 
>                 First of all hmm_range_fault() does *not* pin anything!
> 
>                 In other words you don't have a page reference when the function returns,
>                 but rather just a sequence number you can check for modifications.
> 
>             I think it's all there, holds the invalidation lock during the critical
>             access/section, drops it when reacquiring pages, retries until it works.
> 
>             I think the issue is more that everyone hand-rolls userptr.
> 
>         Well that is part of the issue.
> 
>         The general problem here is that the eudebug interface tries to simulate
>         the memory accesses as they would have happened by the hardware.
> 
>     Could you elaborate, what is that a problem in that, exactly?
> 
>     It's pretty much the equivalent of ptrace() poke/peek but for GPU memory.
> 
> 
> Exactly that here. You try to debug the GPU without taking control of the CPU
> process.

You seem to have a built-in expectation that the CPU threads and memory space
must be interfered with in order to debug a completely different set of threads
and memory space elsewhere that executes independently. I don't quite see why?

In debugging massively parallel workloads, it's a huge drawback to be limited to
stop all mode in GDB. If ROCm folks are fine with such limitation, I have nothing
against them keeping that limitation. Just it was a starting design principle for
this design to avoid such a limitation.

> This means that you have to re-implement all debug functionalities which where
> previously invested for the CPU process for the GPU once more.

Seems like a strawman argument. Can you list the "all interfaces" being added
that would be possible via indirection via ptrace() beyond peek/poke?

> And that in turn creates a massive attack surface for security related
> problems, especially when you start messing with things like userptrs which
> have a very low level interaction with core memory management.

Again, just seems like a strawman argument. You seem to generalize to some massive
attack surface of hypothetical interfaces which you don't list. We're talking
about memory peek/poke here.

Can you explain the high-level difference from security perspective for
temporarily pinning userptr pages to write them to page tables for GPU to
execute a dma-fence workload with and temporarily pinning pages for
peek/poke?

>     And it is exactly the kind of interface that makes sense for debugger as
>     GPU memory != CPU memory, and they don't need to align at all.
> 
> 
> And that is what I strongly disagree on. When you debug the GPU it is mandatory
> to gain control of the CPU process as well.

You are free to disagree on that. I simply don't agree and have in this
and previous email presented multiple reasons as to why not. We can
agree to disagree on the topic.

> The CPU process is basically the overseer of the GPU activity, so it should
> know everything about the GPU operation, for example what a mapping actually
> means.

How does that relate to what is being discussed here? You just seem to
explain how you think userspace driver should work: Maintain a shadow
tree of each ppGTT VM layout? I don't agree on that, but I think it is
slightly irrelevant here.

> The kernel driver and the hardware only have the information necessary to
> execute the work prepared by the CPU process. So the information available is
> limited to begin with.

And the point here is? Are you saying kernel does not know the actual mappings
maintained in the GPU page tables?

>         What the debugger should probably do is to cleanly attach to the
>         application, get the information which CPU address is mapped to which
>         GPU address and then use the standard ptrace interfaces.
> 
>     I don't quite agree here -- at all. "Which CPU address is mapped to
>     which GPU address" makes no sense when the GPU address space and CPU
>     address space is completely controlled by the userspace driver/application.
> 
> 
> Yeah, that's the reason why you should ask the userspace driver/application for
> the necessary information and not go over the kernel to debug things.

What hypothetical necessary information are you referring to exactly?

I already explained there are good reasons not to map all the GPU memory
into the CPU address space.

>     Please try to consider things outside of the ROCm architecture.
> 
> 
> Well I consider a good part of the ROCm architecture rather broken exactly
> because we haven't pushed back hard enough on bad ideas.
> 
> 
>     Something like a register scratch region or EU instructions should not
>     even be mapped to CPU address space as CPU has no business accessing it
>     during normal operation. And backing of such region will vary per
>     context/LRC on the same virtual address per EU thread.
> 
>     You seem to be suggesting to rewrite even our userspace driver to behave
>     the same way as ROCm driver does just so that we could implement debug memory
>     accesses via ptrace() to the CPU address space.
> 
> 
> Oh, well certainly not. That ROCm has an 1 to 1 mapping between CPU and GPU is
> one thing I've pushed back massively on and has now proven to be problematic.

Right, so is your claim then that instead of being 1:1 the CPU address space
should be a superset of all GPU address spaces instead to make sure
ptrace() can modify all memory?

Cause I'm slightly lost here as you don't give much reasoning, just
claim things to be certain way.

>     That seems bit of a radical suggestion, especially given the drawbacks
>     pointed out in your suggested design.
> 
> 
>         The whole interface re-invents a lot of functionality which is already
>         there
> 
>     I'm not really sure I would call adding a single interface for memory
>     reading and writing to be "re-inventing a lot of functionality".
> 
>     All the functionality behind this interface will be needed by GPU core
>     dumping, anyway. Just like for the other patch series.
> 
> 
> As far as I can see exactly that's an absolutely no-go. Device core dumping
> should *never ever* touch memory imported by userptrs.

Could you again elaborate on what the great difference is to short term
pinning to use in dma-fence workloads? Just the kmap?

> That's what process core dumping is good for.

Not really sure I agree. If you do not dump the memory as seen by the
GPU, then you need to go parsing the CPU address space in order to make
sense which buffers were mapped where and that CPU memory contents containing
metadata could be corrupt as we're dealing with a crashing app to begin with.

Big point of relying to the information from GPU VM for the GPU memory layout
is that it won't be corrupted by rogue memory accesses in CPU process.

>         just because you don't like the idea to attach to the debugged
>         application in userspace.
> 
>     A few points that have been brought up as drawback to the
>     GPU debug through ptrace(), but to recap a few relevant ones for this
>     discussion:
> 
>     - You can only really support GDB stop-all mode or at least have to
>       stop all the CPU threads while you control the GPU threads to
>       avoid interference. Elaborated on this on the other threads more.
>     - Controlling the GPU threads will always interfere with CPU threads.
>       Doesn't seem feasible to single-step an EU thread while CPU threads
>       continue to run freely?
> 
> 
> I would say no.

Should this be understood that you agree these are limitations of the ROCm
debug architecture?

>     - You are very much restricted by the CPU VA ~ GPU VA alignment
>       requirement, which is not true for OpenGL or Vulkan etc. Seems
>       like one of the reasons why ROCm debugging is not easily extendable
>       outside compute?
> 
> 
> Well as long as you can't take debugged threads from the hardware you can
> pretty much forget any OpenGL or Vulkan debugging with this interface since it
> violates the dma_fence restrictions in the kernel.

Agreed. However doesn't mean because you can't do it right now, you you should
design an architecture that actively prevents you from doing that in the future.

>     - You have to expose extra memory to CPU process just for GPU
>       debugger access and keep track of GPU VA for each. Makes the GPU more
>       prone to OOB writes from CPU. Exactly what not mapping the memory
>       to CPU tried to protect the GPU from to begin with.
> 
> 
>         As far as I can see this whole idea is extremely questionable. This
>         looks like re-inventing the wheel in a different color.
> 
>     I see it like reinventing a round wheel compared to octagonal wheel.
> 
>     Could you elaborate with facts much more on your position why the ROCm
>     debugger design is an absolute must for others to adopt?
> 
> 
> Well I'm trying to prevent some of the mistakes we did with the ROCm design.

Well, I would say that the above limitations are direct results of the ROCm
debugging design. So while we're eager to learn about how you perceive
GPU debugging should work, would you mind addressing the above
shortcomings?

> And trying to re-invent well proven kernel interfaces is one of the big
> mistakes made in the ROCm design.

Appreciate the feedback. Please work on the representation a bit as it currently
doesn't seem very helpful but appears just as an attempt to try to throw a spanner
in the works.

> If you really want to expose an interface to userspace

To a debugger process, enabled only behind a flag.

> which walks the process
> page table, installs an MMU notifier

This part is already done to put an userptr to the GPU page tables to
begin with. So hopefully not too controversial.

> kmaps the resulting page

In addition to having it in the page tables where GPU can access it.

> and then memcpy
> to/from it then you absolutely *must* run that by guys like Christoph Hellwig,
> Andrew and even Linus.

Surely, that is why we're seeking out for review.

We could also in theory use an in-kernel GPU context on the GPU hardware for
doing the peek/poke operations on userptr.

But that seems like a high-overhead thing to do due to the overhead of
setting up a transfer per data word and going over the PCI bus twice
compared to accessing the memory directly by CPU when it trivially can.

So this is the current proposal.

Regards, Joonas

> 
> I'm pretty sure that those guys will note that a device driver should
> absolutely not mess with such stuff.
> 
> Regards,
> Christian.
> 
> 
>     Otherwise it just looks like you are trying to prevent others from
>     implementing a more flexible debugging interface through vague comments about
>     "questionable design" without going into details. Not listing much concrete
>     benefits nor addressing the very concretely expressed drawbacks of your
>     suggested design, makes it seem like a very biased non-technical discussion.
> 
>     So while review interest and any comments are very much appreciated, please
>     also work on providing bit more reasoning and facts instead of just claiming
>     things. That'll help make the discussion much more fruitful.
> 
>     Regards, Joonas
> 
>