Messages in this thread | | | Date | Thu, 3 Feb 2022 17:44:03 +0300 | From | "Kirill A. Shutemov" <> | Subject | Re: [PATCHv2 03/29] x86/tdx: Add __tdx_module_call() and __tdx_hypercall() helper functions |
| |
On Wed, Feb 02, 2022 at 11:59:10PM +1300, Kai Huang wrote: > > > /* > > diff --git a/arch/x86/kernel/tdxcall.S b/arch/x86/kernel/tdxcall.S > > new file mode 100644 > > index 000000000000..ba5e8e35de36 > > --- /dev/null > > +++ b/arch/x86/kernel/tdxcall.S > > @@ -0,0 +1,76 @@ > > +#include <asm/asm-offsets.h> > > + > > +/* > > + * TDX guests use the TDCALL instruction to make requests to the > > + * TDX module and hypercalls to the VMM. > > + * > > + * TDX host user SEAMCALL instruction to make requests to TDX module. > > + * > > + * They are supported in Binutils >= 2.36. > > + */ > > +#define tdcall .byte 0x66,0x0f,0x01,0xcc > > +#define seamcall .byte 0x66,0x0f,0x01,0xcf > > + > > +.macro TDX_MODULE_CALL host:req > > + /* > > + * R12 will be used as temporary storage for struct tdx_module_output > > + * pointer. Since R12-R15 registers are not used by TDCALL/SEAMCALL > > + * services supported by this function, it can be reused. > > + */ > > + > > + /* Callee saved, so preserve it */ > > + push %r12 > > + > > + /* > > + * Push output pointer to stack. > > + * After the operation, it will be fetched into R12 register. > > + */ > > + push %r9 > > + > > + /* Mangle function call ABI into TDCALL/SEAMCALL ABI: */ > > + /* Move Leaf ID to RAX */ > > + mov %rdi, %rax > > + /* Move input 4 to R9 */ > > + mov %r8, %r9 > > + /* Move input 3 to R8 */ > > + mov %rcx, %r8 > > + /* Move input 1 to RCX */ > > + mov %rsi, %rcx > > + /* Leave input param 2 in RDX */ > > Currently host seamcall also uses a data structure which has all possible input > registers as input argument, rather than having one argument for each register: > > struct seamcall_regs_in { > u64 rcx; > u64 rdx; > u64 r8; > u64 r9; > }; > > Which way is better (above struct name can be changed of course)? > > Or should we rename tdx_module_output to tdx_module_regs, and use it as both > input and output (similar to __tdx_hypercall())?
Unlike hypercall case, here we have more managable number of arguments. I would rather keep input arguments outside of any structs. It is easier for callers, IMO.
Any objections?
> > + > > + .if \host > > + seamcall > > + .else > > + tdcall > > + .endif > > + > > + /* > > + * Fetch output pointer from stack to R12 (It is used > > + * as temporary storage) > > + */ > > + pop %r12 > > For host SEAMCALL, one additional check against VMfailInvalid needs to be > done after here. This is because at host side, both P-SEAMLDR and TDX module > are expected to be loaded by UEFI (i.e. UEFI shell tool) before booting to the > kernel, therefore host kernel needs to detect whether them have been loaded, by > issuing SEAMCALL. > > When SEAM software (P-SEAMLDR or TDX module) is not loaded, the SEAMCALL fails > with VMfailInvalid. VMfailInvalid is indicated via a combination of RFLAGS > rather than using %rax. In practice, RFLAGS.CF=1 can be used to check whether > VMfailInvalid happened, and we need to have something like below: > > /* > * SEAMCALL instruction is essentially a VMExit from VMX root > * mode to SEAM VMX root mode. VMfailInvalid (CF=1) indicates > * that the targeted SEAM firmware is not loaded or disabled, > * or P-SEAMLDR is busy with another SEAMCALL. %rax is not > * changed in this case. > * > * Set %rax to VMFAILINVALID for VMfailInvalid. This value > * will never be used as actual SEAMCALL error code. > */ > jnb .Lno_vmfailinvalid > mov $(VMFAILINVALID), %rax > jmp .Lno_output_struct > > .Lno_vmfailinvalid:
Okay, I will add it under .if \host.
But maybe use JNC instead of JNB? Since we check for CF flag, Jump-if-Not-Carry sounds more reasonable than Jump-if-Not-Below. Not-Below of what?
-- Kirill A. Shutemov
| |