Re: [RFC v2 28/32] x86/tdx: Make pages shared in ioremap()

[Tom Lendacky]

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