Re: [PATCHv6 02/30] x86/tdx: Provide common base for SEAMCALL and TDCALL C wrappers

Thomas Gleixner
2022-03-16T23:34:05Z

Kirrill,

On Wed, Mar 16 2022 at 05:08, Kirill A. Shutemov wrote:
> Define an assembly macro that can be used to implement C wrapper for
> both TDCALL and SEAMCALL.
>

> TDCALL wrapper will be implemented using the macro later in the series.
> SEAMCALL wrapper is out-of-scope for the series and will be implemented
> as part of TDX host enabling.

This paragraph makes no sense in the changelog once it's merged. That's
a reviewer/maintainer info which wants to be below the ---

> +/*
> + * TDX_MODULE_CALL - common helper macro for both
> + * TDCALL and SEAMCALL instructions.
> + *
> + * TDCALL - used by TDX guests to make requests to the
> + * TDX module and hypercalls to the VMM.
> + * SEAMCALL - used by TDX hosts to make requests to the
> + * TDX module.
> + *
> + * Both instruction are supported in Binutils >= 2.36.
> + */
> +#define tdcall .byte 0x66,0x0f,0x01,0xcc
> +#define seamcall .byte 0x66,0x0f,0x01,0xcf

Bah. I really hate this #define glue. The comment documents
TDX_MODULE_CALL. The defines for the instructions are a seperate
problem. So please move them above the whole thing with their own
comment.

> +.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 */
> +
> + .if \host
> + seamcall
> + /*
> + * 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 TDX_SEAMCALL_VMFAILINVALID for VMfailInvalid.
> + * This value will never be used as actual SEAMCALL error code as
> + * it is from the Reserved status code class.
> + */
> + jnc .Lno_vmfailinvalid
> + mov $TDX_SEAMCALL_VMFAILINVALID, %rax
> +.Lno_vmfailinvalid:

Please add a new line between the label and the .else for readability sake.

> + .else
> + tdcall
> + .endif
> +
> + /*
> + * Fetch output pointer from stack to R12 (It is used
> + * as temporary storage)
> + */
> + pop %r12
> +
> + /*
> + * Since this function can be initiated without an output pointer,
> + * check if caller provided an output struct before storing output
> + * registers.

The function is a macro. It's not initiated, it's invoked. It always has
an output pointer in R12, but that can be NULL.

With those nitpicks addressed:

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>