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

Thomas Gleixner
2022-02-01T19:59:04Z

Kirill,

On Mon, Jan 24 2022 at 18:01, Kirill A. Shutemov wrote:
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -8,11 +8,51 @@
> #define TDX_CPUID_LEAF_ID 0x21
> #define TDX_IDENT "IntelTDX "
>
> +#define TDX_HYPERCALL_STANDARD 0
> +
> +/*
> + * Used in __tdx_module_call() to gather the 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
> + */
> +struct tdx_module_output {
> + u64 rcx;
> + u64 rdx;
> + u64 r8;
> + u64 r9;
> + u64 r10;
> + u64 r11;
> +};

I've seen exactly the same struct named seamcall_regs_out in the TDX
host series. I assume that's not coincidence which begs the question why
this is required twice with different names.

> +#ifdef CONFIG_INTEL_TDX_GUEST
> + BLANK();
> + /* Offset for fields in tdx_module_output */
> + OFFSET(TDX_MODULE_rcx, tdx_module_output, rcx);
> + OFFSET(TDX_MODULE_rdx, tdx_module_output, rdx);
> + OFFSET(TDX_MODULE_r8, tdx_module_output, r8);
> + OFFSET(TDX_MODULE_r9, tdx_module_output, r9);
> + OFFSET(TDX_MODULE_r10, tdx_module_output, r10);
> + OFFSET(TDX_MODULE_r11, tdx_module_output, r11);

Which obviously duplicates the above part as well.

> + *-------------------------------------------------------------------------
> + * TDCALL ABI:
> + *-------------------------------------------------------------------------
> + * Input Registers:
> + *
> + * RAX - TDCALL Leaf number.
> + * RCX,RDX,R8-R9 - TDCALL Leaf specific input registers.
> + *
> + * Output Registers:
> + *
> + * RAX - TDCALL instruction error code.
> + * RCX,RDX,R8-R11 - TDCALL Leaf specific output registers.
> + *
> + *-------------------------------------------------------------------------
> + *
> + * __tdx_module_call() function ABI:
> + *
> + * @fn (RDI) - TDCALL Leaf ID, moved to RAX
> + * @rcx (RSI) - Input parameter 1, moved to RCX
> + * @rdx (RDX) - Input parameter 2, moved to RDX
> + * @r8 (RCX) - Input parameter 3, moved to R8
> + * @r9 (R8) - Input parameter 4, moved to R9
> + *
> + * @out (R9) - struct tdx_module_output pointer
> + * stored temporarily in R12 (not
> + * shared with the TDX module). It
> + * can be NULL.
> + *
> + * Return status of TDCALL via RAX.
> + */

And unsurprisingly this function and __seamcall of the other patch set
are very similar aside of the calling convention (__seamcall has a
struct for the input parameters) and the obvious difference that one
issues TDCALL and the other SEAMCALL.

So can we please have _one_ implementation and the same struct(s) for
the module call which is exactly the same for host and guest except for
the instruction used.

IOW, this begs a macro implementation

.macro TDX_MODULE_CALL host:req

....

.if \host
seamcall
.else
tdcall
.endif

....

So the actual functions become:

SYM_FUNC_START(__tdx_module_call)
FRAME_BEGIN
TDX_MODULE_CALL host=0
FRAME_END
ret
SYM_FUNC_END(__tdx_module_call)

SYM_FUNC_START(__tdx_seam_call)
FRAME_BEGIN
TDX_MODULE_CALL host=1
FRAME_END
ret
SYM_FUNC_END(__tdx_seam_call)

Hmm?

> +/*
> + * Wrapper for standard use of __tdx_hypercall with panic report
> + * for TDCALL error.
> + */
> +static inline u64 _tdx_hypercall(u64 fn, u64 r12, u64 r13, u64 r14,
> + u64 r15, struct tdx_hypercall_output
> *out)

This begs the question whether having a struct hypercall_input similar
to the way how seamcall input parameters are implemented makes more
sense than 7 function arguments. Hmm?

Thanks,

tglx