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 <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
---
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
diff --git a/arch/Kconfig b/arch/Kconfig
index c45b770d3579..3c5bf55ee752 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -1011,6 +1011,9 @@ config HAVE_ARCH_NVRAM_OPS
config ISA_BUS_API
def_bool ISA
+config ARCH_HAS_PROTECTED_GUEST
+ bool
+
#
# ABI hall of shame
#
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ff79263aebd1..d506aae29dd9 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -883,6 +883,7 @@ config INTEL_TDX_GUEST
select PARAVIRT_XL
select X86_X2APIC
select SECURITY_LOCKDOWN_LSM
+ select ARCH_HAS_PROTECTED_GUEST
help
Provide support for running in a trusted domain on Intel processors
equipped with Trusted Domain eXtenstions. TDX is a new Intel
@@ -1539,6 +1540,7 @@ config AMD_MEM_ENCRYPT
select ARCH_HAS_FORCE_DMA_UNENCRYPTED
select INSTRUCTION_DECODER
select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
+ select ARCH_HAS_PROTECTED_GUEST
help
Say yes to enable support for the encryption of system memory.
This requires an AMD processor that supports Secure Memory
diff --git a/arch/x86/include/asm/protected_guest.h b/arch/x86/include/asm/protected_guest.h
new file mode 100644
index 000000000000..d47668dee6c2
--- /dev/null
+++ b/arch/x86/include/asm/protected_guest.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright (C) 2020 Intel Corporation */
+#ifndef _ASM_PROTECTED_GUEST_H
+#define _ASM_PROTECTED_GUEST_H 1
+
+#include <asm/processor.h>
+#include <asm/tdx.h>
+#include <asm/sev.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);
+
+ return false;
+}
+
+#endif /* _ASM_PROTECTED_GUEST_H */
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index fa5cd05d3b5b..e9b0b93a3157 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -81,12 +81,15 @@ static __always_inline void sev_es_nmi_complete(void)
__sev_es_nmi_complete();
}
extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd);
+bool sev_protected_guest_has(unsigned long flag);
+
#else
static inline void sev_es_ist_enter(struct pt_regs *regs) { }
static inline void sev_es_ist_exit(void) { }
static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { return 0; }
static inline void sev_es_nmi_complete(void) { }
static inline int sev_es_efi_map_ghcbs(pgd_t *pgd) { return 0; }
+static inline bool sev_protected_guest_has(unsigned long flag) { return false; }
#endif
#endif
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index c738bde944d1..1c17c9080a2c 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -11,10 +11,14 @@
void __init tdx_early_init(void);
+bool tdx_protected_guest_has(unsigned long flag);
+
#else
static inline void tdx_early_init(void) { };
+static inline bool tdx_protected_guest_has(unsigned long flag) { return false; }
+
#endif /* CONFIG_INTEL_TDX_GUEST */
#endif /* _ASM_X86_TDX_H */
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 651b81cd648e..3e88576555d2 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -19,6 +19,7 @@
#include <linux/memblock.h>
#include <linux/kernel.h>
#include <linux/mm.h>
+#include <linux/protected_guest.h>
#include <asm/cpu_entry_area.h>
#include <asm/stacktrace.h>
@@ -1493,3 +1494,19 @@ bool __init handle_vc_boot_ghcb(struct pt_regs *regs)
while (true)
halt();
}
+
+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;
+ case PR_GUEST_SEV:
+ return sev_active();
+ }
+
+ return false;
+}
+EXPORT_SYMBOL_GPL(sev_protected_guest_has);
diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
index b1492e076168..ae3334a2b29d 100644
--- a/arch/x86/kernel/tdx.c
+++ b/arch/x86/kernel/tdx.c
@@ -4,6 +4,8 @@
#undef pr_fmt
#define pr_fmt(fmt) "x86/tdx: " fmt
+#include <linux/protected_guest.h>
+
#include <asm/tdx.h>
static inline bool cpuid_has_tdx_guest(void)
@@ -18,6 +20,21 @@ static inline bool cpuid_has_tdx_guest(void)
return !memcmp("IntelTDX ", sig, 12);
}
+bool tdx_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_SHARED_MAPPING_INIT:
+ case PR_GUEST_TDX:
+ return static_cpu_has(X86_FEATURE_TDX_GUEST);
+ }
+
+ return false;
+}
+EXPORT_SYMBOL_GPL(tdx_protected_guest_has);
+
void __init tdx_early_init(void)
{
if (!cpuid_has_tdx_guest())
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
+/* Encryption support is active */
+#define PR_GUEST_MEM_ENCRYPT_ACTIVE 0x1001
+/* Support for unrolled string IO */
+#define PR_GUEST_UNROLL_STRING_IO 0x1002
+/* Support for host memory encryption */
+#define PR_GUEST_HOST_MEM_ENCRYPT 0x1003
+/* Support for shared mapping initialization (after early init) */
+#define PR_GUEST_SHARED_MAPPING_INIT 0x1004
+
+#ifdef CONFIG_ARCH_HAS_PROTECTED_GUEST
+#include <asm/protected_guest.h>
+#else
+static inline bool prot_guest_has(unsigned long flag) { return false; }
+#endif
+
+#endif /* _LINUX_PROTECTED_GUEST_H */
--
2.25.1
On Fri, Jun 18, 2021 at 03:57:48PM -0700, Kuppuswamy Sathyanarayanan wrote:
> Add a generic way to check if we run with an encrypted guest,
Please use passive voice in your commit message: no "we" or "I", etc,
and describe your changes in imperative mood.
Also, pls read section "2) Describe your changes" in
Documentation/process/submitting-patches.rst for more details.
Bottom line is: personal pronouns are ambiguous in text, especially with
so many parties/companies/etc developing the kernel so let's avoid them
please.
> without requiring x86 specific ifdefs. This can then be used in
> non architecture specific code.
"... in arch-independent code." or so.
> prot_guest_has() is used to check for protected guest feature
> flags.
>
> Originally-by: Andi Kleen <[email protected]>
> Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> ---
>
> 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.
...
> diff --git a/arch/x86/include/asm/protected_guest.h b/arch/x86/include/asm/protected_guest.h
> new file mode 100644
> index 000000000000..d47668dee6c2
> --- /dev/null
> +++ b/arch/x86/include/asm/protected_guest.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright (C) 2020 Intel Corporation */
> +#ifndef _ASM_PROTECTED_GUEST_H
> +#define _ASM_PROTECTED_GUEST_H 1
#define _ASM_X86_PROTECTED_GUEST_H
> +
> +#include <asm/processor.h>
> +#include <asm/tdx.h>
> +#include <asm/sev.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);
s/protected/prot/
tdx_prot_guest_has
sev_prot_guest_has
...
> @@ -18,6 +20,21 @@ static inline bool cpuid_has_tdx_guest(void)
> return !memcmp("IntelTDX ", sig, 12);
> }
>
> +bool tdx_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_SHARED_MAPPING_INIT:
> + case PR_GUEST_TDX:
> + return static_cpu_has(X86_FEATURE_TDX_GUEST);
return cpu_feature_enabled(...)
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 6/24/21 8:01 AM, Borislav Petkov wrote:
> On Fri, Jun 18, 2021 at 03:57:48PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> Add a generic way to check if we run with an encrypted guest,
>
> Please use passive voice in your commit message: no "we" or "I", etc,
> and describe your changes in imperative mood.
>
> Also, pls read section "2) Describe your changes" in
> Documentation/process/submitting-patches.rst for more details.
>
> Bottom line is: personal pronouns are ambiguous in text, especially with
> so many parties/companies/etc developing the kernel so let's avoid them
> please.
I will fix this in next version. I will make sure to follow it in future
submissions.
>
>> without requiring x86 specific ifdefs. This can then be used in
>> non architecture specific code.
>
> "... in arch-independent code." or so.
I will fix this in next version.
>
>> prot_guest_has() is used to check for protected guest feature
>> flags.
>>
>> Originally-by: Andi Kleen <[email protected]>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
>> ---
>>
>> 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.
>
> ...
>
>> diff --git a/arch/x86/include/asm/protected_guest.h b/arch/x86/include/asm/protected_guest.h
>> new file mode 100644
>> index 000000000000..d47668dee6c2
>> --- /dev/null
>> +++ b/arch/x86/include/asm/protected_guest.h
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/* Copyright (C) 2020 Intel Corporation */
>> +#ifndef _ASM_PROTECTED_GUEST_H
>> +#define _ASM_PROTECTED_GUEST_H 1
>
> #define _ASM_X86_PROTECTED_GUEST_H
>
>> +
>> +#include <asm/processor.h>
>> +#include <asm/tdx.h>
>> +#include <asm/sev.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);
>
> s/protected/prot/
>
> tdx_prot_guest_has
> sev_prot_guest_has
Ok. I will make this change in next version.
>
> ...
>
>> @@ -18,6 +20,21 @@ static inline bool cpuid_has_tdx_guest(void)
>> return !memcmp("IntelTDX ", sig, 12);
>> }
>>
>> +bool tdx_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_SHARED_MAPPING_INIT:
>> + case PR_GUEST_TDX:
>> + return static_cpu_has(X86_FEATURE_TDX_GUEST);
>
> return cpu_feature_enabled(...)
I will use it in next version.
>
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
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 <[email protected]>
> Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> ---
>
> 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
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index c45b770d3579..3c5bf55ee752 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -1011,6 +1011,9 @@ config HAVE_ARCH_NVRAM_OPS
> config ISA_BUS_API
> def_bool ISA
>
> +config ARCH_HAS_PROTECTED_GUEST
> + bool
> +
> #
> # ABI hall of shame
> #
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index ff79263aebd1..d506aae29dd9 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -883,6 +883,7 @@ config INTEL_TDX_GUEST
> select PARAVIRT_XL
> select X86_X2APIC
> select SECURITY_LOCKDOWN_LSM
> + select ARCH_HAS_PROTECTED_GUEST
> help
> Provide support for running in a trusted domain on Intel processors
> equipped with Trusted Domain eXtenstions. TDX is a new Intel
> @@ -1539,6 +1540,7 @@ config AMD_MEM_ENCRYPT
> select ARCH_HAS_FORCE_DMA_UNENCRYPTED
> select INSTRUCTION_DECODER
> select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
> + select ARCH_HAS_PROTECTED_GUEST
> help
> Say yes to enable support for the encryption of system memory.
> This requires an AMD processor that supports Secure Memory
> diff --git a/arch/x86/include/asm/protected_guest.h b/arch/x86/include/asm/protected_guest.h
> new file mode 100644
> index 000000000000..d47668dee6c2
> --- /dev/null
> +++ b/arch/x86/include/asm/protected_guest.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright (C) 2020 Intel Corporation */
> +#ifndef _ASM_PROTECTED_GUEST_H
> +#define _ASM_PROTECTED_GUEST_H 1
> +
> +#include <asm/processor.h>
> +#include <asm/tdx.h>
> +#include <asm/sev.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?
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.
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.
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.
> +
> + return false;
> +}
> +
> +#endif /* _ASM_PROTECTED_GUEST_H */
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index fa5cd05d3b5b..e9b0b93a3157 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -81,12 +81,15 @@ static __always_inline void sev_es_nmi_complete(void)
> __sev_es_nmi_complete();
> }
> extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd);
> +bool sev_protected_guest_has(unsigned long flag);
> +
> #else
> static inline void sev_es_ist_enter(struct pt_regs *regs) { }
> static inline void sev_es_ist_exit(void) { }
> static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { return 0; }
> static inline void sev_es_nmi_complete(void) { }
> static inline int sev_es_efi_map_ghcbs(pgd_t *pgd) { return 0; }
> +static inline bool sev_protected_guest_has(unsigned long flag) { return false; }
> #endif
>
> #endif
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index c738bde944d1..1c17c9080a2c 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -11,10 +11,14 @@
>
> void __init tdx_early_init(void);
>
> +bool tdx_protected_guest_has(unsigned long flag);
> +
> #else
>
> static inline void tdx_early_init(void) { };
>
> +static inline bool tdx_protected_guest_has(unsigned long flag) { return false; }
> +
> #endif /* CONFIG_INTEL_TDX_GUEST */
>
> #endif /* _ASM_X86_TDX_H */
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index 651b81cd648e..3e88576555d2 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -19,6 +19,7 @@
> #include <linux/memblock.h>
> #include <linux/kernel.h>
> #include <linux/mm.h>
> +#include <linux/protected_guest.h>
>
> #include <asm/cpu_entry_area.h>
> #include <asm/stacktrace.h>
> @@ -1493,3 +1494,19 @@ bool __init handle_vc_boot_ghcb(struct pt_regs *regs)
> while (true)
> halt();
> }
> +
> +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):
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.
> + case PR_GUEST_SEV:
> + return sev_active();
> + }
> +
> + return false;
> +}
> +EXPORT_SYMBOL_GPL(sev_protected_guest_has);
> diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
> index b1492e076168..ae3334a2b29d 100644
> --- a/arch/x86/kernel/tdx.c
> +++ b/arch/x86/kernel/tdx.c
> @@ -4,6 +4,8 @@
> #undef pr_fmt
> #define pr_fmt(fmt) "x86/tdx: " fmt
>
> +#include <linux/protected_guest.h>
> +
> #include <asm/tdx.h>
>
> static inline bool cpuid_has_tdx_guest(void)
> @@ -18,6 +20,21 @@ static inline bool cpuid_has_tdx_guest(void)
> return !memcmp("IntelTDX ", sig, 12);
> }
>
> +bool tdx_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_SHARED_MAPPING_INIT:
> + case PR_GUEST_TDX:
> + return static_cpu_has(X86_FEATURE_TDX_GUEST);
> + }
> +
> + return false;
> +}
> +EXPORT_SYMBOL_GPL(tdx_protected_guest_has);
> +
> void __init tdx_early_init(void)
> {
> if (!cpuid_has_tdx_guest())
> 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)?
Thanks,
Tom
> +/* Encryption support is active */
> +#define PR_GUEST_MEM_ENCRYPT_ACTIVE 0x1001
> +/* Support for unrolled string IO */
> +#define PR_GUEST_UNROLL_STRING_IO 0x1002
> +/* Support for host memory encryption */
> +#define PR_GUEST_HOST_MEM_ENCRYPT 0x1003
> +/* Support for shared mapping initialization (after early init) */
> +#define PR_GUEST_SHARED_MAPPING_INIT 0x1004
> +
> +#ifdef CONFIG_ARCH_HAS_PROTECTED_GUEST
> +#include <asm/protected_guest.h>
> +#else
> +static inline bool prot_guest_has(unsigned long flag) { return false; }
> +#endif
> +
> +#endif /* _LINUX_PROTECTED_GUEST_H */
>
On 6/28/21 12:52 PM, Tom Lendacky wrote:
> On 6/18/21 5:57 PM, Kuppuswamy Sathyanarayanan wrote:
>> +
>> +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?
>
> 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.
>
> 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.
>
> 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.
>
Or keep the separate vendor routines for separation and easier testing
but, instead, they would have to key off of the support:
if (static_cpu_has(X86_FEATURE_TDX_GUEST))
return tdx_prot_guest_has(flag);
else if (sme_active() || sev_active())
return sev_prot_guest_has(flag);
Thanks,
Tom
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 <[email protected]>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
>> ---
>>
>> 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
On 6/28/21 2:14 PM, Kuppuswamy, Sathyanarayanan wrote:
>
>
> On 6/28/21 10:52 AM, Tom Lendacky wrote:
>> On 6/18/21 5:57 PM, Kuppuswamy Sathyanarayanan wrote:
>>> +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.
Hmmm..., I must be missing something then. I thought early_identify_cpu()
will read the signature, which would be "IntelTDX ", right? Then that
is be compared against the structs that register via cpu_dev_register()
which would contain the x86_vendor value. I don't see anything registering
with the "IndexTDX " signature so I don't know how you'll get an
x86_vendor value of X86_VENDOR_INTEL.
I'm probably missing something there, but it shouldn't matter for this
routine going forward.
>
>>
>> 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?
No, we don't.
>
>> 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).
Yeah, this is the problem with the name having guest in everything when
there are host and guest scenarios for AMD.
We have PR_GUEST_HOST_MEM_ENCRYPT but it would look strange to have
PR_GUEST_GUEST_MEM_ENCRYPT.
>
> 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?
So I can work on a pre-patch series. It will be purely a replacement for
the current SME/SEV calls. You'll need to add all of the TDX support in a
subsequent patch in the TDX series. Given this is a pre-patch, I will
probably reset the flag values slightly and work on the names to be less
confusing.
Thanks,
Tom