Decrypting tt maps in ttm

Fri Sep 22 10:46:01 UTC 2023

Am 21.09.23 um 09:12 schrieb Thomas Hellström:
>
> On 9/21/23 05:51, Zack Rusin wrote:
>> On Wed, 2023-09-20 at 21:22 +0200, Thomas Hellström wrote:
>>> !! External Email
>>>
>>> On 9/20/23 20:24, Zack Rusin wrote:
>>>> On Wed, 2023-09-20 at 19:17 +0200, Thomas Hellström wrote:
>>>>> !! External Email
>>>>>
>>>>> Hi, Zack
>>>>>
>>>>> On 9/20/23 18:39, Zack Rusin wrote:
>>>>>> On Wed, 2023-09-20 at 12:48 +0200, Christian König wrote:
>>>>>>> !! External Email
>>>>>>>
>>>>>>> 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 at 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.
>>>>>>>>> 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.
>>>>>> Well, that's my favorite square. Number one, just like me...
>>>>>>
>>>>>> Maybe we're overthinking this particular problem a bit. As is 
>>>>>> use_dma_alloc
>>>>>> in
>>>>>> ttm
>>>>>> is only set in two cases:
>>>>>> - driver explicitly wants coherent mappings (vmwgfx, which 
>>>>>> require decrypted
>>>>>> pages)
>>>>>> - driver needs swiotlb (which, as was pointed out, would require 
>>>>>> the pages
>>>>>> to be
>>>>>> decrypted as well)
>>>>>>
>>>>>> So use_dma_alloc always requires the pages to be decrypted.
>>>>> IIRC moving forward it doesn't, since there is (or at least there 
>>>>> was)
>>>>> implement missing TTM functionality in the dma layer and most TTM
>>>>> drivers should at least support dma coherent memory. That means all
>>>>> devices supporting a sufficiently large dma mask will break with 
>>>>> SME and
>>>>> your proposal then.
>>>>>
>>>>> Perhaps if we condition that on
>>>>> "cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)" that will capture all 
>>>>> the
>>>>> SEV cases, and limit the existing bug to the hopefully very few TTM
>>>>> devices with limited dma mask on SME.
>>>> Ah, I wasn't aware those exist, do you know what platforms are 
>>>> those? I can try
>>>> to
>>>> find one around here to see.
>>> My understanding is cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) will
>>> return true in the guest iff SEV is active, and can be used in TTM as a
>>> poor man's force_dma_unencrypted(), enabling the functionality in your
>>> diff. It looks like a similar check is present in vmwgfx to detect SEV,
>>> but also see below.
>>>
>>>> And they don't really break, they just might unnecessarily decrypt 
>>>> tt pages,
>>>> right?
>>> No, with SME, dma from hw will encrypt the content, because the dma
>>> layer will set the "encrypt" bit in the physical address given to the
>>> iommu or the device in case iommu is not active, but a subsequent
>>> reading the content using the CPU won't decrypt so CPU and device will
>>> have different views of the page.
>>>
>>> Also the linear kernel mapping PTEs will conflict in encryption mode
>>> with the ones TTM sets up, and IIRC that's forbidden in the SEV spec.
>>> (The x86 arch code goes through some serious work to flush out caches
>>> and TLBs to convert a page kernel linear mapping from encrypted to
>>> non-encrypted,
>>>
>>> https://elixir.bootlin.com/linux/latest/source/arch/x86/mm/pat/set_memory.c#L2129 
>>>
>>>
>>> and that is also seen as pretty heavy dma_alloc_coherent() latency).
>> Thanks for this!
>>
>>> So the pgprot_t TTM sets up *must* be identical to the one used by the
>>> dma layer, so anything we should be aware here that anything we do in
>>> TTM less than adding needed functionality in the dma layer is
>>> second-guessing what the dma layer does internally and is not really 
>>> the
>>> right solution.
>> I think this is already the case for virtualized drivers, but I see 
>> what you're
>> saying that fixing this for them might break some real hardware and 
>> that's bad.
>> Playing those games with matching pgprot between ttm and dma is 
>> really fragile.
>>
>> cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) is also used in 
>> drm_need_swiotlb so
>> adding that check to the last patch would seem to make sense. Of 
>> course, it's up to
>> Christian whether that's robust enough or whether we need to think 
>> about the
>> dma/page fault rework to fix it properly. I'm not sure if I see any 
>> other reasonable
>> solution besides these two options.
> Agreed.

Uff, of hand that looks like the right thing to do. But I'm really not 
an expert for that stuff.

I think the best thing you can do is to write a patch and send it to 
LKML and dri-devel and see if anybody objects.

Christian.

>
> /Thomas
>
>
>>
>> z