Re: Decrypting tt maps in ttm

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

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@linux.intel.com> 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.

/Thomas


>
>>
>> z
>>
>