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.
