For TDX guest, KVM needs to call __seamcall_saved_ret() to make the
TDH.VP.ENTER SEAMCALL to enter the guest, possibly taking all registers
in 'struct tdx_module_args' as input/output.
KVM caches guest's GPRs in 'kvm_vcpu_arch::regs[]', which follows the
"register index" hardware layout of x86 GPRs. On the other hand, the
__seamcall_saved_ret() takes the pointer of 'struct tdx_module_args' as
argument, thus there's a mismatch.
KVM could choose to copy input registers from 'vcpu::regs[]' to a
'struct tdx_module_args' and use that as argument to make the SEAMCALL,
but such memory copy isn't desired and should be avoided if possible.
It's not feasible to change KVM's 'vcpu::regs[]' layout due to various
reasons (e.g., emulation code uses decoded register index as array index
to access the register). Therefore, adjust 'struct tdx_module_args' to
match KVM's 'vcpu::regs[]' layout so that KVM can simply do below:
__seamcall_saved_ret(TDH_VP_ENTER,
(struct tdx_module_args *)vcpu->arch.regs);
Note RAX/RSP/RBP are not used by the TDX_MODULE_CALL assembly, but they
are necessary in order match the layout of 'struct tdx_module_args' to
KVM's 'vcpu::regs[]'. Thus add them to the structure, but name them as
*_unused. Also don't include them to asm-offset.c so that any misuse of
them in the assembly would result in build error.
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Isaku Yamahata <isaku.yamahata@intel.com>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Kai Huang <kai.huang@intel.com>
---
v2 -> v3:
- New patch
---
arch/x86/include/asm/shared/tdx.h | 19 +++++++++++++------
arch/x86/kernel/asm-offsets.c | 6 +++---
2 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
index 74fc466dfdcd..8d1427562c63 100644
--- a/arch/x86/include/asm/shared/tdx.h
+++ b/arch/x86/include/asm/shared/tdx.h
@@ -58,24 +58,31 @@
* Used in __tdcall*() to gather the input/output registers' values of the
* TDCALL instruction when requesting services from the TDX module. This is a
* software only structure and not part of the TDX module/VMM ABI
+ *
+ * Note those *_unused are not used by the TDX_MODULE_CALL assembly.
+ * The layout of this structure also matches KVM's kvm_vcpu_arch::regs[]
+ * layout, which follows the "register index" order of x86 GPRs. KVM
+ * then can simply type cast kvm_vcpu_arch::regs[] to this structure to
+ * avoid the extra memory copy between two structures when making
+ * TDH.VP.ENTER SEAMCALL.
*/
struct tdx_module_args {
- /* callee-clobbered */
+ u64 rax_unused;
u64 rcx;
u64 rdx;
+ u64 rbx;
+ u64 rsp_unused;
+ u64 rbp_unused;
+ u64 rsi;
+ u64 rdi;
u64 r8;
u64 r9;
- /* extra callee-clobbered */
u64 r10;
u64 r11;
- /* callee-saved + rdi/rsi */
u64 r12;
u64 r13;
u64 r14;
u64 r15;
- u64 rbx;
- u64 rdi;
- u64 rsi;
};
/* Used to communicate with the TDX module */
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index 6913b372ccf7..e4ad822d3acd 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -70,6 +70,9 @@ static void __used common(void)
BLANK();
OFFSET(TDX_MODULE_rcx, tdx_module_args, rcx);
OFFSET(TDX_MODULE_rdx, tdx_module_args, rdx);
+ OFFSET(TDX_MODULE_rbx, tdx_module_args, rbx);
+ OFFSET(TDX_MODULE_rsi, tdx_module_args, rsi);
+ OFFSET(TDX_MODULE_rdi, tdx_module_args, rdi);
OFFSET(TDX_MODULE_r8, tdx_module_args, r8);
OFFSET(TDX_MODULE_r9, tdx_module_args, r9);
OFFSET(TDX_MODULE_r10, tdx_module_args, r10);
@@ -78,9 +81,6 @@ static void __used common(void)
OFFSET(TDX_MODULE_r13, tdx_module_args, r13);
OFFSET(TDX_MODULE_r14, tdx_module_args, r14);
OFFSET(TDX_MODULE_r15, tdx_module_args, r15);
- OFFSET(TDX_MODULE_rbx, tdx_module_args, rbx);
- OFFSET(TDX_MODULE_rdi, tdx_module_args, rdi);
- OFFSET(TDX_MODULE_rsi, tdx_module_args, rsi);
BLANK();
OFFSET(BP_scratch, boot_params, scratch);
--
2.41.0On Wed, Jul 26, 2023 at 11:25:14PM +1200, Kai Huang wrote: > For TDX guest, KVM needs to call __seamcall_saved_ret() to make the > TDH.VP.ENTER SEAMCALL to enter the guest, possibly taking all registers > in 'struct tdx_module_args' as input/output. > > KVM caches guest's GPRs in 'kvm_vcpu_arch::regs[]', which follows the > "register index" hardware layout of x86 GPRs. On the other hand, the > __seamcall_saved_ret() takes the pointer of 'struct tdx_module_args' as > argument, thus there's a mismatch. > > KVM could choose to copy input registers from 'vcpu::regs[]' to a > 'struct tdx_module_args' and use that as argument to make the SEAMCALL, > but such memory copy isn't desired and should be avoided if possible. I doubt the copy will be visible on any profile. I personally don't like that kvm implementation detail leaks here. It suppose to be generic TDX code. -- Kiryl Shutsemau / Kirill A. Shutemov
On Sun, 2023-08-06 at 14:50 +0300, kirill.shutemov@linux.intel.com wrote: > On Wed, Jul 26, 2023 at 11:25:14PM +1200, Kai Huang wrote: > > For TDX guest, KVM needs to call __seamcall_saved_ret() to make the > > TDH.VP.ENTER SEAMCALL to enter the guest, possibly taking all registers > > in 'struct tdx_module_args' as input/output. > > > > KVM caches guest's GPRs in 'kvm_vcpu_arch::regs[]', which follows the > > "register index" hardware layout of x86 GPRs. On the other hand, the > > __seamcall_saved_ret() takes the pointer of 'struct tdx_module_args' as > > argument, thus there's a mismatch. > > > > KVM could choose to copy input registers from 'vcpu::regs[]' to a > > 'struct tdx_module_args' and use that as argument to make the SEAMCALL, > > but such memory copy isn't desired and should be avoided if possible. > > I doubt the copy will be visible on any profile. > > I personally don't like that kvm implementation detail leaks here. It > suppose to be generic TDX code. > > Well I kinda agree with you. But it seems Peter wanted this to be done: https://lore.kernel.org/lkml/a23ce8fd289141cea3a1b4f3dace221dca847238.camel@intel.com/T/#m37f39493e9f2bf0a4c9ccc72aaf4938927375dc1
On Wed, Jul 26, 2023 at 11:25:14PM +1200,
Kai Huang <kai.huang@intel.com> wrote:
> For TDX guest, KVM needs to call __seamcall_saved_ret() to make the
> TDH.VP.ENTER SEAMCALL to enter the guest, possibly taking all registers
> in 'struct tdx_module_args' as input/output.
>
> KVM caches guest's GPRs in 'kvm_vcpu_arch::regs[]', which follows the
> "register index" hardware layout of x86 GPRs. On the other hand, the
> __seamcall_saved_ret() takes the pointer of 'struct tdx_module_args' as
> argument, thus there's a mismatch.
>
> KVM could choose to copy input registers from 'vcpu::regs[]' to a
> 'struct tdx_module_args' and use that as argument to make the SEAMCALL,
> but such memory copy isn't desired and should be avoided if possible.
>
> It's not feasible to change KVM's 'vcpu::regs[]' layout due to various
> reasons (e.g., emulation code uses decoded register index as array index
> to access the register). Therefore, adjust 'struct tdx_module_args' to
> match KVM's 'vcpu::regs[]' layout so that KVM can simply do below:
>
> __seamcall_saved_ret(TDH_VP_ENTER,
> (struct tdx_module_args *)vcpu->arch.regs);
>
> Note RAX/RSP/RBP are not used by the TDX_MODULE_CALL assembly, but they
> are necessary in order match the layout of 'struct tdx_module_args' to
> KVM's 'vcpu::regs[]'. Thus add them to the structure, but name them as
> *_unused. Also don't include them to asm-offset.c so that any misuse of
> them in the assembly would result in build error.
Maybe we can have static check if the offsets match.
e.g. BUILD_BUG_ON(__VCPU_REGS_RAX * 8 != TDX_MODULE_rax); etc...
Anyway, I can have such a patch in TDX KVM side when I use this function for
TDH.VP.ENTER.
Thansk,
>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Isaku Yamahata <isaku.yamahata@intel.com>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> ---
>
> v2 -> v3:
> - New patch
>
> ---
> arch/x86/include/asm/shared/tdx.h | 19 +++++++++++++------
> arch/x86/kernel/asm-offsets.c | 6 +++---
> 2 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
> index 74fc466dfdcd..8d1427562c63 100644
> --- a/arch/x86/include/asm/shared/tdx.h
> +++ b/arch/x86/include/asm/shared/tdx.h
> @@ -58,24 +58,31 @@
> * Used in __tdcall*() to gather the input/output registers' values of the
> * TDCALL instruction when requesting services from the TDX module. This is a
> * software only structure and not part of the TDX module/VMM ABI
> + *
> + * Note those *_unused are not used by the TDX_MODULE_CALL assembly.
> + * The layout of this structure also matches KVM's kvm_vcpu_arch::regs[]
> + * layout, which follows the "register index" order of x86 GPRs. KVM
> + * then can simply type cast kvm_vcpu_arch::regs[] to this structure to
> + * avoid the extra memory copy between two structures when making
> + * TDH.VP.ENTER SEAMCALL.
> */
> struct tdx_module_args {
> - /* callee-clobbered */
> + u64 rax_unused;
> u64 rcx;
> u64 rdx;
> + u64 rbx;
> + u64 rsp_unused;
> + u64 rbp_unused;
> + u64 rsi;
> + u64 rdi;
> u64 r8;
> u64 r9;
> - /* extra callee-clobbered */
> u64 r10;
> u64 r11;
> - /* callee-saved + rdi/rsi */
> u64 r12;
> u64 r13;
> u64 r14;
> u64 r15;
> - u64 rbx;
> - u64 rdi;
> - u64 rsi;
> };
>
> /* Used to communicate with the TDX module */
> diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
> index 6913b372ccf7..e4ad822d3acd 100644
> --- a/arch/x86/kernel/asm-offsets.c
> +++ b/arch/x86/kernel/asm-offsets.c
> @@ -70,6 +70,9 @@ static void __used common(void)
> BLANK();
> OFFSET(TDX_MODULE_rcx, tdx_module_args, rcx);
> OFFSET(TDX_MODULE_rdx, tdx_module_args, rdx);
> + OFFSET(TDX_MODULE_rbx, tdx_module_args, rbx);
> + OFFSET(TDX_MODULE_rsi, tdx_module_args, rsi);
> + OFFSET(TDX_MODULE_rdi, tdx_module_args, rdi);
> OFFSET(TDX_MODULE_r8, tdx_module_args, r8);
> OFFSET(TDX_MODULE_r9, tdx_module_args, r9);
> OFFSET(TDX_MODULE_r10, tdx_module_args, r10);
> @@ -78,9 +81,6 @@ static void __used common(void)
> OFFSET(TDX_MODULE_r13, tdx_module_args, r13);
> OFFSET(TDX_MODULE_r14, tdx_module_args, r14);
> OFFSET(TDX_MODULE_r15, tdx_module_args, r15);
> - OFFSET(TDX_MODULE_rbx, tdx_module_args, rbx);
> - OFFSET(TDX_MODULE_rdi, tdx_module_args, rdi);
> - OFFSET(TDX_MODULE_rsi, tdx_module_args, rsi);
>
> BLANK();
> OFFSET(BP_scratch, boot_params, scratch);
> --
> 2.41.0
>
--
Isaku Yamahata <isaku.yamahata@gmail.com>
On Wed, Jul 26, 2023 at 11:25:14PM +1200,
Kai Huang <kai.huang@intel.com> wrote:
> For TDX guest, KVM needs to call __seamcall_saved_ret() to make the
> TDH.VP.ENTER SEAMCALL to enter the guest, possibly taking all registers
> in 'struct tdx_module_args' as input/output.
>
> KVM caches guest's GPRs in 'kvm_vcpu_arch::regs[]', which follows the
> "register index" hardware layout of x86 GPRs. On the other hand, the
> __seamcall_saved_ret() takes the pointer of 'struct tdx_module_args' as
> argument, thus there's a mismatch.
>
> KVM could choose to copy input registers from 'vcpu::regs[]' to a
> 'struct tdx_module_args' and use that as argument to make the SEAMCALL,
> but such memory copy isn't desired and should be avoided if possible.
>
> It's not feasible to change KVM's 'vcpu::regs[]' layout due to various
> reasons (e.g., emulation code uses decoded register index as array index
> to access the register). Therefore, adjust 'struct tdx_module_args' to
> match KVM's 'vcpu::regs[]' layout so that KVM can simply do below:
>
> __seamcall_saved_ret(TDH_VP_ENTER,
> (struct tdx_module_args *)vcpu->arch.regs);
>
> Note RAX/RSP/RBP are not used by the TDX_MODULE_CALL assembly, but they
> are necessary in order match the layout of 'struct tdx_module_args' to
> KVM's 'vcpu::regs[]'. Thus add them to the structure, but name them as
> *_unused. Also don't include them to asm-offset.c so that any misuse of
> them in the assembly would result in build error.
>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Isaku Yamahata <isaku.yamahata@intel.com>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> ---
>
> v2 -> v3:
> - New patch
>
> ---
> arch/x86/include/asm/shared/tdx.h | 19 +++++++++++++------
> arch/x86/kernel/asm-offsets.c | 6 +++---
> 2 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
> index 74fc466dfdcd..8d1427562c63 100644
> --- a/arch/x86/include/asm/shared/tdx.h
> +++ b/arch/x86/include/asm/shared/tdx.h
> @@ -58,24 +58,31 @@
> * Used in __tdcall*() to gather the input/output registers' values of the
> * TDCALL instruction when requesting services from the TDX module. This is a
> * software only structure and not part of the TDX module/VMM ABI
> + *
> + * Note those *_unused are not used by the TDX_MODULE_CALL assembly.
> + * The layout of this structure also matches KVM's kvm_vcpu_arch::regs[]
> + * layout, which follows the "register index" order of x86 GPRs. KVM
> + * then can simply type cast kvm_vcpu_arch::regs[] to this structure to
> + * avoid the extra memory copy between two structures when making
> + * TDH.VP.ENTER SEAMCALL.
> */
> struct tdx_module_args {
> - /* callee-clobbered */
> + u64 rax_unused;
> u64 rcx;
> u64 rdx;
> + u64 rbx;
> + u64 rsp_unused;
> + u64 rbp_unused;
> + u64 rsi;
> + u64 rdi;
> u64 r8;
> u64 r9;
> - /* extra callee-clobbered */
> u64 r10;
> u64 r11;
> - /* callee-saved + rdi/rsi */
> u64 r12;
> u64 r13;
> u64 r14;
> u64 r15;
> - u64 rbx;
> - u64 rdi;
> - u64 rsi;
> };
>
> /* Used to communicate with the TDX module */
> diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
> index 6913b372ccf7..e4ad822d3acd 100644
> --- a/arch/x86/kernel/asm-offsets.c
> +++ b/arch/x86/kernel/asm-offsets.c
> @@ -70,6 +70,9 @@ static void __used common(void)
> BLANK();
> OFFSET(TDX_MODULE_rcx, tdx_module_args, rcx);
> OFFSET(TDX_MODULE_rdx, tdx_module_args, rdx);
> + OFFSET(TDX_MODULE_rbx, tdx_module_args, rbx);
> + OFFSET(TDX_MODULE_rsi, tdx_module_args, rsi);
> + OFFSET(TDX_MODULE_rdi, tdx_module_args, rdi);
> OFFSET(TDX_MODULE_r8, tdx_module_args, r8);
> OFFSET(TDX_MODULE_r9, tdx_module_args, r9);
> OFFSET(TDX_MODULE_r10, tdx_module_args, r10);
> @@ -78,9 +81,6 @@ static void __used common(void)
> OFFSET(TDX_MODULE_r13, tdx_module_args, r13);
> OFFSET(TDX_MODULE_r14, tdx_module_args, r14);
> OFFSET(TDX_MODULE_r15, tdx_module_args, r15);
> - OFFSET(TDX_MODULE_rbx, tdx_module_args, rbx);
> - OFFSET(TDX_MODULE_rdi, tdx_module_args, rdi);
> - OFFSET(TDX_MODULE_rsi, tdx_module_args, rsi);
>
> BLANK();
> OFFSET(BP_scratch, boot_params, scratch);
> --
> 2.41.0
I replaced the current TDX KVM TDH.VP.ENTER with this function and it worked.
Test-by: Isaku Yamahata <isaku.yamahata@intel.com>
--
Isaku Yamahata <isaku.yamahata@gmail.com>
> > I replaced the current TDX KVM TDH.VP.ENTER with this function and it worked. > > Test-by: Isaku Yamahata <isaku.yamahata@intel.com> I suppose you mean: Tested-by: ... :-) Anyway I only sent this series out after I got confirmation from you that VP.ENTER worked using the SEAMCALL function in this series, but I forgot to add your Tested-by before sending out this series. Will add. Thanks!
© 2016 - 2026 Red Hat, Inc.