Re: [PATCH v8 11/11] x86/tdx: Handle CPUID via #VE

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Thu, Oct 07, 2021 at 07:25:44PM -0700, Kuppuswamy, Sathyanarayanan wrote:
> > - _tdx_halt() warns and doesn't return anything.
> 
> Since tdx_halt handler is shared with pv_ops, we can't return anything
> back (so we use WARN_ON to report the error).

Saying the "current structure of the code doesn't allow it" is not a
valid reason.

You could for example have _tdx_halt() return an error, and have the #VE
handler call _tdx_halt() directly so it can receive the error.

> > - tdx_read_msr_safe() and tdx_write_msr_safe() convert all errors to -EIO.
> 
> Return value does not matter. we only check for zero/non-zero value in
> tdx_handle_virtualization_exception(). we have used -EIO to convey that it is
> an IO error.

If I'm reading the GHCI spec right, the only possible error code for msr
access is VMCALL_INVALID_OPERAND.  Which sounds like -EINVAL to me.

> > - tdx_handle_cpuid() returns the raw vmcall error.
> > 
> > Second, as far as I can tell, the #VE handler doesn't check the actual
> > return code value, other than checking for non-zero.  Should it at least
> > be printed in a warning?
> 
> I don't think this is required. We can use trace to check the error code
> or argument details in failure case. Since we don't really use the
> error value, I am planning to change the #VE handler return type to
> bool.

In general the kernel prefers errno values instead of bool.  If
hard-coding an error, it should probably be hard-coded to a valid kernel
errno from within the inner handler (_tdx_halt(), tdx_read_msr_safe(),
etc).

That prevents confusion and will give the freedom to add more return
codes in the future, in case the inner handler wants to eventually
report multiple error types, or the top-level handler wants to handle
error types differently, or trace them, or warn, etc.

-- 
Josh