Re: [PATCHv5.1 04/30] x86/tdx: Extend the confidential computing API to support TDX guests

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

On 3/9/22 08:01, Kirill A. Shutemov wrote:
> +/*
> + * Wrapper for __tdx_module_call() for cases when the call doesn't suppose to
> + * fail. Panic if the call fails.
> + */
> +static inline void tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
> +				   struct tdx_module_output *out)
> +{
> +	if (__tdx_module_call(fn, rcx, rdx, r8, r9, out))
> +		panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
> +}

That comment didn't do much for me.  I know it's a wrapper.  I know it
panics() if the call returns a failure.  That's what the code *does*.  I
want a comment to tell me *why* it does that.

I _think_ I may have been getting this confused with the TDVMCALL mechanism.

All TDVMCALLs that return with rax==0 are fatal, we jump right to a ud2
instruction.  A __tdx_module_call() (via TDCALL) with rax==0 doesn't
*have* to be fatal.  But, this establishes a policy that all TDCALLs via
tdx_module_call() *ARE* fatal.

How about this for a comment?

/*
 * Used for TDX guests to make calls directly to the TD module.  This
 * should only be used for calls that have no legitimate reason to fail
 * or where the kernel can not survive the call failing.
 */

That tells me a *LOT*: This is a guest -> TD module thing.  Not a host
thing, not a hypercall.  And, no the naming isn't good enough to tell me
that.  Also, it give me advice.  It tells me when I should use this
function.  If it look at the call site, it even makes sense.  A guest
can't even build a sane PTE without this call succeeding.  If *COURSE*
we panic() if the call fails.

You could even call this information out in the comment in get_info():


...
	 * The GPA width that comes out of this call is critical.  TDX
	 * guests can not meaningfully run without it.
	 */


Then it all kinda fits together.  Oh, this panic() is awfully harsh.
Oh, it's only supposed to be used for important things that the guest
really needs.  Then there's a comment about why it needs it so badly.

Otherwise the panic() just looks superfluous and mean.