* [PATCHv2 0/4] x86/tdx: Adjust TD settings on boot
@ 2024-03-25 10:46 Kirill A. Shutemov
2024-03-25 10:46 ` [PATCHv2 1/4] x86/tdx: Introduce tdg_vm_wr() Kirill A. Shutemov
` (4 more replies)
0 siblings, 5 replies; 17+ messages in thread
From: Kirill A. Shutemov @ 2024-03-25 10:46 UTC (permalink / raw)
To: tglx, mingo, bp, dave.hansen
Cc: sathyanarayanan.kuppuswamy, hpa, seanjc, elena.reshetova,
rick.p.edgecombe, x86, linux-kernel, Kirill A. Shutemov
Adjust TD setting on boot:
- Disable EPT violation #VE on private memory if TD can
control it;
- Enable virtualization of topology-related CPUID leafs
X2APIC_APICID MSR;
v2:
- Rebased;
- Allow write to TDCS_TD_CTLS to fail;
- Adjust commit messages;
Kirill A. Shutemov (4):
x86/tdx: Introduce tdg_vm_wr()
x86/tdx: Rename tdx_parse_tdinfo() to tdx_setup()
x86/tdx: Handle PENDING_EPT_VIOLATION_V2
x86/tdx: Enable ENUM_TOPOLOGY
arch/x86/coco/tdx/tdx.c | 125 ++++++++++++++++++++++++++----
arch/x86/include/asm/shared/tdx.h | 21 ++++-
2 files changed, 131 insertions(+), 15 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCHv2 1/4] x86/tdx: Introduce tdg_vm_wr()
2024-03-25 10:46 [PATCHv2 0/4] x86/tdx: Adjust TD settings on boot Kirill A. Shutemov
@ 2024-03-25 10:46 ` Kirill A. Shutemov
2024-03-26 10:45 ` Huang, Kai
` (2 more replies)
2024-03-25 10:46 ` [PATCHv2 2/4] x86/tdx: Rename tdx_parse_tdinfo() to tdx_setup() Kirill A. Shutemov
` (3 subsequent siblings)
4 siblings, 3 replies; 17+ messages in thread
From: Kirill A. Shutemov @ 2024-03-25 10:46 UTC (permalink / raw)
To: tglx, mingo, bp, dave.hansen
Cc: sathyanarayanan.kuppuswamy, hpa, seanjc, elena.reshetova,
rick.p.edgecombe, x86, linux-kernel, Kirill A. Shutemov
Add a helper function to write to a TD-scope metadata field and use it
to set NOTIFY_ENABLES.
The helper function will be paired with tdg_vm_rd() and will be used to
manipulate other metadata fields, not just NOTIFY_ENABLES.
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
arch/x86/coco/tdx/tdx.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 59776ce1c1d7..4fb36e5c4e80 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -77,6 +77,20 @@ static inline void tdcall(u64 fn, struct tdx_module_args *args)
panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
}
+static inline u64 tdg_vm_wr(u64 field, u64 value, u64 mask)
+{
+ struct tdx_module_args args = {
+ .rdx = field,
+ .r8 = value,
+ .r9 = mask,
+ };
+
+ tdcall(TDG_VM_WR, &args);
+
+ /* Old value */
+ return args.r8;
+}
+
/**
* tdx_mcall_get_report0() - Wrapper to get TDREPORT0 (a.k.a. TDREPORT
* subtype 0) using TDG.MR.REPORT TDCALL.
@@ -902,10 +916,6 @@ static void tdx_kexec_unshare_mem(void)
void __init tdx_early_init(void)
{
- struct tdx_module_args args = {
- .rdx = TDCS_NOTIFY_ENABLES,
- .r9 = -1ULL,
- };
u64 cc_mask;
u32 eax, sig[3];
@@ -924,7 +934,7 @@ void __init tdx_early_init(void)
cc_set_mask(cc_mask);
/* Kernel does not use NOTIFY_ENABLES and does not need random #VEs */
- tdcall(TDG_VM_WR, &args);
+ tdg_vm_wr(TDCS_NOTIFY_ENABLES, 0, -1ULL);
/*
* All bits above GPA width are reserved and kernel treats shared bit
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCHv2 2/4] x86/tdx: Rename tdx_parse_tdinfo() to tdx_setup()
2024-03-25 10:46 [PATCHv2 0/4] x86/tdx: Adjust TD settings on boot Kirill A. Shutemov
2024-03-25 10:46 ` [PATCHv2 1/4] x86/tdx: Introduce tdg_vm_wr() Kirill A. Shutemov
@ 2024-03-25 10:46 ` Kirill A. Shutemov
2024-03-26 10:45 ` Huang, Kai
2024-03-25 10:46 ` [PATCHv2 3/4] x86/tdx: Handle PENDING_EPT_VIOLATION_V2 Kirill A. Shutemov
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Kirill A. Shutemov @ 2024-03-25 10:46 UTC (permalink / raw)
To: tglx, mingo, bp, dave.hansen
Cc: sathyanarayanan.kuppuswamy, hpa, seanjc, elena.reshetova,
rick.p.edgecombe, x86, linux-kernel, Kirill A. Shutemov
Rename tdx_parse_tdinfo() to tdx_setup() and move setting NOTIFY_ENABLES
there.
The function will be extended to adjust TD configuration.
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
arch/x86/coco/tdx/tdx.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 4fb36e5c4e80..08e2bb462ce8 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -181,7 +181,7 @@ static void __noreturn tdx_panic(const char *msg)
__tdx_hypercall(&args);
}
-static void tdx_parse_tdinfo(u64 *cc_mask)
+static void tdx_setup(u64 *cc_mask)
{
struct tdx_module_args args = {};
unsigned int gpa_width;
@@ -206,6 +206,9 @@ static void tdx_parse_tdinfo(u64 *cc_mask)
gpa_width = args.rcx & GENMASK(5, 0);
*cc_mask = BIT_ULL(gpa_width - 1);
+ /* Kernel does not use NOTIFY_ENABLES and does not need random #VEs */
+ tdg_vm_wr(TDCS_NOTIFY_ENABLES, 0, -1ULL);
+
/*
* The kernel can not handle #VE's when accessing normal kernel
* memory. Ensure that no #VE will be delivered for accesses to
@@ -930,11 +933,11 @@ void __init tdx_early_init(void)
setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
cc_vendor = CC_VENDOR_INTEL;
- tdx_parse_tdinfo(&cc_mask);
- cc_set_mask(cc_mask);
- /* Kernel does not use NOTIFY_ENABLES and does not need random #VEs */
- tdg_vm_wr(TDCS_NOTIFY_ENABLES, 0, -1ULL);
+ /* Configure the TD */
+ tdx_setup(&cc_mask);
+
+ cc_set_mask(cc_mask);
/*
* All bits above GPA width are reserved and kernel treats shared bit
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCHv2 3/4] x86/tdx: Handle PENDING_EPT_VIOLATION_V2
2024-03-25 10:46 [PATCHv2 0/4] x86/tdx: Adjust TD settings on boot Kirill A. Shutemov
2024-03-25 10:46 ` [PATCHv2 1/4] x86/tdx: Introduce tdg_vm_wr() Kirill A. Shutemov
2024-03-25 10:46 ` [PATCHv2 2/4] x86/tdx: Rename tdx_parse_tdinfo() to tdx_setup() Kirill A. Shutemov
@ 2024-03-25 10:46 ` Kirill A. Shutemov
2024-04-24 18:29 ` Dave Hansen
2024-03-25 10:46 ` [PATCHv2 4/4] x86/tdx: Enable ENUM_TOPOLOGY Kirill A. Shutemov
2024-04-10 14:37 ` [PATCHv2 0/4] x86/tdx: Adjust TD settings on boot Kirill A. Shutemov
4 siblings, 1 reply; 17+ messages in thread
From: Kirill A. Shutemov @ 2024-03-25 10:46 UTC (permalink / raw)
To: tglx, mingo, bp, dave.hansen
Cc: sathyanarayanan.kuppuswamy, hpa, seanjc, elena.reshetova,
rick.p.edgecombe, x86, linux-kernel, Kirill A. Shutemov
PENDING_EPT_VIOLATION_V2 allows TD to control whether access to
a pending page triggers #VE.
Kernel doesn't want to see any #VEs on accesses to private memory:
disable such #VEs.
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
arch/x86/coco/tdx/tdx.c | 75 ++++++++++++++++++++++++++++---
arch/x86/include/asm/shared/tdx.h | 18 +++++++-
2 files changed, 87 insertions(+), 6 deletions(-)
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 08e2bb462ce8..860bfdd5a11d 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -77,6 +77,17 @@ static inline void tdcall(u64 fn, struct tdx_module_args *args)
panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
}
+static inline u64 tdg_vm_rd(u64 field)
+{
+ struct tdx_module_args args = {
+ .rdx = field,
+ };
+
+ tdcall(TDG_VM_RD, &args);
+
+ return args.r8;
+}
+
static inline u64 tdg_vm_wr(u64 field, u64 value, u64 mask)
{
struct tdx_module_args args = {
@@ -91,6 +102,28 @@ static inline u64 tdg_vm_wr(u64 field, u64 value, u64 mask)
return args.r8;
}
+static inline u64 tdg_sys_rd(u64 field)
+{
+ struct tdx_module_args args = {
+ .rdx = field,
+ };
+
+ tdcall(TDG_SYS_RD, &args);
+
+ return args.r8;
+}
+
+static bool tdcs_ctls_set(u64 mask)
+{
+ struct tdx_module_args args = {
+ .rdx = TDCS_TD_CTLS,
+ .r8 = mask,
+ .r9 = mask,
+ };
+
+ return __tdcall(TDG_VM_WR, &args) == TDX_SUCCESS;
+}
+
/**
* tdx_mcall_get_report0() - Wrapper to get TDREPORT0 (a.k.a. TDREPORT
* subtype 0) using TDG.MR.REPORT TDCALL.
@@ -185,7 +218,8 @@ static void tdx_setup(u64 *cc_mask)
{
struct tdx_module_args args = {};
unsigned int gpa_width;
- u64 td_attr;
+ u64 td_attr, features;
+ bool sept_ve_disabled;
/*
* TDINFO TDX module call is used to get the TD execution environment
@@ -206,19 +240,50 @@ static void tdx_setup(u64 *cc_mask)
gpa_width = args.rcx & GENMASK(5, 0);
*cc_mask = BIT_ULL(gpa_width - 1);
+ td_attr = args.rdx;
+
/* Kernel does not use NOTIFY_ENABLES and does not need random #VEs */
tdg_vm_wr(TDCS_NOTIFY_ENABLES, 0, -1ULL);
+ features = tdg_sys_rd(TDCS_TDX_FEATURES0);
+
/*
* The kernel can not handle #VE's when accessing normal kernel
* memory. Ensure that no #VE will be delivered for accesses to
* TD-private memory. Only VMM-shared memory (MMIO) will #VE.
+ *
+ * Check if the TD is created with SEPT #VE disabled.
*/
- td_attr = args.rdx;
- if (!(td_attr & ATTR_SEPT_VE_DISABLE)) {
- const char *msg = "TD misconfiguration: SEPT_VE_DISABLE attribute must be set.";
+ sept_ve_disabled = td_attr & ATTR_SEPT_VE_DISABLE;
- /* Relax SEPT_VE_DISABLE check for debug TD. */
+ /*
+ * Check if flexible control of SEPT #VE is supported.
+ *
+ * The check consists of verifying if the feature is supported by the
+ * TDX module (the TDX_FEATURES0 check) and if the feature is enabled
+ * for this TD (CONFIG_FLAGS check).
+ *
+ * If flexible control is supported, disable SEPT #VE.
+ *
+ * Disable SEPT #VE regardless of ATTR_SEPT_VE_DISABLE status as
+ * flexible control allows software running before the kernel to
+ * enable it.
+ *
+ * Skip SEPT disabling for debug TD. SEPT #VE is unsafe but can be
+ * useful for debugging to produce a stack trace. Known to be useful
+ * for debugging unaccepted memory problems.
+ */
+ if (features & TDX_FEATURES0_PENDING_EPT_VIOLATION_V2 &&
+ (tdg_vm_rd(TDCS_CONFIG_FLAGS) & TDCS_CONFIG_FLEXIBLE_PENDING_VE) &&
+ !(td_attr & ATTR_DEBUG)) {
+ if (tdcs_ctls_set(TD_CTLS_PENDING_VE_DISABLE))
+ sept_ve_disabled = true;
+ }
+
+ if (!sept_ve_disabled) {
+ const char *msg = "TD misconfiguration: SEPT #VE has to be disabled";
+
+ /* Relax SEPT #VE disable check for debug TD. */
if (td_attr & ATTR_DEBUG)
pr_warn("%s\n", msg);
else
diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
index fdfd41511b02..29a61c72e4dd 100644
--- a/arch/x86/include/asm/shared/tdx.h
+++ b/arch/x86/include/asm/shared/tdx.h
@@ -16,11 +16,27 @@
#define TDG_VP_VEINFO_GET 3
#define TDG_MR_REPORT 4
#define TDG_MEM_PAGE_ACCEPT 6
+#define TDG_VM_RD 7
#define TDG_VM_WR 8
+#define TDG_SYS_RD 11
-/* TDCS fields. To be used by TDG.VM.WR and TDG.VM.RD module calls */
+/* TDX Global Metadata. To be used by TDG.SYS.RD */
+#define TDCS_TDX_FEATURES0 0x0A00000300000008
+
+/* TDX TD-Scope Metadata. To be used by TDG.VM.WR and TDG.VM.RD */
+#define TDCS_CONFIG_FLAGS 0x1110000300000016
+#define TDCS_TD_CTLS 0x1110000300000017
#define TDCS_NOTIFY_ENABLES 0x9100000000000010
+/* TDCS_TDX_FEATURES0 bits */
+#define TDX_FEATURES0_PENDING_EPT_VIOLATION_V2 BIT_ULL(16)
+
+/* TDCS_CONFIG_FLAGS bits */
+#define TDCS_CONFIG_FLEXIBLE_PENDING_VE BIT_ULL(1)
+
+/* TDCS_TD_CTLS bits */
+#define TD_CTLS_PENDING_VE_DISABLE BIT_ULL(0)
+
/* TDX hypercall Leaf IDs */
#define TDVMCALL_MAP_GPA 0x10001
#define TDVMCALL_GET_QUOTE 0x10002
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCHv2 4/4] x86/tdx: Enable ENUM_TOPOLOGY
2024-03-25 10:46 [PATCHv2 0/4] x86/tdx: Adjust TD settings on boot Kirill A. Shutemov
` (2 preceding siblings ...)
2024-03-25 10:46 ` [PATCHv2 3/4] x86/tdx: Handle PENDING_EPT_VIOLATION_V2 Kirill A. Shutemov
@ 2024-03-25 10:46 ` Kirill A. Shutemov
2024-03-26 10:59 ` Huang, Kai
2024-04-24 22:20 ` Dave Hansen
2024-04-10 14:37 ` [PATCHv2 0/4] x86/tdx: Adjust TD settings on boot Kirill A. Shutemov
4 siblings, 2 replies; 17+ messages in thread
From: Kirill A. Shutemov @ 2024-03-25 10:46 UTC (permalink / raw)
To: tglx, mingo, bp, dave.hansen
Cc: sathyanarayanan.kuppuswamy, hpa, seanjc, elena.reshetova,
rick.p.edgecombe, x86, linux-kernel, Kirill A. Shutemov
TDX 1.0 defines baseline behaviour of TDX guest platform. In TDX 1.0
generates a #VE when accessing topology-related CPUID leafs (0xB and
0x1F) and the X2APIC_APICID MSR. The kernel returns all zeros on CPUID
topology. Any complications will cause problems.
The ENUM_TOPOLOGY feature allows the VMM to provide topology
information to the guest. Enabling the feature eliminates
topology-related #VEs: the TDX module virtualizes accesses to
the CPUID leafs and the MSR.
Enable ENUM_TOPOLOGY if it is available.
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
arch/x86/coco/tdx/tdx.c | 19 +++++++++++++++++++
arch/x86/include/asm/shared/tdx.h | 3 +++
2 files changed, 22 insertions(+)
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 860bfdd5a11d..b2d969432a22 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -289,6 +289,25 @@ static void tdx_setup(u64 *cc_mask)
else
tdx_panic(msg);
}
+
+ /*
+ * TDX 1.0 generates a #VE when accessing topology-related CPUID leafs
+ * (0xB and 0x1F) and the X2APIC_APICID MSR. The kernel returns all
+ * zeros on CPUID #VEs. In practice, this means that the kernel can only
+ * boot with a plain topology. Any complications will cause problems.
+ *
+ * The ENUM_TOPOLOGY feature allows the VMM to provide topology
+ * information to the guest in a safe manner. Enabling the feature
+ * eliminates topology-related #VEs: the TDX module virtualizes
+ * accesses to the CPUID leafs and the MSR.
+ *
+ * Enable ENUM_TOPOLOGY if it is available.
+ */
+ if ((features & TDX_FEATURES0_ENUM_TOPOLOGY) &&
+ tdg_vm_rd(TDCS_TOPOLOGY_ENUM_CONFIGURED)) {
+ if (!tdcs_ctls_set(TD_CTLS_ENUM_TOPOLOGY))
+ pr_warn("Failed to enable ENUM_TOPOLOGY\n");
+ }
}
/*
diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
index 29a61c72e4dd..2964c506b241 100644
--- a/arch/x86/include/asm/shared/tdx.h
+++ b/arch/x86/include/asm/shared/tdx.h
@@ -27,15 +27,18 @@
#define TDCS_CONFIG_FLAGS 0x1110000300000016
#define TDCS_TD_CTLS 0x1110000300000017
#define TDCS_NOTIFY_ENABLES 0x9100000000000010
+#define TDCS_TOPOLOGY_ENUM_CONFIGURED 0x9100000000000019
/* TDCS_TDX_FEATURES0 bits */
#define TDX_FEATURES0_PENDING_EPT_VIOLATION_V2 BIT_ULL(16)
+#define TDX_FEATURES0_ENUM_TOPOLOGY BIT_ULL(20)
/* TDCS_CONFIG_FLAGS bits */
#define TDCS_CONFIG_FLEXIBLE_PENDING_VE BIT_ULL(1)
/* TDCS_TD_CTLS bits */
#define TD_CTLS_PENDING_VE_DISABLE BIT_ULL(0)
+#define TD_CTLS_ENUM_TOPOLOGY BIT_ULL(1)
/* TDX hypercall Leaf IDs */
#define TDVMCALL_MAP_GPA 0x10001
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCHv2 1/4] x86/tdx: Introduce tdg_vm_wr()
2024-03-25 10:46 ` [PATCHv2 1/4] x86/tdx: Introduce tdg_vm_wr() Kirill A. Shutemov
@ 2024-03-26 10:45 ` Huang, Kai
2024-03-26 14:56 ` Kuppuswamy Sathyanarayanan
2024-04-24 17:53 ` Dave Hansen
2 siblings, 0 replies; 17+ messages in thread
From: Huang, Kai @ 2024-03-26 10:45 UTC (permalink / raw)
To: kirill.shutemov, tglx, mingo, Hansen, Dave, bp
Cc: sathyanarayanan.kuppuswamy, hpa, Reshetova, Elena, seanjc,
Edgecombe, Rick P, x86, linux-kernel
On Mon, 2024-03-25 at 12:46 +0200, Kirill A. Shutemov wrote:
> Add a helper function to write to a TD-scope metadata field and use it
> to set NOTIFY_ENABLES.
>
> The helper function will be paired with tdg_vm_rd() and will be used to
> manipulate other metadata fields, not just NOTIFY_ENABLES.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>
Reviewed-by: Kai Huang <kai.huang@intel.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2 2/4] x86/tdx: Rename tdx_parse_tdinfo() to tdx_setup()
2024-03-25 10:46 ` [PATCHv2 2/4] x86/tdx: Rename tdx_parse_tdinfo() to tdx_setup() Kirill A. Shutemov
@ 2024-03-26 10:45 ` Huang, Kai
0 siblings, 0 replies; 17+ messages in thread
From: Huang, Kai @ 2024-03-26 10:45 UTC (permalink / raw)
To: kirill.shutemov, tglx, mingo, Hansen, Dave, bp
Cc: sathyanarayanan.kuppuswamy, hpa, Reshetova, Elena, seanjc,
Edgecombe, Rick P, x86, linux-kernel
On Mon, 2024-03-25 at 12:46 +0200, Kirill A. Shutemov wrote:
> Rename tdx_parse_tdinfo() to tdx_setup() and move setting NOTIFY_ENABLES
> there.
>
> The function will be extended to adjust TD configuration.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
Reviewed-by: Kai Huang <kai.huang@intel.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2 4/4] x86/tdx: Enable ENUM_TOPOLOGY
2024-03-25 10:46 ` [PATCHv2 4/4] x86/tdx: Enable ENUM_TOPOLOGY Kirill A. Shutemov
@ 2024-03-26 10:59 ` Huang, Kai
2024-03-26 12:21 ` kirill.shutemov
2024-04-24 22:20 ` Dave Hansen
1 sibling, 1 reply; 17+ messages in thread
From: Huang, Kai @ 2024-03-26 10:59 UTC (permalink / raw)
To: kirill.shutemov, tglx, mingo, Hansen, Dave, bp
Cc: sathyanarayanan.kuppuswamy, hpa, Reshetova, Elena, seanjc,
Edgecombe, Rick P, x86, linux-kernel
On Mon, 2024-03-25 at 12:46 +0200, Kirill A. Shutemov wrote:
> TDX 1.0 defines baseline behaviour of TDX guest platform. In TDX 1.0
^ Not needed.
> generates a #VE when accessing topology-related CPUID leafs (0xB and
> 0x1F) and the X2APIC_APICID MSR. The kernel returns all zeros on CPUID
> topology. Any complications will cause problems.
>
> The ENUM_TOPOLOGY feature allows the VMM to provide topology
> information to the guest. Enabling the feature eliminates
> topology-related #VEs: the TDX module virtualizes accesses to
> the CPUID leafs and the MSR.
>
> Enable ENUM_TOPOLOGY if it is available.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
> arch/x86/coco/tdx/tdx.c | 19 +++++++++++++++++++
> arch/x86/include/asm/shared/tdx.h | 3 +++
> 2 files changed, 22 insertions(+)
>
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 860bfdd5a11d..b2d969432a22 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -289,6 +289,25 @@ static void tdx_setup(u64 *cc_mask)
> else
> tdx_panic(msg);
> }
> +
> + /*
> + * TDX 1.0 generates a #VE when accessing topology-related CPUID leafs
> + * (0xB and 0x1F) and the X2APIC_APICID MSR. The kernel returns all
> + * zeros on CPUID #VEs. In practice, this means that the kernel can only
> + * boot with a plain topology. Any complications will cause problems.
> + *
> + * The ENUM_TOPOLOGY feature allows the VMM to provide topology
> + * information to the guest in a safe manner. Enabling the feature
> + * eliminates topology-related #VEs: the TDX module virtualizes
> + * accesses to the CPUID leafs and the MSR.
> + *
> + * Enable ENUM_TOPOLOGY if it is available.
> + */
> + if ((features & TDX_FEATURES0_ENUM_TOPOLOGY) &&
> + tdg_vm_rd(TDCS_TOPOLOGY_ENUM_CONFIGURED)) {
> + if (!tdcs_ctls_set(TD_CTLS_ENUM_TOPOLOGY))
> + pr_warn("Failed to enable ENUM_TOPOLOGY\n");
> + }
> }
>
> /*
> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
> index 29a61c72e4dd..2964c506b241 100644
> --- a/arch/x86/include/asm/shared/tdx.h
> +++ b/arch/x86/include/asm/shared/tdx.h
> @@ -27,15 +27,18 @@
> #define TDCS_CONFIG_FLAGS 0x1110000300000016
> #define TDCS_TD_CTLS 0x1110000300000017
> #define TDCS_NOTIFY_ENABLES 0x9100000000000010
> +#define TDCS_TOPOLOGY_ENUM_CONFIGURED 0x9100000000000019
Do you know where can I find the metadata field ID definition?
It seems I couldn't find all metadata field ID definitions in the latest TDX 1.5
ABI spec anymore.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2 4/4] x86/tdx: Enable ENUM_TOPOLOGY
2024-03-26 10:59 ` Huang, Kai
@ 2024-03-26 12:21 ` kirill.shutemov
0 siblings, 0 replies; 17+ messages in thread
From: kirill.shutemov @ 2024-03-26 12:21 UTC (permalink / raw)
To: Huang, Kai
Cc: tglx, mingo, Hansen, Dave, bp, sathyanarayanan.kuppuswamy, hpa,
Reshetova, Elena, seanjc, Edgecombe, Rick P, x86, linux-kernel
On Tue, Mar 26, 2024 at 10:59:38AM +0000, Huang, Kai wrote:
> On Mon, 2024-03-25 at 12:46 +0200, Kirill A. Shutemov wrote:
> > TDX 1.0 defines baseline behaviour of TDX guest platform. In TDX 1.0
>
Okay.
> > generates a #VE when accessing topology-related CPUID leafs (0xB and
> > 0x1F) and the X2APIC_APICID MSR. The kernel returns all zeros on CPUID
> > topology. Any complications will cause problems.
> >
> > The ENUM_TOPOLOGY feature allows the VMM to provide topology
> > information to the guest. Enabling the feature eliminates
> > topology-related #VEs: the TDX module virtualizes accesses to
> > the CPUID leafs and the MSR.
> >
> > Enable ENUM_TOPOLOGY if it is available.
> >
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> > arch/x86/coco/tdx/tdx.c | 19 +++++++++++++++++++
> > arch/x86/include/asm/shared/tdx.h | 3 +++
> > 2 files changed, 22 insertions(+)
> >
> > diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> > index 860bfdd5a11d..b2d969432a22 100644
> > --- a/arch/x86/coco/tdx/tdx.c
> > +++ b/arch/x86/coco/tdx/tdx.c
> > @@ -289,6 +289,25 @@ static void tdx_setup(u64 *cc_mask)
> > else
> > tdx_panic(msg);
> > }
> > +
> > + /*
> > + * TDX 1.0 generates a #VE when accessing topology-related CPUID leafs
> > + * (0xB and 0x1F) and the X2APIC_APICID MSR. The kernel returns all
> > + * zeros on CPUID #VEs. In practice, this means that the kernel can only
> > + * boot with a plain topology. Any complications will cause problems.
> > + *
> > + * The ENUM_TOPOLOGY feature allows the VMM to provide topology
> > + * information to the guest in a safe manner. Enabling the feature
> > + * eliminates topology-related #VEs: the TDX module virtualizes
> > + * accesses to the CPUID leafs and the MSR.
> > + *
> > + * Enable ENUM_TOPOLOGY if it is available.
> > + */
> > + if ((features & TDX_FEATURES0_ENUM_TOPOLOGY) &&
> > + tdg_vm_rd(TDCS_TOPOLOGY_ENUM_CONFIGURED)) {
> > + if (!tdcs_ctls_set(TD_CTLS_ENUM_TOPOLOGY))
> > + pr_warn("Failed to enable ENUM_TOPOLOGY\n");
> > + }
> > }
> >
> > /*
> > diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
> > index 29a61c72e4dd..2964c506b241 100644
> > --- a/arch/x86/include/asm/shared/tdx.h
> > +++ b/arch/x86/include/asm/shared/tdx.h
> > @@ -27,15 +27,18 @@
> > #define TDCS_CONFIG_FLAGS 0x1110000300000016
> > #define TDCS_TD_CTLS 0x1110000300000017
> > #define TDCS_NOTIFY_ENABLES 0x9100000000000010
> > +#define TDCS_TOPOLOGY_ENUM_CONFIGURED 0x9100000000000019
>
> Do you know where can I find the metadata field ID definition?
>
> It seems I couldn't find all metadata field ID definitions in the latest TDX 1.5
> ABI spec anymore.
See "Intel TDX Module v1.5 ABI Definitions":
https://cdrdv2.intel.com/v1/dl/getContent/795381
It has all fields described in JSON.
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2 1/4] x86/tdx: Introduce tdg_vm_wr()
2024-03-25 10:46 ` [PATCHv2 1/4] x86/tdx: Introduce tdg_vm_wr() Kirill A. Shutemov
2024-03-26 10:45 ` Huang, Kai
@ 2024-03-26 14:56 ` Kuppuswamy Sathyanarayanan
2024-04-24 17:53 ` Dave Hansen
2 siblings, 0 replies; 17+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2024-03-26 14:56 UTC (permalink / raw)
To: Kirill A. Shutemov, tglx, mingo, bp, dave.hansen
Cc: hpa, seanjc, elena.reshetova, rick.p.edgecombe, x86, linux-kernel
On 3/25/24 3:46 AM, Kirill A. Shutemov wrote:
> Add a helper function to write to a TD-scope metadata field and use it
> to set NOTIFY_ENABLES.
>
> The helper function will be paired with tdg_vm_rd() and will be used to
> manipulate other metadata fields, not just NOTIFY_ENABLES.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
Looks good to me.
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> arch/x86/coco/tdx/tdx.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 59776ce1c1d7..4fb36e5c4e80 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -77,6 +77,20 @@ static inline void tdcall(u64 fn, struct tdx_module_args *args)
> panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
> }
>
> +static inline u64 tdg_vm_wr(u64 field, u64 value, u64 mask)
> +{
> + struct tdx_module_args args = {
> + .rdx = field,
> + .r8 = value,
> + .r9 = mask,
> + };
> +
> + tdcall(TDG_VM_WR, &args);
> +
> + /* Old value */
> + return args.r8;
> +}
> +
> /**
> * tdx_mcall_get_report0() - Wrapper to get TDREPORT0 (a.k.a. TDREPORT
> * subtype 0) using TDG.MR.REPORT TDCALL.
> @@ -902,10 +916,6 @@ static void tdx_kexec_unshare_mem(void)
>
> void __init tdx_early_init(void)
> {
> - struct tdx_module_args args = {
> - .rdx = TDCS_NOTIFY_ENABLES,
> - .r9 = -1ULL,
> - };
> u64 cc_mask;
> u32 eax, sig[3];
>
> @@ -924,7 +934,7 @@ void __init tdx_early_init(void)
> cc_set_mask(cc_mask);
>
> /* Kernel does not use NOTIFY_ENABLES and does not need random #VEs */
> - tdcall(TDG_VM_WR, &args);
> + tdg_vm_wr(TDCS_NOTIFY_ENABLES, 0, -1ULL);
>
> /*
> * All bits above GPA width are reserved and kernel treats shared bit
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2 0/4] x86/tdx: Adjust TD settings on boot
2024-03-25 10:46 [PATCHv2 0/4] x86/tdx: Adjust TD settings on boot Kirill A. Shutemov
` (3 preceding siblings ...)
2024-03-25 10:46 ` [PATCHv2 4/4] x86/tdx: Enable ENUM_TOPOLOGY Kirill A. Shutemov
@ 2024-04-10 14:37 ` Kirill A. Shutemov
2024-04-10 14:41 ` Edgecombe, Rick P
4 siblings, 1 reply; 17+ messages in thread
From: Kirill A. Shutemov @ 2024-04-10 14:37 UTC (permalink / raw)
To: tglx, mingo, bp, dave.hansen
Cc: sathyanarayanan.kuppuswamy, hpa, seanjc, elena.reshetova,
rick.p.edgecombe, x86, linux-kernel
On Mon, Mar 25, 2024 at 12:46:03PM +0200, Kirill A. Shutemov wrote:
> Adjust TD setting on boot:
>
> - Disable EPT violation #VE on private memory if TD can
> control it;
>
> - Enable virtualization of topology-related CPUID leafs
> X2APIC_APICID MSR;
Any feedback?
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2 0/4] x86/tdx: Adjust TD settings on boot
2024-04-10 14:37 ` [PATCHv2 0/4] x86/tdx: Adjust TD settings on boot Kirill A. Shutemov
@ 2024-04-10 14:41 ` Edgecombe, Rick P
2024-04-12 15:01 ` kirill.shutemov
0 siblings, 1 reply; 17+ messages in thread
From: Edgecombe, Rick P @ 2024-04-10 14:41 UTC (permalink / raw)
To: kirill.shutemov, tglx, mingo, Hansen, Dave, bp
Cc: sathyanarayanan.kuppuswamy, hpa, Reshetova, Elena, seanjc,
linux-kernel, x86
On Wed, 2024-04-10 at 17:37 +0300, Kirill A. Shutemov wrote:
> On Mon, Mar 25, 2024 at 12:46:03PM +0200, Kirill A. Shutemov wrote:
> > Adjust TD setting on boot:
> >
> > - Disable EPT violation #VE on private memory if TD can
> > control it;
> >
> > - Enable virtualization of topology-related CPUID leafs
> > X2APIC_APICID MSR;
>
> Any feedback?
It is missing a lot of the normal things that come in coverletters like what is
the problem and importance. It might help attract more review.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2 0/4] x86/tdx: Adjust TD settings on boot
2024-04-10 14:41 ` Edgecombe, Rick P
@ 2024-04-12 15:01 ` kirill.shutemov
0 siblings, 0 replies; 17+ messages in thread
From: kirill.shutemov @ 2024-04-12 15:01 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: tglx, mingo, Hansen, Dave, bp, sathyanarayanan.kuppuswamy, hpa,
Reshetova, Elena, seanjc, linux-kernel, x86
On Wed, Apr 10, 2024 at 02:41:13PM +0000, Edgecombe, Rick P wrote:
> On Wed, 2024-04-10 at 17:37 +0300, Kirill A. Shutemov wrote:
> > On Mon, Mar 25, 2024 at 12:46:03PM +0200, Kirill A. Shutemov wrote:
> > > Adjust TD setting on boot:
> > >
> > > - Disable EPT violation #VE on private memory if TD can
> > > control it;
> > >
> > > - Enable virtualization of topology-related CPUID leafs
> > > X2APIC_APICID MSR;
> >
> > Any feedback?
>
> It is missing a lot of the normal things that come in coverletters like what is
> the problem and importance. It might help attract more review.
What about this:
The patchset adjusts a few TD settings on boot for the optimal functioning
of the system:
- Disable EPT violation #VE on private memory if TD can control it
The newer TDX module allows the guest to control whether it wants to
see #VE on EPT violation on private memory. The Linux kernel does not
want such #VEs and needs to disable them.
- Enable virtualization of topology-related CPUID leafs X2APIC_APICID MSR;
The ENUM_TOPOLOGY feature allows the VMM to provide topology
information to the guest. Enabling the feature eliminates
topology-related #VEs: the TDX module virtualizes accesses to the
CPUID leafs and the MSR.
It allows TDX guest to run with non-trivial topology configuration.
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2 1/4] x86/tdx: Introduce tdg_vm_wr()
2024-03-25 10:46 ` [PATCHv2 1/4] x86/tdx: Introduce tdg_vm_wr() Kirill A. Shutemov
2024-03-26 10:45 ` Huang, Kai
2024-03-26 14:56 ` Kuppuswamy Sathyanarayanan
@ 2024-04-24 17:53 ` Dave Hansen
2 siblings, 0 replies; 17+ messages in thread
From: Dave Hansen @ 2024-04-24 17:53 UTC (permalink / raw)
To: Kirill A. Shutemov, tglx, mingo, bp
Cc: sathyanarayanan.kuppuswamy, hpa, seanjc, elena.reshetova,
rick.p.edgecombe, x86, linux-kernel
So, the text here should help me understand what is going on. What it
tries to tell me should not be a literal rephrasing of the contents of
the diff.
This patch literally introduces a function called tdg_vm_wr(). The
subject adds precisely zero to what I can get from reading the diff.
How about:
x86/tdx: Factor out TD metadata write tdcall
On 3/25/24 03:46, Kirill A. Shutemov wrote:
> Add a helper function to write to a TD-scope metadata field and use it
> to set NOTIFY_ENABLES.
>
> The helper function will be paired with tdg_vm_rd() and will be used to
> manipulate other metadata fields, not just NOTIFY_ENABLES.
Ideally this would at least tell me what problem this is solving:
The TDG_VM_WR seamcall is used to ask the TDX module to change
some TD-specific VM configuration. There is currently only one
user in the kernel of this seamcall. More will be added
shortly.
... and then the solution:
Refactor to make way for more users of TDG_VM_WR who will need
to modify other TD configuration values.
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 59776ce1c1d7..4fb36e5c4e80 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -77,6 +77,20 @@ static inline void tdcall(u64 fn, struct tdx_module_args *args)
> panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
> }
>
> +static inline u64 tdg_vm_wr(u64 field, u64 value, u64 mask)
> +{
> + struct tdx_module_args args = {
> + .rdx = field,
> + .r8 = value,
> + .r9 = mask,
> + };
> +
> + tdcall(TDG_VM_WR, &args);
> +
> + /* Old value */
> + return args.r8;
> +}
> +
> /**
> * tdx_mcall_get_report0() - Wrapper to get TDREPORT0 (a.k.a. TDREPORT
> * subtype 0) using TDG.MR.REPORT TDCALL.
> @@ -902,10 +916,6 @@ static void tdx_kexec_unshare_mem(void)
>
> void __init tdx_early_init(void)
> {
> - struct tdx_module_args args = {
> - .rdx = TDCS_NOTIFY_ENABLES,
> - .r9 = -1ULL,
> - };
> u64 cc_mask;
> u32 eax, sig[3];
>
> @@ -924,7 +934,7 @@ void __init tdx_early_init(void)
> cc_set_mask(cc_mask);
>
> /* Kernel does not use NOTIFY_ENABLES and does not need random #VEs */
> - tdcall(TDG_VM_WR, &args);
> + tdg_vm_wr(TDCS_NOTIFY_ENABLES, 0, -1ULL);
>
> /*
> * All bits above GPA width are reserved and kernel treats shared bit
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2 3/4] x86/tdx: Handle PENDING_EPT_VIOLATION_V2
2024-03-25 10:46 ` [PATCHv2 3/4] x86/tdx: Handle PENDING_EPT_VIOLATION_V2 Kirill A. Shutemov
@ 2024-04-24 18:29 ` Dave Hansen
0 siblings, 0 replies; 17+ messages in thread
From: Dave Hansen @ 2024-04-24 18:29 UTC (permalink / raw)
To: Kirill A. Shutemov, tglx, mingo, bp
Cc: sathyanarayanan.kuppuswamy, hpa, seanjc, elena.reshetova,
rick.p.edgecombe, x86, linux-kernel
On 3/25/24 03:46, Kirill A. Shutemov wrote:
> PENDING_EPT_VIOLATION_V2 allows TD to control whether access to
> a pending page triggers #VE.
>
> Kernel doesn't want to see any #VEs on accesses to private memory:
> disable such #VEs.
This is really a bare bones changelog. The connection between "pending"
and "private" goes unmentioned. It's not obvious when the kernel might
be exposed to one of these #VE's.
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 08e2bb462ce8..860bfdd5a11d 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -77,6 +77,17 @@ static inline void tdcall(u64 fn, struct tdx_module_args *args)
> panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
> }
>
> +static inline u64 tdg_vm_rd(u64 field)
> +{
> + struct tdx_module_args args = {
> + .rdx = field,
> + };
> +
> + tdcall(TDG_VM_RD, &args);
> +
> + return args.r8;
> +}
> +
> static inline u64 tdg_vm_wr(u64 field, u64 value, u64 mask)
> {
> struct tdx_module_args args = {
> @@ -91,6 +102,28 @@ static inline u64 tdg_vm_wr(u64 field, u64 value, u64 mask)
> return args.r8;
> }
>
> +static inline u64 tdg_sys_rd(u64 field)
> +{
> + struct tdx_module_args args = {
> + .rdx = field,
> + };
> +
> + tdcall(TDG_SYS_RD, &args);
> +
> + return args.r8;
> +}
I scratched my head for a minute on why these two identical functions
were introduced. Then I realized that they're different TDCALLs.
Please break them out into a separate patch where you can mention this.
Also, the least we can do is a one-line comment about what they do:
/* Read metadata about how *this* VM is configured. */
static inline u64 tdg_vm_rd(u64 field)
...
/* Read metadata about the TDX system as whole. */
static inline u64 tdg_sys_rd(u64 field)
> +static bool tdcs_ctls_set(u64 mask)
> +{
> + struct tdx_module_args args = {
> + .rdx = TDCS_TD_CTLS,
> + .r8 = mask,
> + .r9 = mask,
> + };
> +
> + return __tdcall(TDG_VM_WR, &args) == TDX_SUCCESS;
> +}
This is acronym soup. It's made worse by "TDCS_TD_CTLS" being
ungoogleable and not literally present in the first three TDX PDFs that
I dug in.
It's also not clear logically how this differs from tdg_vm_wr().
> /**
> * tdx_mcall_get_report0() - Wrapper to get TDREPORT0 (a.k.a. TDREPORT
> * subtype 0) using TDG.MR.REPORT TDCALL.
> @@ -185,7 +218,8 @@ static void tdx_setup(u64 *cc_mask)
> {
> struct tdx_module_args args = {};
> unsigned int gpa_width;
> - u64 td_attr;
> + u64 td_attr, features;
> + bool sept_ve_disabled;
>
> /*
> * TDINFO TDX module call is used to get the TD execution environment
> @@ -206,19 +240,50 @@ static void tdx_setup(u64 *cc_mask)
> gpa_width = args.rcx & GENMASK(5, 0);
> *cc_mask = BIT_ULL(gpa_width - 1);
>
> + td_attr = args.rdx;
> +
> /* Kernel does not use NOTIFY_ENABLES and does not need random #VEs */
> tdg_vm_wr(TDCS_NOTIFY_ENABLES, 0, -1ULL);
>
> + features = tdg_sys_rd(TDCS_TDX_FEATURES0);
> +
> /*
> * The kernel can not handle #VE's when accessing normal kernel
> * memory. Ensure that no #VE will be delivered for accesses to
> * TD-private memory. Only VMM-shared memory (MMIO) will #VE.
> + *
> + * Check if the TD is created with SEPT #VE disabled.
> */
> - td_attr = args.rdx;
> - if (!(td_attr & ATTR_SEPT_VE_DISABLE)) {
> - const char *msg = "TD misconfiguration: SEPT_VE_DISABLE attribute must be set.";
> + sept_ve_disabled = td_attr & ATTR_SEPT_VE_DISABLE;
>
> - /* Relax SEPT_VE_DISABLE check for debug TD. */
> + /*
> + * Check if flexible control of SEPT #VE is supported.
> + *
> + * The check consists of verifying if the feature is supported by the
> + * TDX module (the TDX_FEATURES0 check) and if the feature is enabled
> + * for this TD (CONFIG_FLAGS check).
> + *
> + * If flexible control is supported, disable SEPT #VE.
> + *
> + * Disable SEPT #VE regardless of ATTR_SEPT_VE_DISABLE status as
> + * flexible control allows software running before the kernel to
> + * enable it.
> + *
> + * Skip SEPT disabling for debug TD. SEPT #VE is unsafe but can be
> + * useful for debugging to produce a stack trace. Known to be useful
> + * for debugging unaccepted memory problems.
> + */
> + if (features & TDX_FEATURES0_PENDING_EPT_VIOLATION_V2 &&
> + (tdg_vm_rd(TDCS_CONFIG_FLAGS) & TDCS_CONFIG_FLEXIBLE_PENDING_VE) &&
Can't we just check TDCS_CONFIG_FLEXIBLE_PENDING_VE alone? Surely that
flag can't get set if !TDX_FEATURES0_PENDING_EPT_VIOLATION_V2.
In any case, this is, um, kinda messy. Look at what a helper could do:
/*
* Private memory #VE's are bad because the kernel has no way of
* recovering from them and can only die. Newer TDX modules can
* configured to inflict fewer #VE's on guests. Do it when available.
*/
static inline int try_disable_sept_ve(td_addr)
{
/* Does the TDX module support the #VE reduction feature? */
if (!(features & TDX_FEATURES0_PENDING_EPT_VIOLATION_V2))
return -ENOTSUP;
/* Is this TD allowed to use the feature? */
if (!(tdg_vm_rd(TDCS_CONFIG_FLAGS) &
TDCS_CONFIG_FLEXIBLE_PENDING_VE)))
return -ENOTSUP;
/* Keep #VE's enabled for splats in debugging environments: */
if (td_attr & ATTR_DEBUG)
return -ENOTSUP;
if (!tdcs_ctls_set(TD_CTLS_PENDING_VE_DISABLE))
return -ENOTSUP;
return 0;
}
Then the code becomes:
if (!try_disable_sept_ve(td_addr))
sept_ve_disabled = true;
else
sept_ve_disabled = td_attr & ATTR_SEPT_VE_DISABLE;
See how it sets the state in *ONE* easy-to-understand place?
Oh, 'sept_ve_disabled' either gets disabled dynamically if it can, or it
gets read from the static state if not.
Oh, and if you ever have a comment that long with a bunch of
explanations of individual if() conditions, *PLEASE* try to break it up
in some way. Otherwise readers just spent all their time trying to
match up the comment to the code. It's not a fun game.
> + !(td_attr & ATTR_DEBUG)) {
> + if (tdcs_ctls_set(TD_CTLS_PENDING_VE_DISABLE))
> + sept_ve_disabled = true;
> + }
> +
> + if (!sept_ve_disabled) {
> + const char *msg = "TD misconfiguration: SEPT #VE has to be disabled";
> +
> + /* Relax SEPT #VE disable check for debug TD. */
> if (td_attr & ATTR_DEBUG)
> pr_warn("%s\n", msg);
> else
> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
> index fdfd41511b02..29a61c72e4dd 100644
> --- a/arch/x86/include/asm/shared/tdx.h
> +++ b/arch/x86/include/asm/shared/tdx.h
> @@ -16,11 +16,27 @@
> #define TDG_VP_VEINFO_GET 3
> #define TDG_MR_REPORT 4
> #define TDG_MEM_PAGE_ACCEPT 6
> +#define TDG_VM_RD 7
> #define TDG_VM_WR 8
> +#define TDG_SYS_RD 11
>
> -/* TDCS fields. To be used by TDG.VM.WR and TDG.VM.RD module calls */
> +/* TDX Global Metadata. To be used by TDG.SYS.RD */
> +#define TDCS_TDX_FEATURES0 0x0A00000300000008
> +
> +/* TDX TD-Scope Metadata. To be used by TDG.VM.WR and TDG.VM.RD */
> +#define TDCS_CONFIG_FLAGS 0x1110000300000016
> +#define TDCS_TD_CTLS 0x1110000300000017
> #define TDCS_NOTIFY_ENABLES 0x9100000000000010
>
> +/* TDCS_TDX_FEATURES0 bits */
> +#define TDX_FEATURES0_PENDING_EPT_VIOLATION_V2 BIT_ULL(16)
> +
> +/* TDCS_CONFIG_FLAGS bits */
> +#define TDCS_CONFIG_FLEXIBLE_PENDING_VE BIT_ULL(1)
> +
> +/* TDCS_TD_CTLS bits */
> +#define TD_CTLS_PENDING_VE_DISABLE BIT_ULL(0)
> +
> /* TDX hypercall Leaf IDs */
> #define TDVMCALL_MAP_GPA 0x10001
> #define TDVMCALL_GET_QUOTE 0x10002
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2 4/4] x86/tdx: Enable ENUM_TOPOLOGY
2024-03-25 10:46 ` [PATCHv2 4/4] x86/tdx: Enable ENUM_TOPOLOGY Kirill A. Shutemov
2024-03-26 10:59 ` Huang, Kai
@ 2024-04-24 22:20 ` Dave Hansen
2024-04-27 16:53 ` Kirill A. Shutemov
1 sibling, 1 reply; 17+ messages in thread
From: Dave Hansen @ 2024-04-24 22:20 UTC (permalink / raw)
To: Kirill A. Shutemov, tglx, mingo, bp
Cc: sathyanarayanan.kuppuswamy, hpa, seanjc, elena.reshetova,
rick.p.edgecombe, x86, linux-kernel
On 3/25/24 03:46, Kirill A. Shutemov wrote:
> + if ((features & TDX_FEATURES0_ENUM_TOPOLOGY) &&
> + tdg_vm_rd(TDCS_TOPOLOGY_ENUM_CONFIGURED)) {
> + if (!tdcs_ctls_set(TD_CTLS_ENUM_TOPOLOGY))
> + pr_warn("Failed to enable ENUM_TOPOLOGY\n");
> + }
This is looking pretty familiar at this point.
I'd almost rather just have it do a plain old:
tdcs_ctls_set(TD_CTLS_ENUM_TOPOLOGY);
... and leave it at that.
*If* there is a generic return code for "invalid" as opposed to a
not-supported tdcs_ctls_set(), then it's arguable that you can spit out
a message for an unexpected "invalid" error.
But seriously, why all the complexity over and over? What does this buy?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2 4/4] x86/tdx: Enable ENUM_TOPOLOGY
2024-04-24 22:20 ` Dave Hansen
@ 2024-04-27 16:53 ` Kirill A. Shutemov
0 siblings, 0 replies; 17+ messages in thread
From: Kirill A. Shutemov @ 2024-04-27 16:53 UTC (permalink / raw)
To: Dave Hansen
Cc: tglx, mingo, bp, sathyanarayanan.kuppuswamy, hpa, seanjc,
elena.reshetova, rick.p.edgecombe, x86, linux-kernel
On Wed, Apr 24, 2024 at 03:20:54PM -0700, Dave Hansen wrote:
> But seriously, why all the complexity over and over? What does this buy?
Thanks. That's a good perspective. I will rework the patchset dropping all
unneeded enumeration.
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2024-04-27 16:53 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-25 10:46 [PATCHv2 0/4] x86/tdx: Adjust TD settings on boot Kirill A. Shutemov
2024-03-25 10:46 ` [PATCHv2 1/4] x86/tdx: Introduce tdg_vm_wr() Kirill A. Shutemov
2024-03-26 10:45 ` Huang, Kai
2024-03-26 14:56 ` Kuppuswamy Sathyanarayanan
2024-04-24 17:53 ` Dave Hansen
2024-03-25 10:46 ` [PATCHv2 2/4] x86/tdx: Rename tdx_parse_tdinfo() to tdx_setup() Kirill A. Shutemov
2024-03-26 10:45 ` Huang, Kai
2024-03-25 10:46 ` [PATCHv2 3/4] x86/tdx: Handle PENDING_EPT_VIOLATION_V2 Kirill A. Shutemov
2024-04-24 18:29 ` Dave Hansen
2024-03-25 10:46 ` [PATCHv2 4/4] x86/tdx: Enable ENUM_TOPOLOGY Kirill A. Shutemov
2024-03-26 10:59 ` Huang, Kai
2024-03-26 12:21 ` kirill.shutemov
2024-04-24 22:20 ` Dave Hansen
2024-04-27 16:53 ` Kirill A. Shutemov
2024-04-10 14:37 ` [PATCHv2 0/4] x86/tdx: Adjust TD settings on boot Kirill A. Shutemov
2024-04-10 14:41 ` Edgecombe, Rick P
2024-04-12 15:01 ` kirill.shutemov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.