Am 20.09.23 um 09:36 schrieb Thomas Hellström:
Hi, Zack,
On 9/20/23 05:43, Zack Rusin wrote:
On Tue, 2023-09-19 at 09:47 +0200, Christian König wrote:
!! External Email
Am 19.09.23 um 08:56 schrieb Thomas Hellström:
On 9/19/23 07:39, Christian König wrote:
Am 19.09.23 um 03:26 schrieb Zack Rusin:
On Mon, 2023-09-18 at 16:21 -0400, Alex Deucher wrote:
!! External Email
On Mon, Sep 18, 2023 at 3:06 PM Thomas Hellström
<thomas.hellstrom@xxxxxxxxxxxxxxx> wrote:
On 9/18/23 17:52, Zack Rusin wrote:
On Mon, 2023-09-18 at 17:13 +0200, Thomas Hellström wrote:
Hi,
On 9/18/23 16:56, Thomas Hellström wrote:
Hi Zack, Christian
On 9/18/23 13:36, Christian König wrote:
Hi Zack,
adding Thomas and Daniel.
I briefly remember that I talked with Thomas and some other
people
about that quite a while ago as well, but I don't fully
remember the
outcome.
Found one old thread, but didn't read it:
https://lists.freedesktop.org/archives/dri-devel/2019-September/234100.html
/Thomas
Ugh. Now starting to read that thread I have a vague
recollection it all
ended with not supporting mapping any device pages whatsoever
when SEV
was enabled, but rather resorting to llvmpipe and VM-local bos.
Hi, Thomas.
Thanks for finding this! I'd (of course) like to solve it
properly and get
vmwgfx
running with 3d support with SEV-ES active instead of essentially
disabling
the
driver when SEV-ES is active.
I think there are two separate discussions there, the
non-controversial one
and the
controversial one:
1) The non-controversial: is there a case where drivers would
want encrypted
memory
for TT pages but not for io mem mappings? Because if not then as
Christian
pointed
out we could just add pgprot_decrypted to ttm_io_prot and be
essentially done.
The
current method of decrypting io mem but leaving sys mem mappings
encrypted is
a bit
weird anyway.
If the answer to that question is "yes, some driver does want the
TT mappings
to be
encrypted" then your "[PATCH v2 3/4] drm/ttm, drm/vmwgfx:
Correctly support
support
AMD memory encryption" solves that. I think getting one of those
two in makes
sense
regardless of everything else, agreed?
Well, there is more to it I think.
IIRC, the AMD SME encryption mode has a way for a device to
have the
memory controller (?) encrypt / decrypt device traffic by using an
address range alias, so in theory it supports encrypted TT
pages, and
the dma-layer may indeed hand encrypted DMA pages to TTM on such
systems
depending on the device's DMA mask. That's why I think that
force_dma_unencrypted() export was needed, and If the amdgpu
driver
accesses TT memory in SME mode *without* pgprot_decrypted() and it
still
works, then I think that mode is actually used. How could it
otherwise work?
For SME, as long as the encrypted bit is set in the physical
address
used for DMA, the memory controller will handle the encrypt/decrypt
for the device. For devices with a limited dma mask, you need
to use
the IOMMU so that the encrypted bit is retained when the address
hits
the memory controller.
How does that work on systems with swiotlb, e.g. swiotlb=force, or
i.e. what would
decrypt the ttm tt mappings when copying between system and vram
when iommu is
disabled/absent?
SME makes it mandatory that all devices can handle the physical
address used for DMA, either native or with the help of IOMMU.
Hacks like SWIOTLB are not directly supported as far as I know. Maybe
somehow SWIOTLB manually decrypts the data while copying it or
something like this, but I'm not 100% sure if that is actually
implemented.
Regards,
Christian.
A bold guess after looking at various code and patches:
1) Devices under SME that don't support the encryption bit and SEV:
a) Coherent memory is unencrypted.
b) Streaming DMA under IOMMU: The IOMMU sets the encrypted bit.
c) Streaming DMA with SWIOTLB: The bounce buffer is unencrypted.
Copying to/from bounce-buffer decrypts/encrypts.
2) Devices under SME that do support the encryption bit (which I
believe is most graphics devices in general on SME systems, not just
amdgpu; it "just works")
*) Coherent memory is encrypted. The DMA layer sets dma addresses and
pgprot accordingly.
*) Streaming DMA is encrypted.
So the bug in TTM would then be it's not handling 1a) and 1b)
correctly.
Remedy:
1b) Shouldn't be used with encryption.
1a) This is what we should try to fix. Exporting
dma_force_unencrypted() didn't seem to be a way forward. Properly
fixing this would, I guess, mean implement the missing functionality
in the dma layer: For vmap / kmap we could simply reuse the virtual
addresses we get back from dma_alloc_coherent(), but for faulting one
would want something like dma_coherent_insert_pfn() (if it doesn't
exist already) after a proper disussion with Christoph Hellwig.
Christoph once pointed me to dma_mmap_attrs() for this, but I never
found the time to fully look into it.
Hmm, yea, that would make sense
https://elixir.bootlin.com/linux/latest/source/kernel/dma/direct.c#L564
Replacing the vmap's with dma_mmap_attrs would probably fix this, but
it would
require a bit of extra setup.
So we're saying that yes, we don't want unconditional pgprot_decrypt
in ttm_io_prot.
We'd like to leave those tt mappings as encrypted when possible and
instead maybe
add a vaddr to ttm_tt (or extract it from the pages->private via the
ttm_pool_dma,
but that seems rather ugly),
It could probably be extracted from pages->private from a helper in
the ttm pool code, (Christian has a final saying here). However, that
requires that all ttm_tts are built from a single dma_alloc chunk. Not
sure that's the case? In that case we're back to square zero for vmaps.
Nope they aren't and yes we are back to square one with that.
i think when we want to make this really clean we would need to forward
the page fault request to the ttm_pool so that it can call
dma_mmap_attrs() or similar.
But I'm not sure if dma_mmap_attrs() is even guaranteed to work with
reverse mapping.
plus add get_vm_area() to be able to use dma_mmap_attrs
instead of vmap when use_dma_alloc's is true in ttm_bo_vmap/ttm_bo_kmap?
vmap is for kernel mappings, and dma_mmap_attrs is for user-space
mappings, But dma_mmap_attrs()wouldn't be sufficient. If, for example.
a bo is moved from VRAM to dma-coherent memory, or vice-versa we Zap
the user-space page-table entries and replace them using faulting to
the new address. That's why we'd need a dma_coherent_insert_pfn().
Yes, exactly that.
I think at the time Christoph was under the impression that you could
replace xxx_insert_pfn() with xxx_mmap() in the fault handler, but
that doesn't work since the latter requires the mmap_lock in write
mode, whereas the fault handler only holds it in read mode. (That
caused some bugs in the i915 driver when that change was attempted).
Good to know.
Christian.
/Thomas
z
