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 12:04:51 +0000 |
| |
On Fri, 2023-07-21 at 11:44 +0000, Huang, Kai wrote: > 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); >
Damn.. I think it's better to split this into two patches:
1) Introduce __seamcall_saved_ret() for VP.ENTER 2) Adjust 'struct tdx_module_args' layout
Because 1) is mandatory to support VP.ENTER, but 2) theoretically is an optimization.
Then perhaps 1) can even be merged to patch "x86/virt/tdx: Wire up basic SEAMCALL functions".
I'll make those patches and test the VP.ENTER actually works using the adjusted 'struct tdx_module_args' layout, and post v3 when done.
Thanks again Peter, and let me know if you have any comments here.
| |