From: Isaku Yamahata <[email protected]>
A VMM interacts with the TDX module using a new instruction (SEAMCALL).
For instance, a TDX VMM does not have full access to the VM control
structure corresponding to VMX VMCS. Instead, a VMM induces the TDX module
to act on behalf via SEAMCALLs.
Export __seamcall and define C wrapper functions for SEAMCALLs for
readability.
Some SEAMCALL APIs donate host pages to TDX module or guest TD, and the
donated pages are encrypted. Such SEAMCALLs flush cache lines
(typically by movdir64b instruction), but some don't. Those that don't
clear cache lines require the VMM to flush the cache lines to avoid cache
line alias.
Signed-off-by: Sean Christopherson <[email protected]>
Signed-off-by: Isaku Yamahata <[email protected]>
---
arch/x86/include/asm/tdx.h | 4 +
arch/x86/kvm/vmx/tdx_ops.h | 202 +++++++++++++++++++++++++++++++
arch/x86/virt/vmx/tdx/seamcall.S | 2 +
arch/x86/virt/vmx/tdx/tdx.h | 3 -
4 files changed, 208 insertions(+), 3 deletions(-)
create mode 100644 arch/x86/kvm/vmx/tdx_ops.h
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 112a5b9bd5cd..6c01ab572c1f 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -104,10 +104,14 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
bool platform_tdx_enabled(void);
int tdx_cpu_enable(void);
int tdx_enable(void);
+u64 __seamcall(u64 op, u64 rcx, u64 rdx, u64 r8, u64 r9,
+ struct tdx_module_output *out);
#else /* !CONFIG_INTEL_TDX_HOST */
static inline bool platform_tdx_enabled(void) { return false; }
static inline int tdx_cpu_enable(void) { return -EINVAL; }
static inline int tdx_enable(void) { return -EINVAL; }
+static inline u64 __seamcall(u64 op, u64 rcx, u64 rdx, u64 r8, u64 r9,
+ struct tdx_module_output *out) { return TDX_SEAMCALL_UD; };
#endif /* CONFIG_INTEL_TDX_HOST */
#endif /* !__ASSEMBLY__ */
diff --git a/arch/x86/kvm/vmx/tdx_ops.h b/arch/x86/kvm/vmx/tdx_ops.h
new file mode 100644
index 000000000000..893cc6c25f3b
--- /dev/null
+++ b/arch/x86/kvm/vmx/tdx_ops.h
@@ -0,0 +1,202 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* constants/data definitions for TDX SEAMCALLs */
+
+#ifndef __KVM_X86_TDX_OPS_H
+#define __KVM_X86_TDX_OPS_H
+
+#include <linux/compiler.h>
+
+#include <asm/cacheflush.h>
+#include <asm/asm.h>
+#include <asm/kvm_host.h>
+
+#include "tdx_errno.h"
+#include "tdx_arch.h"
+#include "x86.h"
+
+static inline u64 kvm_seamcall(u64 op, u64 rcx, u64 rdx, u64 r8, u64 r9,
+ struct tdx_module_output *out)
+{
+ u64 ret;
+
+ ret = __seamcall(op, rcx, rdx, r8, r9, out);
+ if (unlikely(ret == TDX_SEAMCALL_UD)) {
+ /*
+ * TDX requires VMXON or #UD. In the case of reboot or kexec,
+ * VMX is made off (VMXOFF) by kvm reboot notifier,
+ * kvm_reboot(), while TDs are still running. The callers check
+ * the returned error and complain. Suppress it by returning 0.
+ */
+ kvm_spurious_fault();
+ return 0;
+ }
+ return ret;
+}
+
+static inline u64 tdh_mng_addcx(hpa_t tdr, hpa_t addr)
+{
+ clflush_cache_range(__va(addr), PAGE_SIZE);
+ return kvm_seamcall(TDH_MNG_ADDCX, addr, tdr, 0, 0, NULL);
+}
+
+static inline u64 tdh_mem_page_add(hpa_t tdr, gpa_t gpa, hpa_t hpa, hpa_t source,
+ struct tdx_module_output *out)
+{
+ clflush_cache_range(__va(hpa), PAGE_SIZE);
+ return kvm_seamcall(TDH_MEM_PAGE_ADD, gpa, tdr, hpa, source, out);
+}
+
+static inline u64 tdh_mem_sept_add(hpa_t tdr, gpa_t gpa, int level, hpa_t page,
+ struct tdx_module_output *out)
+{
+ clflush_cache_range(__va(page), PAGE_SIZE);
+ return kvm_seamcall(TDH_MEM_SEPT_ADD, gpa | level, tdr, page, 0, out);
+}
+
+static inline u64 tdh_mem_sept_remove(hpa_t tdr, gpa_t gpa, int level,
+ struct tdx_module_output *out)
+{
+ return kvm_seamcall(TDH_MEM_SEPT_REMOVE, gpa | level, tdr, 0, 0, out);
+}
+
+static inline u64 tdh_vp_addcx(hpa_t tdvpr, hpa_t addr)
+{
+ clflush_cache_range(__va(addr), PAGE_SIZE);
+ return kvm_seamcall(TDH_VP_ADDCX, addr, tdvpr, 0, 0, NULL);
+}
+
+static inline u64 tdh_mem_page_relocate(hpa_t tdr, gpa_t gpa, hpa_t hpa,
+ struct tdx_module_output *out)
+{
+ clflush_cache_range(__va(hpa), PAGE_SIZE);
+ return kvm_seamcall(TDH_MEM_PAGE_RELOCATE, gpa, tdr, hpa, 0, out);
+}
+
+static inline u64 tdh_mem_page_aug(hpa_t tdr, gpa_t gpa, hpa_t hpa,
+ struct tdx_module_output *out)
+{
+ clflush_cache_range(__va(hpa), PAGE_SIZE);
+ return kvm_seamcall(TDH_MEM_PAGE_AUG, gpa, tdr, hpa, 0, out);
+}
+
+static inline u64 tdh_mem_range_block(hpa_t tdr, gpa_t gpa, int level,
+ struct tdx_module_output *out)
+{
+ return kvm_seamcall(TDH_MEM_RANGE_BLOCK, gpa | level, tdr, 0, 0, out);
+}
+
+static inline u64 tdh_mng_key_config(hpa_t tdr)
+{
+ return kvm_seamcall(TDH_MNG_KEY_CONFIG, tdr, 0, 0, 0, NULL);
+}
+
+static inline u64 tdh_mng_create(hpa_t tdr, int hkid)
+{
+ clflush_cache_range(__va(tdr), PAGE_SIZE);
+ return kvm_seamcall(TDH_MNG_CREATE, tdr, hkid, 0, 0, NULL);
+}
+
+static inline u64 tdh_vp_create(hpa_t tdr, hpa_t tdvpr)
+{
+ clflush_cache_range(__va(tdvpr), PAGE_SIZE);
+ return kvm_seamcall(TDH_VP_CREATE, tdvpr, tdr, 0, 0, NULL);
+}
+
+static inline u64 tdh_mng_rd(hpa_t tdr, u64 field, struct tdx_module_output *out)
+{
+ return kvm_seamcall(TDH_MNG_RD, tdr, field, 0, 0, out);
+}
+
+static inline u64 tdh_mr_extend(hpa_t tdr, gpa_t gpa,
+ struct tdx_module_output *out)
+{
+ return kvm_seamcall(TDH_MR_EXTEND, gpa, tdr, 0, 0, out);
+}
+
+static inline u64 tdh_mr_finalize(hpa_t tdr)
+{
+ return kvm_seamcall(TDH_MR_FINALIZE, tdr, 0, 0, 0, NULL);
+}
+
+static inline u64 tdh_vp_flush(hpa_t tdvpr)
+{
+ return kvm_seamcall(TDH_VP_FLUSH, tdvpr, 0, 0, 0, NULL);
+}
+
+static inline u64 tdh_mng_vpflushdone(hpa_t tdr)
+{
+ return kvm_seamcall(TDH_MNG_VPFLUSHDONE, tdr, 0, 0, 0, NULL);
+}
+
+static inline u64 tdh_mng_key_freeid(hpa_t tdr)
+{
+ return kvm_seamcall(TDH_MNG_KEY_FREEID, tdr, 0, 0, 0, NULL);
+}
+
+static inline u64 tdh_mng_init(hpa_t tdr, hpa_t td_params,
+ struct tdx_module_output *out)
+{
+ return kvm_seamcall(TDH_MNG_INIT, tdr, td_params, 0, 0, out);
+}
+
+static inline u64 tdh_vp_init(hpa_t tdvpr, u64 rcx)
+{
+ return kvm_seamcall(TDH_VP_INIT, tdvpr, rcx, 0, 0, NULL);
+}
+
+static inline u64 tdh_vp_rd(hpa_t tdvpr, u64 field,
+ struct tdx_module_output *out)
+{
+ return kvm_seamcall(TDH_VP_RD, tdvpr, field, 0, 0, out);
+}
+
+static inline u64 tdh_mng_key_reclaimid(hpa_t tdr)
+{
+ return kvm_seamcall(TDH_MNG_KEY_RECLAIMID, tdr, 0, 0, 0, NULL);
+}
+
+static inline u64 tdh_phymem_page_reclaim(hpa_t page,
+ struct tdx_module_output *out)
+{
+ return kvm_seamcall(TDH_PHYMEM_PAGE_RECLAIM, page, 0, 0, 0, out);
+}
+
+static inline u64 tdh_mem_page_remove(hpa_t tdr, gpa_t gpa, int level,
+ struct tdx_module_output *out)
+{
+ return kvm_seamcall(TDH_MEM_PAGE_REMOVE, gpa | level, tdr, 0, 0, out);
+}
+
+static inline u64 tdh_sys_lp_shutdown(void)
+{
+ return kvm_seamcall(TDH_SYS_LP_SHUTDOWN, 0, 0, 0, 0, NULL);
+}
+
+static inline u64 tdh_mem_track(hpa_t tdr)
+{
+ return kvm_seamcall(TDH_MEM_TRACK, tdr, 0, 0, 0, NULL);
+}
+
+static inline u64 tdh_mem_range_unblock(hpa_t tdr, gpa_t gpa, int level,
+ struct tdx_module_output *out)
+{
+ return kvm_seamcall(TDH_MEM_RANGE_UNBLOCK, gpa | level, tdr, 0, 0, out);
+}
+
+static inline u64 tdh_phymem_cache_wb(bool resume)
+{
+ return kvm_seamcall(TDH_PHYMEM_CACHE_WB, resume ? 1 : 0, 0, 0, 0, NULL);
+}
+
+static inline u64 tdh_phymem_page_wbinvd(hpa_t page)
+{
+ return kvm_seamcall(TDH_PHYMEM_PAGE_WBINVD, page, 0, 0, 0, NULL);
+}
+
+static inline u64 tdh_vp_wr(hpa_t tdvpr, u64 field, u64 val, u64 mask,
+ struct tdx_module_output *out)
+{
+ return kvm_seamcall(TDH_VP_WR, tdvpr, field, val, mask, out);
+}
+
+#endif /* __KVM_X86_TDX_OPS_H */
diff --git a/arch/x86/virt/vmx/tdx/seamcall.S b/arch/x86/virt/vmx/tdx/seamcall.S
index f81be6b9c133..b90a7fe05494 100644
--- a/arch/x86/virt/vmx/tdx/seamcall.S
+++ b/arch/x86/virt/vmx/tdx/seamcall.S
@@ -1,5 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0 */
#include <linux/linkage.h>
+#include <asm/export.h>
#include <asm/frame.h>
#include "tdxcall.S"
@@ -50,3 +51,4 @@ SYM_FUNC_START(__seamcall)
FRAME_END
RET
SYM_FUNC_END(__seamcall)
+EXPORT_SYMBOL_GPL(__seamcall)
diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
index 48f830087e7e..4e497f202586 100644
--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -144,7 +144,4 @@ struct tdmr_info_list {
int max_tdmrs; /* How many 'tdmr_info's are allocated */
};
-struct tdx_module_output;
-u64 __seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
- struct tdx_module_output *out);
#endif
--
2.25.1
On Monday, May 29, 2023 12:19 PM, [email protected] wrote:
> From: Isaku Yamahata <[email protected]>
>
> A VMM interacts with the TDX module using a new instruction (SEAMCALL).
> For instance, a TDX VMM does not have full access to the VM control
> structure corresponding to VMX VMCS. Instead, a VMM induces the TDX
> module to act on behalf via SEAMCALLs.
>
> Export __seamcall and define C wrapper functions for SEAMCALLs for
> readability.
>
> Some SEAMCALL APIs donate host pages to TDX module or guest TD, and the
> donated pages are encrypted. Such SEAMCALLs flush cache lines (typically by
> movdir64b instruction), but some don't. Those that don't clear cache lines
> require the VMM to flush the cache lines to avoid cache line alias.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> Signed-off-by: Isaku Yamahata <[email protected]>
> ---
> arch/x86/include/asm/tdx.h | 4 +
> arch/x86/kvm/vmx/tdx_ops.h | 202
> +++++++++++++++++++++++++++++++
> arch/x86/virt/vmx/tdx/seamcall.S | 2 +
> arch/x86/virt/vmx/tdx/tdx.h | 3 -
> 4 files changed, 208 insertions(+), 3 deletions(-) create mode 100644
> arch/x86/kvm/vmx/tdx_ops.h
>
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h index
> 112a5b9bd5cd..6c01ab572c1f 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -104,10 +104,14 @@ static inline long tdx_kvm_hypercall(unsigned int
> nr, unsigned long p1, bool platform_tdx_enabled(void); int
> tdx_cpu_enable(void); int tdx_enable(void);
> +u64 __seamcall(u64 op, u64 rcx, u64 rdx, u64 r8, u64 r9,
> + struct tdx_module_output *out);
> #else /* !CONFIG_INTEL_TDX_HOST */
> static inline bool platform_tdx_enabled(void) { return false; } static inline int
> tdx_cpu_enable(void) { return -EINVAL; } static inline int tdx_enable(void)
> { return -EINVAL; }
> +static inline u64 __seamcall(u64 op, u64 rcx, u64 rdx, u64 r8, u64 r9,
> + struct tdx_module_output *out) { return
> TDX_SEAMCALL_UD; };
> #endif /* CONFIG_INTEL_TDX_HOST */
>
> #endif /* !__ASSEMBLY__ */
> diff --git a/arch/x86/kvm/vmx/tdx_ops.h b/arch/x86/kvm/vmx/tdx_ops.h
> new file mode 100644 index 000000000000..893cc6c25f3b
> --- /dev/null
> +++ b/arch/x86/kvm/vmx/tdx_ops.h
> @@ -0,0 +1,202 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* constants/data definitions for TDX SEAMCALLs */
> +
> +#ifndef __KVM_X86_TDX_OPS_H
> +#define __KVM_X86_TDX_OPS_H
> +
> +#include <linux/compiler.h>
> +
> +#include <asm/cacheflush.h>
> +#include <asm/asm.h>
> +#include <asm/kvm_host.h>
> +
> +#include "tdx_errno.h"
> +#include "tdx_arch.h"
> +#include "x86.h"
> +
> +static inline u64 kvm_seamcall(u64 op, u64 rcx, u64 rdx, u64 r8, u64 r9,
> + struct tdx_module_output *out) {
As discussed somewhere before, kvm_* is more common to be labelled for the
generic code. Would it be better to be named tdx_seamcall here?
On Thu, Jun 01, 2023 at 01:24:35PM +0000,
"Wang, Wei W" <[email protected]> wrote:
> > diff --git a/arch/x86/kvm/vmx/tdx_ops.h b/arch/x86/kvm/vmx/tdx_ops.h
> > new file mode 100644 index 000000000000..893cc6c25f3b
> > --- /dev/null
> > +++ b/arch/x86/kvm/vmx/tdx_ops.h
> > @@ -0,0 +1,202 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* constants/data definitions for TDX SEAMCALLs */
> > +
> > +#ifndef __KVM_X86_TDX_OPS_H
> > +#define __KVM_X86_TDX_OPS_H
> > +
> > +#include <linux/compiler.h>
> > +
> > +#include <asm/cacheflush.h>
> > +#include <asm/asm.h>
> > +#include <asm/kvm_host.h>
> > +
> > +#include "tdx_errno.h"
> > +#include "tdx_arch.h"
> > +#include "x86.h"
> > +
> > +static inline u64 kvm_seamcall(u64 op, u64 rcx, u64 rdx, u64 r8, u64 r9,
> > + struct tdx_module_output *out) {
>
> As discussed somewhere before, kvm_* is more common to be labelled for the
> generic code. Would it be better to be named tdx_seamcall here?
Ok, let's rename it to tdx_seamcall(), that's more consistent in this file.
--
Isaku Yamahata <[email protected]>
On Monday, May 29, 2023 12:19 PM, [email protected] wrote:
> From: Isaku Yamahata <[email protected]>
>
> A VMM interacts with the TDX module using a new instruction (SEAMCALL).
> For instance, a TDX VMM does not have full access to the VM control
> structure corresponding to VMX VMCS. Instead, a VMM induces the TDX
> module to act on behalf via SEAMCALLs.
>
> Export __seamcall and define C wrapper functions for SEAMCALLs for
> readability.
>
> Some SEAMCALL APIs donate host pages to TDX module or guest TD, and the
> donated pages are encrypted. Such SEAMCALLs flush cache lines (typically by
> movdir64b instruction), but some don't. Those that don't clear cache lines
> require the VMM to flush the cache lines to avoid cache line alias.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> Signed-off-by: Isaku Yamahata <[email protected]>
> ---
> arch/x86/include/asm/tdx.h | 4 +
> arch/x86/kvm/vmx/tdx_ops.h | 202
> +++++++++++++++++++++++++++++++
> arch/x86/virt/vmx/tdx/seamcall.S | 2 +
> arch/x86/virt/vmx/tdx/tdx.h | 3 -
> 4 files changed, 208 insertions(+), 3 deletions(-) create mode 100644
> arch/x86/kvm/vmx/tdx_ops.h
>
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h index
> 112a5b9bd5cd..6c01ab572c1f 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -104,10 +104,14 @@ static inline long tdx_kvm_hypercall(unsigned int
> nr, unsigned long p1, bool platform_tdx_enabled(void); int
> tdx_cpu_enable(void); int tdx_enable(void);
> +u64 __seamcall(u64 op, u64 rcx, u64 rdx, u64 r8, u64 r9,
> + struct tdx_module_output *out);
> #else /* !CONFIG_INTEL_TDX_HOST */
> static inline bool platform_tdx_enabled(void) { return false; } static inline int
> tdx_cpu_enable(void) { return -EINVAL; } static inline int tdx_enable(void)
> { return -EINVAL; }
> +static inline u64 __seamcall(u64 op, u64 rcx, u64 rdx, u64 r8, u64 r9,
> + struct tdx_module_output *out) { return
> TDX_SEAMCALL_UD; };
> #endif /* CONFIG_INTEL_TDX_HOST */
>
> #endif /* !__ASSEMBLY__ */
> diff --git a/arch/x86/kvm/vmx/tdx_ops.h b/arch/x86/kvm/vmx/tdx_ops.h
> new file mode 100644 index 000000000000..893cc6c25f3b
> --- /dev/null
> +++ b/arch/x86/kvm/vmx/tdx_ops.h
> @@ -0,0 +1,202 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* constants/data definitions for TDX SEAMCALLs */
> +
> +#ifndef __KVM_X86_TDX_OPS_H
> +#define __KVM_X86_TDX_OPS_H
> +
> +#include <linux/compiler.h>
> +
> +#include <asm/cacheflush.h>
> +#include <asm/asm.h>
> +#include <asm/kvm_host.h>
> +
> +#include "tdx_errno.h"
> +#include "tdx_arch.h"
> +#include "x86.h"
> +
> +static inline u64 kvm_seamcall(u64 op, u64 rcx, u64 rdx, u64 r8, u64 r9,
> + struct tdx_module_output *out) {
> + u64 ret;
> +
> + ret = __seamcall(op, rcx, rdx, r8, r9, out);
> + if (unlikely(ret == TDX_SEAMCALL_UD)) {
> + /*
> + * TDX requires VMXON or #UD. In the case of reboot or
> kexec,
> + * VMX is made off (VMXOFF) by kvm reboot notifier,
> + * kvm_reboot(), while TDs are still running. The callers
> check
> + * the returned error and complain. Suppress it by returning 0.
> + */
Curious how do the callers check the returned error when " Suppress
it by returning 0" here.
> + kvm_spurious_fault();
> + return 0;
> + }
> + return ret;
> +}
On Mon, Jun 05, 2023 at 03:20:19PM +0000,
"Wang, Wei W" <[email protected]> wrote:
> > diff --git a/arch/x86/kvm/vmx/tdx_ops.h b/arch/x86/kvm/vmx/tdx_ops.h
> > new file mode 100644 index 000000000000..893cc6c25f3b
> > --- /dev/null
> > +++ b/arch/x86/kvm/vmx/tdx_ops.h
> > @@ -0,0 +1,202 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* constants/data definitions for TDX SEAMCALLs */
> > +
> > +#ifndef __KVM_X86_TDX_OPS_H
> > +#define __KVM_X86_TDX_OPS_H
> > +
> > +#include <linux/compiler.h>
> > +
> > +#include <asm/cacheflush.h>
> > +#include <asm/asm.h>
> > +#include <asm/kvm_host.h>
> > +
> > +#include "tdx_errno.h"
> > +#include "tdx_arch.h"
> > +#include "x86.h"
> > +
> > +static inline u64 kvm_seamcall(u64 op, u64 rcx, u64 rdx, u64 r8, u64 r9,
> > + struct tdx_module_output *out) {
> > + u64 ret;
> > +
> > + ret = __seamcall(op, rcx, rdx, r8, r9, out);
> > + if (unlikely(ret == TDX_SEAMCALL_UD)) {
> > + /*
> > + * TDX requires VMXON or #UD. In the case of reboot or
> > kexec,
> > + * VMX is made off (VMXOFF) by kvm reboot notifier,
> > + * kvm_reboot(), while TDs are still running. The callers
> > check
> > + * the returned error and complain. Suppress it by returning 0.
> > + */
>
> Curious how do the callers check the returned error when " Suppress
> it by returning 0" here.
It doesn't make sense for the caller to check the error and warn when
kvm_rebooting = true.
Let's make it "return kvm_rebooting ? 0 : ret;" instread of "return 0;".
Does it make sense?
--
Isaku Yamahata <[email protected]>
On Thursday, June 8, 2023 2:16 AM, Isaku Yamahata wrote:
> On Mon, Jun 05, 2023 at 03:20:19PM +0000, "Wang, Wei W"
> <[email protected]> wrote:
> > > +static inline u64 kvm_seamcall(u64 op, u64 rcx, u64 rdx, u64 r8, u64 r9,
> > > + struct tdx_module_output *out) {
> > > + u64 ret;
> > > +
> > > + ret = __seamcall(op, rcx, rdx, r8, r9, out);
> > > + if (unlikely(ret == TDX_SEAMCALL_UD)) {
> > > + /*
> > > + * TDX requires VMXON or #UD. In the case of reboot or
> > > kexec,
> > > + * VMX is made off (VMXOFF) by kvm reboot notifier,
> > > + * kvm_reboot(), while TDs are still running. The callers
> > > check
> > > + * the returned error and complain. Suppress it by returning 0.
> > > + */
> >
> > Curious how do the callers check the returned error when " Suppress it
> > by returning 0" here.
>
> It doesn't make sense for the caller to check the error and warn when
> kvm_rebooting = true.
> Let's make it "return kvm_rebooting ? 0 : ret;" instread of "return 0;".
> Does it make sense?
Seems no need. The comments look confusing, and not aligned to what
the code achieves. From what I read:
- if kvm_rebooting=true there: return 0 to caller and no error or warning happens
- if kvm_rebooting=false there: crash the system via kvm_spurious_fault.
In this non-rebooting case, I think the callers don’t get a chance to read the
returned value and complain.
Another thing is, have you double-checked that invocation of seamcalls
indeed returns TDX_SEAMCALL_UD when VMX=off?
On Thu, Jun 08, 2023 at 01:43:27AM +0000,
"Wang, Wei W" <[email protected]> wrote:
> On Thursday, June 8, 2023 2:16 AM, Isaku Yamahata wrote:
> > On Mon, Jun 05, 2023 at 03:20:19PM +0000, "Wang, Wei W"
> > <[email protected]> wrote:
> > > > +static inline u64 kvm_seamcall(u64 op, u64 rcx, u64 rdx, u64 r8, u64 r9,
> > > > + struct tdx_module_output *out) {
> > > > + u64 ret;
> > > > +
> > > > + ret = __seamcall(op, rcx, rdx, r8, r9, out);
> > > > + if (unlikely(ret == TDX_SEAMCALL_UD)) {
> > > > + /*
> > > > + * TDX requires VMXON or #UD. In the case of reboot or
> > > > kexec,
> > > > + * VMX is made off (VMXOFF) by kvm reboot notifier,
> > > > + * kvm_reboot(), while TDs are still running. The callers
> > > > check
> > > > + * the returned error and complain. Suppress it by returning 0.
> > > > + */
> > >
> > > Curious how do the callers check the returned error when " Suppress it
> > > by returning 0" here.
> >
> > It doesn't make sense for the caller to check the error and warn when
> > kvm_rebooting = true.
> > Let's make it "return kvm_rebooting ? 0 : ret;" instread of "return 0;".
> > Does it make sense?
>
> Seems no need. The comments look confusing, and not aligned to what
> the code achieves. From what I read:
> - if kvm_rebooting=true there: return 0 to caller and no error or warning happens
> - if kvm_rebooting=false there: crash the system via kvm_spurious_fault.
> In this non-rebooting case, I think the callers don’t get a chance to read the
> returned value and complain.
How about this comment?
if (unlikely(ret == TDX_SEAMCALL_UD)) {
/*
* TDX requires VMXON or #UD. In the case of reboot or kexec,
* kvm shutdown notifier, kvm_shutdown(), makes VMX off (VMXOFF)
* while TDs can be still running to invoke SEAMCALL. It
* results in superfluous errors or warnings.
* If rebooting, return 0 to suppress superfluous messages.
* If not rebooting, panic by kvm_spurious_fault().
*/
kvm_spurious_fault();
return 0;
}
> Another thing is, have you double-checked that invocation of seamcalls
> indeed returns TDX_SEAMCALL_UD when VMX=off?
I observed it several times during testing tdx module initialization. Here is
the example I dug out from my debug logs.
tdx: SEAMCALL failed: CPU 162 is not in VMX operation.
tdx: TDX module initialization failed (-22)
kvm_intel: Failed to initialize TDX module.
--
Isaku Yamahata <[email protected]>
On Friday, June 9, 2023 4:11 AM, Isaku Yamahata wrote:
> How about this comment?
>
> if (unlikely(ret == TDX_SEAMCALL_UD)) {
Where is the TDX_SEAMCALL_UD error code documented in the spec?
I failed to find it.
> /*
> * TDX requires VMXON or #UD.
TDX requires #UD? Seems a bit ambiguous.
> In the case of reboot or kexec,
> * kvm shutdown notifier, kvm_shutdown(), makes VMX off
> (VMXOFF)
> * while TDs can be still running to invoke SEAMCALL. It
> * results in superfluous errors or warnings.
> * If rebooting, return 0 to suppress superfluous messages.
> * If not rebooting, panic by kvm_spurious_fault().
> */
> kvm_spurious_fault();
I would put it this way:
SEAMCALLs fail with TDX_SEAMCALL_UD returned when VMX is off.
This can happen when host gets rebooted or live updated. In this case,
the instruction execution is actually ignored as KVM is shut down, so
the error code is suppressed. Other than this, the error is unexpected
and the execution can't continue as the TDX features reply on VMX to
be on.
On Wed, Jun 14, 2023 at 11:45:49AM +0000,
"Wang, Wei W" <[email protected]> wrote:
> On Friday, June 9, 2023 4:11 AM, Isaku Yamahata wrote:
> > How about this comment?
> >
> > if (unlikely(ret == TDX_SEAMCALL_UD)) {
>
> Where is the TDX_SEAMCALL_UD error code documented in the spec?
> I failed to find it.
This is not a part of the spec, but a convention of __seamcall().
Please refer to
https://lore.kernel.org/all/ec640452a4385d61bec97f8b761ed1ff38898504.1685887183.git.kai.huang@intel.com/
> > In the case of reboot or kexec,
> > * kvm shutdown notifier, kvm_shutdown(), makes VMX off
> > (VMXOFF)
> > * while TDs can be still running to invoke SEAMCALL. It
> > * results in superfluous errors or warnings.
> > * If rebooting, return 0 to suppress superfluous messages.
> > * If not rebooting, panic by kvm_spurious_fault().
> > */
> > kvm_spurious_fault();
>
> I would put it this way:
> SEAMCALLs fail with TDX_SEAMCALL_UD returned when VMX is off.
> This can happen when host gets rebooted or live updated. In this case,
> the instruction execution is actually ignored as KVM is shut down, so
> the error code is suppressed. Other than this, the error is unexpected
> and the execution can't continue as the TDX features reply on VMX to
> be on.
Thanks for it. I made minor fix to it.
if (unlikely(ret == TDX_SEAMCALL_UD)) {
/*
* SEAMCALLs fail with TDX_SEAMCALL_UD returned when VMX is off.
* This can happen when the host gets rebooted or live
* updated. In this case, the instruction execution is ignored
* as KVM is shut down, so the error code is suppressed. Other
* than this, the error is unexpected and the execution can't
* continue as the TDX features reply on VMX to be on.
*/
kvm_spurious_fault();
return 0;
}
--
Isaku Yamahata <[email protected]>