Re: Decrypting tt maps in ttm

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

z






[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux