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

[Christian König]

Am 10.12.24 um 12:57 schrieb 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?

Because the GPU only gets the information it needs to execute the commands.

A simple example would be to single step through the high level shader 
code. That is usually not available to the GPU, but only to the 
application who has submitted the work.

The GPU only sees the result of the compiler from high level into low 
level assembler.

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

Well, that's the part I don't understand. Why is that a drawback?

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

That peek/poke interface is more than enough to cause problems.

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

If you want to access userptr imported pages from the GPU going through 
the hops of using hhm_range_fault()/get_user_pages() plus an MMU 
notifier is a must have.

For a CPU based debugging interface that isn't necessary, you can just 
look directly into the application address space with existing interfaces.

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

Yeah, that's ok. I also think we can agree on that this doesn't matter 
for the discussion.

The question is rather should the userptr functionality be used for 
debugging or not.

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

I'm trying to understand why you want to debug only the GPU without also 
attaching to the CPU process.

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

The kernel knows that, the question is why does userspace don't know that?

On the other hand I have to agree that this isn't much of a problem.

If userspace really doesn't know what is mapped where in the GPU's VM 
address space then an IOCTL to query that is probably not an issue.

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

What you said before: "the GPU address space and CPU address space is 
completely controlled by the userspace driver/application". When that's 
the case, then why as the kernel for help? The driver/application is in 
control.
> I already explained there are good reasons not to map all the GPU memory
> into the CPU address space.

Well I still don't fully agree to that argumentation, but compared to 
using userptr the peek/pook on a GEM handle is basically harmless.

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

Well why not? Mapping a BO and not accessing it has only minimal overhead.

We already considered to making that mandatory for TTM drivers for 
better OOM killer handling. That approach was discontinued, but 
certainly not for the overhead.

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

Ok, that's certainly not what I'm trying to express.

Things don't need to be in a certain way, especially not in the way ROCm 
does things.

But you should not try to re-create GPU accesses with the CPU, 
especially when that isn't memory you have control over in the sense 
that it was allocated through your driver stack.

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

The big difference is that the memory doesn't belong to the driver who 
is core dumping.

That is just something you have imported from the MM subsystem, e.g. 
anonymous memory and file backed mappings.

We also don't allow to mmap() dma-bufs on importing devices for similar 
reasons.

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

Well that you don't want to use potentially corrupted information is a 
good argument, but why just not dump an information like "range 
0xabcd-0xbcde came as userptr from process 1 VMA 0x1234-0x5678" ?

A process address space is not really something a device driver should 
be messing with.

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

ROCm has a bunch of design decisions I would say we should never ever 
repeat:

1. Forcing a 1 to 1 model between GPU address space and CPU address space.

2. Using a separate file descriptor additional to the DRM render node.

3. Attaching information and context to the CPU process instead of the 
DRM render node.
....

But stopping the world, e.g. both CPU and GPU threads if you want to 
debug something is not one of the problematic decisions.

That's why I'm really surprised that you insist so much on that.

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

Good point. That's what I can totally agree on as well.

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

Yeah, absolutely. That you don't have a 1 to 1 mapping on the GPU is a 
step in the right direction if you ask me.

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

Yeah, I mean that should clearly work out. We have something similar.

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

Understandable, but that will create another way of accessing process 
memory.

Regards,
Christian.

>
> 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
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-xe/attachments/20241210/3aa8e88e/attachment-0001.htm>