Re: [PATCH v3 04/11] x86: Introduce generic protected guest abstraction
From: Kuppuswamy, Sathyanarayanan
Date: Mon Jun 28 2021 - 15:15:00 EST
On 6/28/21 10:52 AM, Tom Lendacky wrote:
On 6/18/21 5:57 PM, Kuppuswamy Sathyanarayanan wrote:
Add a generic way to check if we run with an encrypted guest,
without requiring x86 specific ifdefs. This can then be used in
non architecture specific code.
prot_guest_has() is used to check for protected guest feature
flags.
Originally-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
---
Change since v1:
* Introduced PR_GUEST_TDX and PR_GUEST_SEV vendor flags as per
Boris suggestion.
* Replaced is_tdx_guest() with if (boot_cpu_data.x86_vendor ==
X86_VENDOR_INTEL) in prot_guest_has().
* Modified tdx_protected_guest_has() and sev_protected_guest_has()
to support vendor flags.
arch/Kconfig | 3 +++
arch/x86/Kconfig | 2 ++
arch/x86/include/asm/protected_guest.h | 20 +++++++++++++++++
arch/x86/include/asm/sev.h | 3 +++
arch/x86/include/asm/tdx.h | 4 ++++
arch/x86/kernel/sev.c | 17 +++++++++++++++
arch/x86/kernel/tdx.c | 17 +++++++++++++++
include/linux/protected_guest.h | 30 ++++++++++++++++++++++++++
8 files changed, 96 insertions(+)
create mode 100644 arch/x86/include/asm/protected_guest.h
create mode 100644 include/linux/protected_guest.h
+static inline bool prot_guest_has(unsigned long flag)
+{
+ if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
+ return tdx_protected_guest_has(flag);
+ else if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
+ return sev_protected_guest_has(flag);
So as I think about this, I don't think this will work if the hypervisor
decides to change the vendor name, right?
For TDX guest, vendor name cannot be changed. It is set by TDX module and
it is fixed as per TDX module spec.
And doesn't TDX supply "IntelTDX " as a signature. I don't see where
the signature is used to set the CPU vendor to X86_VENDOR_INTEL.
We don't need to specially handle it for TDX. Generic early_identify_cpu() will
set boot_cpu_data.x86_vendor as X86_VENDOR_INTEL for TDX guest. I think it is
based on Intel in vendor string.
The current SEV checks to set sev_status, which is used by sme_active(),
sev_active, etc.) are based on the max leaf and CPUID bits, but not a
CPUID vendor check.
You also set x86_vendor id as AMD based on SEV checks?
So maybe we can keep the prot_guest_has() but I think it will have to be a
common routine, with a "switch" statement that has supporting case element
that check for "sev_active() || static_cpu_has(X86_FEATURE_TDX_GUEST)", etc.
}
+
+bool sev_protected_guest_has(unsigned long flag)
+{
+ switch (flag) {
+ case PR_GUEST_MEM_ENCRYPT:
+ case PR_GUEST_MEM_ENCRYPT_ACTIVE:
+ case PR_GUEST_UNROLL_STRING_IO:
+ case PR_GUEST_HOST_MEM_ENCRYPT:
+ return true;
This will need to be fixed up because this function will be called for
baremetal and legacy guests and those properties aren't true for those
situations. Something like (although I'm unsure of the difference between
PR_GUEST_MEM_ENCRYPT and PR_GUEST_MEM_ENCRYPT_ACTIVE):
MEM_ENCRYPT_ACTIVE is suggested for mem_encrypt_active() case (I think it
means some sort of encryption is active).
PR_GUEST_MEM_ENCRYPT means guest supports memory encryption (sev_active()
case).
Yes, I can include following changes in next version.
case PR_GUEST_MEM_ENCRYPT:
case PR_GUEST_MEM_ENCRYPT_ACTIVE:
return sev_active();
case PR_GUEST_UNROLL_STRING_IO:
return sev_active() && !sev_es_active();
case PR_GUEST_HOST_MEM_ENCRYPT:
return sme_active();
But you (or I) would have to audit all of the locations where
mem_encrypt_active(), sme_active(), sev_active() and sev_es_active() are
used, to be sure the right thing is being done. And for bisectability,
that should probably be the first patch if you will be invoking
prot_guest_has() in the same location as any of the identified functions.
Create the new helper and fixup the locations should be one (or more)
patches. Then add the TDX support to the helper function as a follow-on patch.
Can you submit a patch to replace all existing uses cases of mem_encrypt_active()
,sme_active(), sev_active() and sev_es_active() with prot_guest_has() calls? Since
I cannot test any of these changes for AMD, it would be better if you could do it.
Once you submit a tested version, I can enable these features for TDX and test
and submit it separately.
This patch can be split as below:
1. x86: Introduce generic protected guest abstraction patch (with below changes).
- Remove all PR_GUEST flags in sev_protected_guest_has() and
tdx_protected_guest_has().
2. Patch from you to use prot_guest_has() for AMD code and enable relevant
PR_GUEST flags in sev_protected_guest_has().
3. Patch from me to us prot_guest_has() for TDX cases and enable relevant
PR_GUEST flags in tdx_protected_guest_has().
Agree?
diff --git a/include/linux/protected_guest.h b/include/linux/protected_guest.h
new file mode 100644
index 000000000000..c5b7547e5a68
--- /dev/null
+++ b/include/linux/protected_guest.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _LINUX_PROTECTED_GUEST_H
+#define _LINUX_PROTECTED_GUEST_H 1
+
+/* Protected Guest Feature Flags (leave 0-0xfff for vendor specific flags) */
+
+/* 0-ff is reserved for Intel specific flags */
+#define PR_GUEST_TDX 0x0000
+
+/* 100-1ff is reserved for AMD specific flags */
+#define PR_GUEST_SEV 0x0100
+
+/* Support for guest encryption */
+#define PR_GUEST_MEM_ENCRYPT 0x1000
I'm not sure I follow the difference between this and
PR_GUEST_MEM_ENCRYPT_ACTIVE. Is this saying that the host has support for
starting guests that support memory encryption or the guest has support
for memory encryption but it hasn't been activated yet (which doesn't seem
possible)?
Explained it above.
Thanks,
Tom
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer