Re: [PATCH v12 05/22] x86/virt/tdx: Add SEAMCALL infrastructure

[Huang, Kai]

On Tue, 2023-06-27 at 12:48 +0300, kirill.shutemov@xxxxxxxxxxxxxxx wrote:
> On Tue, Jun 27, 2023 at 02:12:35AM +1200, Kai Huang wrote:
> > +/*
> > + * 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;
> > +	int cpu;
> > +
> > +	/* Need a stable CPU id for printing error message */
> > +	cpu = get_cpu();
> > +	sret = __seamcall(fn, rcx, rdx, r8, r9, out);
> > +	put_cpu();
> > +
> > +	/* Save SEAMCALL return code if the caller wants it */
> > +	if (seamcall_ret)
> > +		*seamcall_ret = sret;
> > +
> > +	switch (sret) {
> > +	case 0:
> > +		/* SEAMCALL was successful */
> > +		return 0;
> > +	case TDX_SEAMCALL_VMFAILINVALID:
> > +		pr_err_once("module is not loaded.\n");
> > +		return -ENODEV;
> > +	default:
> > +		pr_err_once("SEAMCALL failed: CPU %d: leaf %llu, error 0x%llx.\n",
> > +				cpu, fn, sret);
> > +		if (out)
> > +			pr_err_once("additional output: rcx 0x%llx, rdx 0x%llx, r8 0x%llx, r9 0x%llx, r10 0x%llx, r11 0x%llx.\n",
> > +					out->rcx, out->rdx, out->r8,
> > +					out->r9, out->r10, out->r11);
> 
> This look excessively noisy.
> 
> Don't we have SEAMCALL leafs that can fail in normal situation? Like
> TDX_OPERAND_BUSY error code that indicate that operation likely will
> succeed on retry.

For TDX module initialization TDX_OPERAND_BUSY cannot happen.  KVM may have
legal cases that BUSY can happen, e.g., KVM's TDP MMU supports handling faults
concurrently on different cpus, but that is still under discussion.  Also KVM
tends to use __seamcall() directly:

https://lore.kernel.org/lkml/3c2c142e14a04a833b47f77faecaa91899b472cd.1678643052.git.isaku.yamahata@xxxxxxxxx/

I guess KVM doesn't want to print message in all cases as you said, but for
module initialization is fine.  Those error messages are useful in case
something goes wrong, and printing them in seamcall() helps to reduce the code
to print in all callers.

> 
> Or is that wrapper only used for never-fail SEAMCALLs? If so, please
> document it.
> 

How about adding below?

	Use __seamcall() directly in cases that printing error message isn't
	desired, e.g., when SEAMCALL can legally fail with BUSY and the caller
	wants to retry.