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

[Dave Hansen <dave.hansen@intel.com>]

...
>  arch/x86/include/asm/tdx.h    | 20 ++++++++
>  arch/x86/kernel/asm-offsets.c |  9 ++++
>  arch/x86/virt/tdxcall.S       | 91 +++++++++++++++++++++++++++++++++++
>  3 files changed, 120 insertions(+)
>  create mode 100644 arch/x86/virt/tdxcall.S
> 
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index ba8042ce61c2..2f8cb1e53e77 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -8,6 +8,25 @@
>  #define TDX_CPUID_LEAF_ID	0x21
>  #define TDX_IDENT		"IntelTDX    "
>  
> +#define TDX_SEAMCALL_VMFAILINVALID     0x8000FF00FFFF0000ULL

This needs a comment.  It's a completely software-defined value, right?

> +#ifndef __ASSEMBLY__
> +
> +/*
> + * Used to gather the output registers values of the TDCALL and SEAMCALL
> + * instructions 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;
> +};
> +
>  #ifdef CONFIG_INTEL_TDX_GUEST
>  
>  void __init tdx_early_init(void);
> @@ -18,4 +37,5 @@ static inline void tdx_early_init(void) { };
>  
>  #endif /* CONFIG_INTEL_TDX_GUEST */
>  
> +#endif /* !__ASSEMBLY__ */
>  #endif /* _ASM_X86_TDX_H */
> diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
> index 9fb0a2f8b62a..7dca52f5cfc6 100644
> --- a/arch/x86/kernel/asm-offsets.c
> +++ b/arch/x86/kernel/asm-offsets.c
> @@ -18,6 +18,7 @@
>  #include <asm/bootparam.h>
>  #include <asm/suspend.h>
>  #include <asm/tlbflush.h>
> +#include <asm/tdx.h>
>  
>  #ifdef CONFIG_XEN
>  #include <xen/interface/xen.h>
> @@ -65,6 +66,14 @@ static void __used common(void)
>  	OFFSET(XEN_vcpu_info_arch_cr2, vcpu_info, arch.cr2);
>  #endif
>  
> +	BLANK();
> +	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);
> +
>  	BLANK();
>  	OFFSET(BP_scratch, boot_params, scratch);
>  	OFFSET(BP_secure_boot, boot_params, secure_boot);
> diff --git a/arch/x86/virt/tdxcall.S b/arch/x86/virt/tdxcall.S
> new file mode 100644
> index 000000000000..90569faedacc
> --- /dev/null
> +++ b/arch/x86/virt/tdxcall.S
> @@ -0,0 +1,91 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#include <asm/asm-offsets.h>
> +#include <asm/tdx.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.

		^ uses??


> + * They are supported in Binutils >= 2.36.
> + */

While you're in there, could we make this a little more structured, please?

/*
 * 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
> +
> +.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.
> +	 */

Why can TDX_SEAMCALL_VMFAILINVALID never be used as an actual SEAMCALL
error code?  Would it be more accurate to say that today's P-SEAMLDR and
TDX module never use TDX_SEAMCALL_VMFAILINVALID as an actual SEAMCALL
error code?

There's an important distinction there.  It's possible that the SEAMCALL
instruction architecture says or implies that it can never have
TDX_SEAMCALL_VMFAILINVALID as a return value.  It's also possible that
this "never" comes from a guess about what P-SEAMLDRs or TDX modules can do.

> +	jnc .Lno_vmfailinvalid
> +	mov $TDX_SEAMCALL_VMFAILINVALID, %rax
> +.Lno_vmfailinvalid:
> +	.else
> +	tdcall
> +	.endif
> +
> +	/*
> +	 * Fetch output pointer from stack to R12 (It is used
> +	 * as temporary storage)
> +	 */
> +	pop %r12
> +
> +	/* Check for success: 0 - Successful, otherwise failed */
> +	test %rax, %rax
> +	jnz .Lno_output_struct
> +
> +	/*
> +	 * Since this function can be initiated without an output pointer,
> +	 * check if caller provided an output struct before storing
> +	 * output registers.
> +	 */
> +	test %r12, %r12
> +	jz .Lno_output_struct
> +
> +	/* Copy result registers to output struct: */
> +	movq %rcx, TDX_MODULE_rcx(%r12)
> +	movq %rdx, TDX_MODULE_rdx(%r12)
> +	movq %r8,  TDX_MODULE_r8(%r12)
> +	movq %r9,  TDX_MODULE_r9(%r12)
> +	movq %r10, TDX_MODULE_r10(%r12)
> +	movq %r11, TDX_MODULE_r11(%r12)
> +
> +.Lno_output_struct:
> +	/* Restore the state of R12 register */
> +	pop %r12
> +.endm

That doesn't look too bad.  Please just send an updated version with the
revised comments.