Re: [PATCH v7 05/20] x86/virt/tdx: Implement functions to make SEAMCALL

[Huang, Kai]

On Tue, 2022-11-22 at 10:20 -0800, Dave Hansen wrote:
> On 11/20/22 16:26, Kai Huang wrote:
> > TDX introduces a new CPU mode: Secure Arbitration Mode (SEAM).  This
> > mode runs only the TDX module itself or other code to load the TDX
> > module.
> > 
> > The host kernel communicates with SEAM software via a new SEAMCALL
> > instruction.  This is conceptually similar to a guest->host hypercall,
> > except it is made from the host to SEAM software instead.
> > 
> > The TDX module defines a set of SEAMCALL leaf functions to allow the
> > host to initialize it, and to create and run protected VMs.  SEAMCALL
> > leaf functions use an ABI different from the x86-64 system-v ABI.
> > Instead, they share the same ABI with the TDCALL leaf functions.
> 
> I may have suggested this along the way, but the mention of the sysv ABI
> is just confusing here.  This is enough for a changelog:
> 
> 	The TDX module establishes a new SEAMCALL ABI which allows the
> 	host to initialize the module and to and to manage VMs.
> 
> Kill the rest.

Thanks will do.

> 
> > Implement a function __seamcall() to allow the host to make SEAMCALL
> > to SEAM software using the TDX_MODULE_CALL macro which is the common
> > assembly for both SEAMCALL and TDCALL.
> 
> In general, I dislike mentioning function names in changelogs.  Keep
> this high-level, like:
> 
> 	Add infrastructure to make SEAMCALLs.  The SEAMCALL ABI is very
> 	similar to the TDCALL ABI and leverages much TDCALL
> 	infrastructure.

Will do.

> 
> > SEAMCALL instruction causes #GP when SEAMRR isn't enabled, and #UD when
> > CPU is not in VMX operation.  The current TDX_MODULE_CALL macro doesn't
> > handle any of them.  There's no way to check whether the CPU is in VMX
> > operation or not.
> 
> What is SEAMRR?

Sorry it is a leftover.  Should be "when TDX isn't enabled".

> 
> Why even mention this behavior in the changelog.  Is this a problem?
> Does it have a solution?

My intention was to provide some background information why to extend
TDX_MODULE_CALL macro to handle #UD and #GP as mentioned below.

> 
> > Initializing the TDX module is done at runtime on demand, and it depends
> > on the caller to ensure CPU is in VMX operation before making SEAMCALL.
> > To avoid getting Oops when the caller mistakenly tries to initialize the
> > TDX module when CPU is not in VMX operation, extend the TDX_MODULE_CALL
> > macro to handle #UD (and also #GP, which can theoretically still happen
> > when TDX isn't actually enabled by the BIOS, i.e. due to BIOS bug).
> 
> I'm not completely sure this is worth it.  If the BIOS lies, we oops.
> There are lots of ways that the BIOS lying can make the kernel oops.
> What's one more?

I agree.  But if we want to handle #UD, then #GP won't cause oops any more, so I
just added error code for #GP too.

Or perhaps we can change to below: ?

"... extend the TDX_MODULE_CALL to handle #UD (and opportunistically #GP since
they share the same assembly)."

Or other suggestions?

> 
> > Introduce two new TDX error codes for #UD and #GP respectively so the
> > caller can distinguish.  Also, Opportunistically put the new TDX error
> > codes and the existing TDX_SEAMCALL_VMFAILINVALID into INTEL_TDX_HOST
> > Kconfig option as they are only used when it is on.
> > 
> > As __seamcall() can potentially return multiple error codes, besides the
> > actual SEAMCALL leaf function return code, also introduce a wrapper
> > function seamcall() to convert the __seamcall() error code to the kernel
> > error code, so the caller doesn't need to duplicate the code to check
> > return value of __seamcall() and return kernel error code accordingly.
> 
> 

[...]

> > +/*
> > + * Wrapper of __seamcall() to convert SEAMCALL leaf function error code
> > + * to kernel error code.  @seamcall_ret and @out contain the SEAMCALL
> > + * leaf function return code and the additional output respectively if
> > + * not NULL.
> > + */
> > +static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
> > +				    u64 *seamcall_ret,
> > +				    struct tdx_module_output *out)
> > +{
> > +	u64 sret;
> > +
> > +	sret = __seamcall(fn, rcx, rdx, r8, r9, out);
> > +
> > +	/* Save SEAMCALL return code if caller wants it */
> > +	if (seamcall_ret)
> > +		*seamcall_ret = sret;
> > +
> > +	/* SEAMCALL was successful */
> > +	if (!sret)
> > +		return 0;
> > +
> > +	switch (sret) {
> > +	case TDX_SEAMCALL_GP:
> > +		/*
> > +		 * platform_tdx_enabled() is checked to be true
> > +		 * before making any SEAMCALL.
> > +		 */
> 
> This doesn't make any sense.  "platform_tdx_enabled() is checked"???
> 
> Do you mean that it *should* be checked and probably wasn't which is
> what caused the error?

I meant tdx_enable() already calls platform_tdx_enabled() to check whether BIOS
has enabled TDX at the very beginning before making any SEAMCALL, so
theoretically #GP should not happen unless there's BIOS bug.  I thought a WARN()
can help to catch.

> 
> > +		WARN_ON_ONCE(1);
> > +		fallthrough;
> > +	case TDX_SEAMCALL_VMFAILINVALID:
> > +		/* Return -ENODEV if the TDX module is not loaded. */
> > +		return -ENODEV;
> 
> Pro tip: you don't need to rewrite code in comments.  If the code
> literally says, "return -ENODEV", there is very little value in writing
> virtually identical bytes "Return -ENODEV" in the comment.
> 

Indeed.  Thanks for the tip!  I'll update those comments.