Was trying to walk away from this ... :-) (all in good fun).
On 01/08/17 03:55 PM, Jerome Glisse wrote:
On Tue, Aug 01, 2017 at 03:28:26PM -0400, Tom St Denis wrote:
Adding the AMDGPU maintainers to get their opinions.
Context:
https://lists.linuxfoundation.org/pipermail/iommu/2017-August/023489.html
(and others in case you missed it on the list).
On 01/08/17 03:03 PM, Jerome Glisse wrote:
You would need to leverage thing like uevent to get event when something
happen like a bo being destroy or command submission ...
The problem with this approach is when I'm reading an IB I'm not given user
space addresses but bus addresses. So I can't correlate anything I'm seeing
in the hardware with the user task if I wanted to.
In fact, to augment [say] OpenGL debugging I would have to correlate a
buffer handle/pointer's page backing with the bus address in the IB so I
could correlate the two (e.g. dump an IB and print out user process variable
names that correspond to the IB contents...).
When you read IB you are provided with GPU virtual address, you can get the
GPU virtual address from the same snoop ioctl just add a field in bo_info
above. So i don't see any issue here.
This is effectively what I'm doing with the patch except via a trace not a
bolted on snooping ioctl.
Tracers are a bit better since there's not really any overhead in the normal
case which is desirable (and the code is relatively very simple).
In an ideal case we could simply search the ttm pages for a given device to
look for a match for a given dma address. But that also has the problem
that ...
This will fail for all the other allocations e.g. for our GART, ring
buffers, etc.
So a "complete" solution would be nice where any bus address mapping that is
still valid for a device could be resolved to the physical page that is
backing it.
So you agree that you can get what you want with ioctl ? Now you object
that the ioctl overhead is too important ?
No, I object that the overhead of keeping track of it in some sort of
snoop structure is too much (and adds too much new code to the driver
for the sole purpose of this one feature).
Not to mention we can't really invent ioctls without co-ordinating with
with the other drm users (re: not just AMD). umr is a lot less coupled
since it's not really versioned just yet (still bringing up a lot of
features so users use git not packages).
I do not believe ioctl overhead to be an issue. Like i say in the model
i adviced you mix ioctl and thing like uevent or trace so you get "real
time" notification of what is going on.
Unless you can nop that in a config invariant fashion (like you can for
tracers) that's a NAK from the get go. And we'd need to buffer them to
be practical since you might run the debugger out of sync with the
application (e.g. app hangs then you fire up umr to see what's going on).
Why do i say that your approach of tracking individual page mapping is
insane. There is several obstacle:
- to get from GPU virtual address to physical memory address you need
to first walk down the GPU page table to get the GPU pte entry. From
that you either get an address inside the GPU memory (VRAM of PCIE
device) or a DMA bus address. If it is the latter (DMA bus address)
you need to walk down the IOMMU page table to find the physical address
of the memory. So this is 2 page table walk down
We already do this in umr. give a GPUVM address we can decode it down
to a PTE on both AI and VI platforms. It's decoding the PTE to a
physical page (outside of vram) that is the issue.
In fact being able to VM decode is important to the kernel team who have
to debug VM issues from time to time.
- none of the above walk down can happen while holding enough lock so
that the GPU page table do not change behind you back (especialy the
second walk down) so you are open to race
Which is fine because you'd only really do this on a stalled/halted GPU
anyways. Just like you don't inspect variables of a program that is
currently running (and not blocked/asleep/etc). We have the ability to
halt currently active waves/etc.
- GPU device memory is not necessary accessible (due to PCIE bar size
limit i know that there is patches to allow to remap all GPU memory).
We have ways of accessing all of VRAM from basic MMIO accesses :)
- you need to track a lot of informations in userspace ie what memory
is behind each 4k of GPU virtual address space you are interested in
so you are basicaly duplicating informations that already exist in
kernel space into userland
Agreed, if we had a TTM query API that'd be better but we don't. And we
work out what the memory is by inspecting the ring. The ring has
pointers to IB buffers which then have their own contents (e.g. CE/DE
content).
With what i outline you rely on the kernel informations to get to the
thing you want. You can even add something like:
struct amdgpu_snoop_snapshot_ioctl {
uint64_t gpu_virtual_address;
uint64_t size;
uint64_t uptr;
};
To snapshot a range of the GPU virtual address space so you not even
have to worry about GPU virtual address -> bo handle -> offset into
bo. You can even go further and add a /dev/dri/card0-snoop device file
that you can mmap to access GPU virtual address space. Process open
the card0 file register as snoop against another process from that point
on a GPU virtual address in the snooped process becomes an offset into
the card0-snoop file. So now you can read/write on that device file
to access the GPU memory of a process without doing ioctl. The changes
to amdgpu are not that big.
Honestly I'm not qualified to agree/disagree with your last sentence. I
suspect it's a lot more complicated than a trace though and as I said a
few times already this won't cover allocations done privately in the kernel.
Well aside from the fact it doesn't cover all mappings, I can't see how it's
materially different than what I was already doing. For this to work I'd
have to be able to mmap objects from another process into the debugger. I
don't know if drm is really designed for that sort of flow and I suspect it
would take a lot of work to add that.
The amount of work does not matter. We are aiming for sane solution not
insane thing. We are aiming for thing that are safe from security point
of view. We are aiming for design that lower barrier (ie no need to be
root to debug/trace GPU). This is what matters.
Except that you're approaching this from the angle that the KMD is
perfect and we're debugging the UMD. umr is meant to debug both. So
things like raw access to VRAM/GART is necessary. For instance, to
debug VM mappings, to read IBs, to read frame buffers, etc...
Quite frankly what i outline is not a lot of work. The snoop device file
scheme for instance is pretty easy to achieve. Everytime there is a change
to ttm object or GPU virtual address space you would need to update
accordingly any mmap of that file. Overall i would say this is in the
500/1000 lines of code range.
Again I'm not familiar enough with the DRM/TTM code design to comment here.
GPU device memory is slightly harder, you probably need to do it as a
snapshot ie you copy device memory to system memory and map the system
memory.
Which is unnecessary since we can directly read VRAM from the debugger :-)
We want thing like debugger to work out of the box on any distribution so
design must be sane and you should not need either to ask special modules
or special kernel config option or to be root. There is way to do what you
want without any of this requirement and thus that should be what you aim
for.
Except again you're looking at this from the lens that the KMD is
working perfectly. We use umr (our debugger) to debug the kernel driver
itself. That means you need to be able to read/write MMIO registers,
peek at VRAM, decode VM addresses, read rings, etc...
Since we already have an entire list of "security breaches" we might as
well make use of them for UMD debugging too. Again, you're not going to
give random logins access to umr on your machine. I mean in reality you
wouldn't do that anyways for UMD tasks (you can easily lock up a GPU
from a user task with no privileges...).
My design do not need to get the reverse mapping at all that is my point.
No need to engage the iommu folks no need to ask them for something as
you have all the information from inside the amdgpu driver.
Which again works iff you're debugging UMD only.
Adding drm support to allow another process to GEM_MAP BOs (and a search
ioctl) might work but seems like a lot more work (and will likely reduce
performance) than the tracer and has the same limitation. And ultimately we
find direct memory access useful in NPI work so that's unlikely to be
removed.
You have not proof that it would be slower and i outline the special file
idea that avoid any ioctl.
By "slower" I don't mean the syscall for ioctl. I mean the housekeeping
we'd have to do in the driver on top of normal TTM housekeeping. Unless
you can nop it all out by default (which I suspect you can) it will get
NAK'ed immediately by the maintainers.
Setting aside the fact of adding 500+ lines of code so you could attach
to other libdrm processes and rifle through their BOs is code they have
to maintain.
Thanks for your time Jerome. I do understand where you're coming from but
I'm not sure a drm update is both politically or technically feasible and we
need this functionality (limited though it is) sooner than later.
You have to understand that kernel is not about compromising design so that
people can reach their schedule. Sane design do matter and if sane design
require lot of work than you should size your team accordingly to face the
challenges.
Except the real world doesn't work that way. And we have legitimate
reasons for being able to peek/poke at the hardware. Philosophies likes
yours lead companies to write private debug tools and not share them
with the public. Whereas we're trying to come up with a debug tool (and
kernel module) that is practically safe and effective at its job without
overly mucking about in the kernel.
You can't legitimately debug hardware from a userspace tool without
having some knobs/etc that would otherwise be ill advised to give to
everyone. In a normal distribution your users run unprivileged and
wouldn't have umr installed. So they have zero access to any of our
debugfs facilities and can only talk to the device via libdrm (via
GL/VK/etc).
I still haven't seen an explanation for why having root-only access to
the device is bad other than "it's insane!" At the point you can run
privileged processes you can simply compile and install a tainted kernel
module to do the same thing.
Ideally, though if we went this route it'd be a drm/ttm change not an
amdgpu change since I can't believe the other vendors wouldn't be able
to make use of this functionality either.
What you want to do can be done with very little code and no change outside
amdgpu kernel driver so i do not see any reason to not do so.
Maybe Alex or Christian can weigh in at this point?
I'm not saying your proposal isn't possible I just don't have enough
clarity on how I'd implement that nor enough confidence that it would be
materially any better (keep in mind we are NOT removing our debugfs
facilities).
Tom
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
