Re: [PATCH v2 00/11] Unify TDCALL/SEAMCALL and TDVMCALL assembly

From: Huang, Kai
Date: Fri Jul 21 2023 - 08:05:29 EST

Next message: Alexandre Mergnat: "Re: [PATCH v2 1/3] arm64: dts: mediatek: mt6795: Add support for display blocks and DPI/DSI"
Previous message: Alain Volmat: "[PATCH v2 2/2] media: stm32: dcmi: only call s_stream on the source subdev"
In reply to: Huang, Kai: "Re: [PATCH v2 00/11] Unify TDCALL/SEAMCALL and TDVMCALL assembly"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>
> > 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@xxxxxxxxx>
> > ---
> > 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.

Next message: Alexandre Mergnat: "Re: [PATCH v2 1/3] arm64: dts: mediatek: mt6795: Add support for display blocks and DPI/DSI"
Previous message: Alain Volmat: "[PATCH v2 2/2] media: stm32: dcmi: only call s_stream on the source subdev"
In reply to: Huang, Kai: "Re: [PATCH v2 00/11] Unify TDCALL/SEAMCALL and TDVMCALL assembly"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]