lkml.org 
[lkml]   [2021]   [May]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC v2 28/32] x86/tdx: Make pages shared in ioremap()
From
Date
On 5/21/21 10:18 AM, Borislav Petkov wrote:
> On Thu, May 20, 2021 at 01:12:58PM -0700, Kuppuswamy, Sathyanarayanan wrote:
>> I see many variants of SEV/SME related checks in the common code path
>> between TDX and SEV/SME. Can a generic call like
>> protected_guest_has(MEMORY_ENCRYPTION) or is_protected_guest()
>> replace all these variants?
>
> It depends...
>
>> We will not be able to test AMD related features. So I need to confirm
>> it with AMD code maintainers/developers before making this change.
>
> Lemme add two to Cc.
>
> So looking at those examples, you guys are making it not very
> suspenceful for TDX - it is the same function in all. :)
>
>> arch/x86/include/asm/io.h:313: if (sev_key_active() || is_tdx_guest()) { \
>> arch/x86/include/asm/io.h:329: if (sev_key_active() || is_tdx_guest()) { \
>
> So I think the static key on the AMD side is not really needed and it
> could be replaced with
>
> sev_active() && !sev_es_active()
>
> i.e. SEV but but not SEV-ES. A vendor-agnostic function would do here
> probably something like:
>
> protected_guest_has(ENC_UNROLL_STRING_IO)
>
> and inside it, it would do:
>
> if (AMD)
> amd_protected_guest_has(...)
> else if (Intel)
> intel_protected_guest_has(...)
> else
> WARN()
>
> and both vendors would each implement that function with the respective
> low-level query functions.
>
>> arch/x86/kernel/pci-swiotlb.c:52: if (sme_active() || is_tdx_guest())
>
> That can be probably
>
> protected_guest_has(ENC_HOST_MEM_ENCRYPT);
>
> as on AMD that means SME but not SEV. I guess on Intel you guys want to
> do bounce buffers in the guest? or so...

In arch/x86/mm/mem_encrypt.c, sme_early_init() (should have renamed that
when SEV support was added), we do:
if (sev_active())
swiotlb_force = SWIOTLB_FORCE;

TDX should be able to do a similar thing without having to touch
arch/x86/kernel/pci-swiotlb.c.

That would remove any confusion over SME being part of a
protected_guest_has() call.

>
>> arch/x86/mm/ioremap.c:96: if (!sev_active() && !is_tdx_guest())
>
> So that function should simply be replaced with:
>
> if (!(desc->flags & IORES_MAP_ENCRYPTED)) {
> /* ... comment bla explaining what this is... */
> if ((sev_active() || is_tdx_guest()) &&
> (res->desc != IORES_DESC_NONE &&
> res->desc != IORES_DESC_RESERVED))
> desc->flags |= IORES_MAP_ENCRYPTED;
> }

I kinda like the separate function, though.

>
> as to the first check I guess:
>
> protected_guest_has(ENC_GUEST_ENABLED)
>
> or so to mean, kernel is running as an encrypted guest...
>
>> arch/x86/mm/pat/set_memory.c:1984: if (!mem_encrypt_active() && !is_tdx_guest())
>
> That should probably be
>
> protected_guest_has(ENC_ACTIVE);
>
> to denote the generic "I'm running some sort of memory encryption..."

Except mem_encrypt_active() covers both SME and SEV, so
protected_guest_has() would be confusing.

Thanks,
Tom

>
> Yeah, this is all rough and should show the main idea - to have a
> vendor-agnostic accessor in such common code paths and then abstract
> away the differences in cpu/amd.c and cpu/intel.c, respectively and thus
> keep the code sane.
>
> How does that sound?
>
> ENC_ being an ENCryption prefix, ofc.
>

\
 
 \ /
  Last update: 2021-05-21 18:20    [W:1.714 / U:0.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site