Re: Decrypting tt maps in ttm

["Christian König" <christian.koenig@amd.com>]

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

Christoph once pointed me to dma_mmap_attrs() for this, but I never 
found the time to fully look into it.

Christian.

>
> /Thomas
>
>
>>
>>>
>>> z
>>>
>>