Re: [PATCH v11 05/20] x86/virt/tdx: Add SEAMCALL infrastructure

["Huang, Kai" <kai.huang@intel.com>]

On Wed, 2023-06-07 at 07:24 -0700, Hansen, Dave wrote:
> On 6/4/23 07:27, 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 establishes a new SEAMCALL ABI which allows the host to
> > initialize the module and to manage VMs.
> > 
> > Add infrastructure to make SEAMCALLs.  The SEAMCALL ABI is very similar
> > to the TDCALL ABI and leverages much TDCALL infrastructure.
> > 
> > SEAMCALL instruction causes #GP when TDX isn't BIOS enabled, and #UD
> > when CPU is not in VMX operation.  Currently, only KVM code mocks with
> 
> "mocks"?  Did you mean "mucks"?

Yes "mucks".  I believe I made some mistake.

> 
> > VMX enabling, and KVM is the only user of TDX.  This implementation
> > chooses to make KVM itself responsible for enabling VMX before using
> > TDX and let the rest of the kernel stay blissfully unaware of VMX.
> > 
> > The current TDX_MODULE_CALL macro handles neither #GP nor #UD.  The
> > kernel would hit Oops if SEAMCALL were mistakenly made w/o enabling VMX
> > first.  Architecturally, there is no CPU flag to check whether the CPU
> > is in VMX operation.  Also, if a BIOS were buggy, it could still report
> > valid TDX private KeyIDs when TDX actually couldn't be enabled.
> 
> I'm not sure this is a great justification.  If the BIOS is lying to the
> OS, we _should_ oops.
> 
> How else can this happen other than silly kernel bugs.  It's OK to oops
> in the face of silly kernel bugs.

Agreed.  And I'll just remove that sentence if you agree with below ...

[...]

> > +	/*
> > +	 * SEAMCALL caused #GP or #UD.  By reaching here %eax contains
> > +	 * the trap number.  Convert the trap number to the TDX error
> > +	 * code by setting TDX_SW_ERROR to the high 32-bits of %rax.
> > +	 *
> > +	 * Note cannot OR TDX_SW_ERROR directly to %rax as OR instruction
> > +	 * only accepts 32-bit immediate at most.
> > +	 */
> > +	mov $TDX_SW_ERROR, %r12
> > +	orq %r12, %rax
> 
> I think the justification for doing the #UD/#GP handling is a bit weak.
> In the end, it gets us a nicer error message.  Is that error message
> *REALLY* needed?  Or is an oops OK in the very rare circumstance that
> the BIOS is totally buggy?

...

It's not just for the "BIOS buggy" case.  The main purpose is to give an error
message when the caller mistakenly calls tdx_enable().

Also, now the machine check handler improvement patch also calls SEAMCALL to get
a given page's page type.  It's totally legal that a machine check happens when
the CPU isn't in VMX operation (e.g. KVM isn't loaded), and in fact we use the
SEAMCALL return value to detect whether CPU is in VMX operation and handles such
case accordingly.