Re: [PATCHv2 03/29] x86/tdx: Add __tdx_module_call() and __tdx_hypercall() helper functions

Kirill A. Shutemov
2022-02-03T14:43:58Z

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