Re: Feature Request: Ability to decode bus/dma address back into physical address

[Jerome Glisse <j.glisse-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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 ?

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.

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

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.

> 
> > > Again, not looking to rehash this debate.  AMDGPU has had register level
> > > debugfs access for nearly two years now.  At the point you can write to
> > > hardware registers you have to be root anyways.  I could just as easily load
> > > a tainted AMDGPU driver if I wanted to at that point which then achieves the
> > > same debugfs-free badness for me.
> > 
> > You also have IOMMU that restrict what you can do. Even if you can write
> > register to program a DMA operation you are still likely behind an IOMMU
> > and thus you can't write anywhere (unless IOMMU is disabled or in pass
> > through ie 1 on 1 mapping).
> 
> Fair point.  All I was saying is at the point you have MMIO access you can
> typically do bad things if you wanted to (like crash the system).
> 
> > I am saying you can have access to this memory with a sane API and not with
> > something insane. What is wrong with what i outlined above ?
> 
> 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.

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.

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.


> 
> > > At the point I'm root I can attach to any process, change memory contents,
> > > do whatever I want.  Unless the kernel has such broken vfs security that it
> > > allows non-root users to read/write debugfs/etc it shouldn't be a problem.
> > 
> > With sane API you would not need to be root to debug/trace GPU activity of
> > your own process like gdb.
> 
> You need to be root to read GPU registers though (simply reading some
> registers can cause a GPU hang).  So if you want to see which waves are
> active, or the contents of a ring (or it's IBs) you can't do that as a
> normal user.  There's a very tiny subset of registers you can access through
> libdrm and often we need more than that (especially for npi work).
> 
> Indeed we recommend that on devel machines people +s umr so they can invoke
> it from normal user processes.  You wouldn't normally install umr unless you
> were a developer anyways so for customers/users it's not an issue.
> 
> It's not exactly as confined as all-software which is where some compromises
> are made.

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.

> 
> > > > > The existing tracer patch "works" but it's not ideal and the maintainers of
> > > > > the AMDGPU driver are legitimately desiring a better solution.
> > > > 
> > > > Again re-design what you are trying to do. There is no legitimate need to
> > > > track individual page mapping. All you need is access to all buffer object
> > > > a process have created and to be inform on anything a process do through
> > > > the amdgpu device file.
> > > 
> > > Again the problem is the hardware is programmed with the dma address. So I
> > > want to correlate what the hardware is doing with what the process is doing
> > > I need the mappings.
> > 
> > You only need the GPU virtual address and you can get it with the API i outline.
> 
> Which is effectively what my tracer is.  But it (and your design) can't
> translate all mappings which is why we even engaged with iommu list in the
> first place at all.

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.

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

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

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.

Jérôme