Messages in this thread Patch in this message | | | From | "Huang, Kai" <> | Subject | Re: [PATCH v2 00/11] Unify TDCALL/SEAMCALL and TDVMCALL assembly | Date | Fri, 21 Jul 2023 11:44:42 +0000 |
| |
On Fri, 2023-07-21 at 11:02 +0000, Huang, Kai wrote: > On Fri, 2023-07-21 at 10:06 +0200, Peter Zijlstra wrote: > > On Fri, Jul 21, 2023 at 12:18:23AM +0000, Huang, Kai wrote: > > > > > Unfortunately I don't think it's feasible. Sean pointed out that > > > kvm_vcpu_arch::regs[] do follow the "register index" hardware layout in x86 (for > > > which I missed sorry), so we cannot re-order KVM part. > > > > > > And unfortunately RBP (5) is in middle of those registers: > > > > > > 0 = RAX > > > 1 = RCX > > > 2 = RDX > > > 3 = RBX > > > 4 = RSP > > > 5 = RBP > > > 6 = RSI > > > 7 = RDI > > > 8–15 represent R8–R15, respectively... > > > > > > Thus unless we add RBP to 'struct tdx_module_args', it's impossible to re-order > > > the structure to match KVM's layout. > > > > Adding RBP to the struct shouldn't be a problem, we'll just not use it. > > Same as RSP, nobody sane would expect that to be used. If you worry > > about it you can give them funny names like: > > > > struct tdx_module_args { > > unsigned long ax; > > unsigned long cx; > > unsigned long dx; > > unsigned long bx; > > unsigned long __sp_unused; > > unsigned long __bp_unused; > > unsigned long si; > > unsigned long di; > > ... > > }; > > > > I mean, at this point the whole thing is just 128 bytes of data anyway. > > Sure I can do. > > How about adding an additional patch on top of this series? > > For instance, below patch (compile only): > > From c63d01bf91fe23f1e2d2e085644326bdee114d49 Mon Sep 17 00:00:00 2001 > From: Kai Huang <kai.huang@intel.com> > Date: Fri, 21 Jul 2023 22:06:31 +1200 > Subject: [PATCH] x86/tdx: Add __seamcall_saved_ret() for TDH.VP.ENTER > > For TDX guest, KVM uses TDH.VP.ENTER SEAMCALL leaf to enter the guest. > When TDH.VP.ENTER returns to KVM due to TDG.VP.VMCALL from the TDX > guest, all RCX/RDX/RBX/RDI/RSI/R8-R15 are valid output registers, and > the following TDH.VP.ENTER also uses all those registers as input. > > Introduce __seamcall_saved_ret(), which uses all above registers as > both input/output, for KVM to support TDH.VP.ENTER. > > Also, KVM caches guest's GPRs in 'kvm_vcpu_arch::regs[]', which follows > follows the "register index" hardware layout of x86 GPRs. However the > __seamcall_saved_ret() takes the pointer of 'struct tdx_module_args' as > argument, thus there's a mismatch here. > > One option is KVM to copy input registers from 'vcpu::regs[]' to a > 'struct tdx_module_args' on the stack and use that 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[]' 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. > > Signed-off-by: Kai Huang <kai.huang@intel.com> > --- > arch/x86/include/asm/shared/tdx.h | 19 +++++++++++++------ > arch/x86/virt/vmx/tdx/seamcall.S | 19 +++++++++++++++++++ > 2 files changed, 32 insertions(+), 6 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/virt/vmx/tdx/seamcall.S b/arch/x86/virt/vmx/tdx/seamcall.S > index b32934837f16..f251ebadb014 100644 > --- a/arch/x86/virt/vmx/tdx/seamcall.S > +++ b/arch/x86/virt/vmx/tdx/seamcall.S > @@ -40,3 +40,22 @@ SYM_FUNC_END(__seamcall) > SYM_FUNC_START(__seamcall_ret) > TDX_MODULE_CALL host=1 ret=1 > SYM_FUNC_END(__seamcall_ret) > + > +/* > + * __seamcall_saved_ret() - Host-side interface functions to SEAM software > + * (the P-SEAMLDR or the TDX module), with saving output registers to the > + * 'struct tdx_module_args' used as input. > + * > + * __seamcall_saved_ret() function ABI: > + * > + * @fn (RDI) - SEAMCALL Leaf number, moved to RAX > + * @args (RSI) - struct tdx_module_args for input and output > + * > + * All registers in @args are used as input/output registers. > + * > + * Return (via RAX) TDX_SEAMCALL_VMFAILINVALID if the SEAMCALL itself > + * fails, or the completion status of the SEAMCALL leaf function. > + */ > +SYM_FUNC_START(__seamcall_ret) > + TDX_MODULE_CALL host=1 ret=1 saved=1 > +SYM_FUNC_END(__seamcall_ret)
Sorry should be __seamcall_saved_ret.
> -- > 2.41.0 > >
Sorry missing the declaration of __seamcall_saved_ret():
--- a/arch/x86/include/asm/tdx.h +++ b/arch/x86/include/asm/tdx.h @@ -80,6 +80,7 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1, #ifdef CONFIG_INTEL_TDX_HOST u64 __seamcall(u64 fn, struct tdx_module_args *args); u64 __seamcall_ret(u64 fn, struct tdx_module_args *args); +u64 __seamcall_saved_ret(u64 fn, struct tdx_module_args *args);
And perhaps we should adjust the order of registers in asm-offset.c to match the change to the 'struct tdx_module_args'?
--- 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);
| |