Messages in this thread | | | From | Thomas Gleixner <> | Subject | Re: [PATCHv6 02/30] x86/tdx: Provide common base for SEAMCALL and TDCALL C wrappers | Date | Thu, 17 Mar 2022 00:33:58 +0100 |
| |
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>
| |