Messages in this thread | | | Subject | Re: [RFC v2 28/32] x86/tdx: Make pages shared in ioremap() | From | Tom Lendacky <> | Date | Fri, 21 May 2021 11:19:15 -0500 |
| |
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. >
| |