[RFC PATCH] drm/ttm, drm/vmwgfx: Have TTM support AMD SEV encryption

[Christian König]

Am 28.05.19 um 19:23 schrieb Lendacky, Thomas:
> On 5/28/19 12:05 PM, Thomas Hellstrom wrote:
>> On 5/28/19 7:00 PM, Lendacky, Thomas wrote:
>>> On 5/28/19 11:32 AM, Koenig, Christian wrote:
>>>> Am 28.05.19 um 18:27 schrieb Thomas Hellstrom:
>>>>> On Tue, 2019-05-28 at 15:50 +0000, Lendacky, Thomas wrote:
>>>>>> On 5/28/19 10:17 AM, Koenig, Christian wrote:
>>>>>>> Hi Thomas,
>>>>>>>
>>>>>>> Am 28.05.19 um 17:11 schrieb Thomas Hellstrom:
>>>>>>>> Hi, Tom,
>>>>>>>>
>>>>>>>> Thanks for the reply. The question is not graphics specific, but
>>>>>>>> lies
>>>>>>>> in your answer further below:
>>>>>>>>
>>>>>>>> On 5/28/19 4:48 PM, Lendacky, Thomas wrote:
>>>>>>>>> On 5/28/19 2:31 AM, Thomas Hellstrom wrote:
>>>>>>>>> [SNIP]
>>>>>>>>> As for kernel vmaps and user-maps, those pages will be marked
>>>>>>>>> encrypted
>>>>>>>>> (unless explicitly made un-encrypted by calling
>>>>>>>>> set_memory_decrypted()).
>>>>>>>>> But, if you are copying to/from those areas into the un-
>>>>>>>>> encrypted DMA
>>>>>>>>> area then everything will be ok.
>>>>>>>> The question is regarding the above paragraph.
>>>>>>>>
>>>>>>>> AFAICT,  set_memory_decrypted() only changes the fixed kernel map
>>>>>>>> PTEs.
>>>>>>>> But when setting up other aliased PTEs to the exact same
>>>>>>>> decrypted
>>>>>>>> pages, for example using dma_mmap_coherent(),
>>>>>>>> kmap_atomic_prot(),
>>>>>>>> vmap() etc. What code is responsible for clearing the encrypted
>>>>>>>> flag
>>>>>>>> on those PTEs? Is there something in the x86 platform code doing
>>>>>>>> that?
>>>>>>> Tom actually explained this:
>>>>>>>> The encryption bit is bit-47 of a physical address.
>>>>>>> In other words set_memory_decrypted() changes the physical address
>>>>>>> in
>>>>>>> the PTEs of the kernel mapping and all other use cases just copy
>>>>>>> that
>>>>>>> from there.
>>>>>> Except I don't think the PTE attributes are copied from the kernel
>>>>>> mapping
>>>>> +1!
>>>>>
>>>>>> in some cases. For example, dma_mmap_coherent() will create the same
>>>>>> vm_page_prot value regardless of whether or not the underlying memory
>>>>>> is
>>>>>> encrypted or not. But kmap_atomic_prot() will return the kernel
>>>>>> virtual
>>>>>> address of the page, so that would be fine.
>>>>> Yes, on 64-bit systems. On 32-bit systems (do they exist with SEV?),
>>>>> they don't.
>>>> I don't think so, but feel free to prove me wrong Tom.
>>> SEV is 64-bit only.
>> And I just noticed that kmap_atomic_prot() indeed returns the kernel map
>> also for 32-bit lowmem.
>>
>>>>> And similarly TTM user-space mappings and vmap() doesn't copy from the
>>>>> kernel map either,  so I think we actually do need to modify the page-
>>>>> prot like done in the patch.
>>>> Well the problem is that this won't have any effect.
>>>>
>>>> As Tom explained encryption is not implemented as a page protection bit,
>>>> but rather as part of the physical address of the part.
>>> This is where things get interesting.  Even though the encryption bit is
>>> part of the physical address (e.g. under SME the device could/would use an
>>> address with the encryption bit set), it is implemented as part of the PTE
>>> attributes. So, for example, using _PAGE_ENC when building a pgprot value
>>> would produce an entry with the encryption bit set.
>>>
>>> And the thing to watch out for is using two virtual addresses that point
>>> to the same physical address (page) in DRAM but one has the encryption bit
>>> set and one doesn't. The hardware does not enforce coherency between an
>>> encrypted and un-encrypted mapping of the same physical address (page).
>>> See section 7.10.6 of the AMD64 Architecture Programmer's Manual Volume 2.
>> Indeed. And I'm pretty sure the kernel map PTE and a TTM / vmap PTE
>> pointing to the same decrypted page differ in the encryption bit (47)
>> setting.

That can't be the case, cause otherwise amdgpu wouldn't already work 
with SME enabled.

I think your patch might go into the right direction, but before we 
commit anything like that we need to understand first how it actually 
works currently.

Maybe I really need to setup a test system here.

Christian.

>>
>> But on the hypervisor that would sort of work, because from what I
>> understand with SEV we select between the guest key and the hypervisor
>> key with that bit. On the hypervisor both keys are the same? On a guest
>> it would probably break.
> For SEV, if the encryption bit is set then the guest key is used. If the
> encryption bit is not set, then the hypervisor key is used IFF the
> encryption bit is set in the hypervisor page tables.  You can have SEV
> guests regardless of whether SME is active (note, there is a difference
> between SME being enabled vs. SME being active).
>
> For SME, there is only one key. The encryption bit determines whether the
> data is run through the encryption process or not.
>
> Thanks,
> Tom