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

[Huang, Kai]

On Wed, 2023-06-07 at 13:22 -0700, Hansen, Dave wrote:
> On 6/7/23 13:08, Sean Christopherson wrote:
> > > > > > > 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.
> > > > > TDX KVM + reboot can hit #UD.  On reboot, VMX is disabled (VMXOFF) via
> > > > > syscore.shutdown callback.  However, guest TD can be still running to issue
> > > > > SEAMCALL resulting in #UD.
> > > > > 
> > > > > Or we can postpone the change and make the TDX KVM patch series carry a patch
> > > > > for it.
> > > > How does the existing KVM use of VMLAUNCH/VMRESUME avoid that problem?
> > > extable. From arch/x86/kvm/vmx/vmenter.S
> > > 
> > > .Lvmresume:
> > >         vmresume
> > >         jmp .Lvmfail
> > > 
> > > .Lvmlaunch:
> > >         vmlaunch
> > >         jmp .Lvmfail
> > > 
> > >         _ASM_EXTABLE(.Lvmresume, .Lfixup)
> > >         _ASM_EXTABLE(.Lvmlaunch, .Lfixup)
> > More specifically, KVM eats faults on VMX and SVM instructions that occur after
> > KVM forcefully disables VMX/SVM.
> 
> <grumble> That's a *TOTALLY* different argument than the patch makes.
> 
> KVM is being a _bit_ nutty here, but I do respect it trying to honor the
> "-f".  I have no objections to the SEAMCALL code being nutty in the same
> way.
> 
> Why do I get the feeling that code is being written without
> understanding _why_, despite this being v11?

Hi Dave,

As I replied in another email, the main reason is to return an error code
instead of Oops when tdx_enable() is called mistakenly when CPU isn't in VMX
operation.  Also in this version, the machine check handler can call SEAMCALL
legally when CPU isn't in VMX operation.

I once mentioned alternatively we could check CR4.VMXE to see whether CPU is in
VMX operation but looks you preferred to use EXTTABLE.  From hardware's point of
view, checking CR4.VMXE isn't enough, although currently setting it and doing
VMXON are always done together with IRQ disabled.

https://lore.kernel.org/lkml/cover.1655894131.git.kai.huang@xxxxxxxxx/T/#m6e5673a191254bf36f48083cd215f7ff8f2b315b

How about I add below to the changelog?

"
The current TDX_MODULE_CALL macro handles neither #GP nor #UD.  The kernel would
hit Oops if SEAMCALL were mistakenly made when TDX is enabled by the BIOS or
when CPU isn't in VMX operation.  For the former, the callers could check
platform_tdx_enabled() first, although that doesn't rule out the buggy BIOS in
which case the kernel could still get Oops.  For the latter, the caller could
check CR4.VMXE based on the fact that currently setting this bit and doing VMXON
are done together when IRQ is disabled, although from hardware's perspective
checking CR4.VMXE isn't enough.

However this could be problematic if SEAMCALL is called in the cases such as
exception handler, NMI handler, etc, as disabling IRQ doesn't prevent any of
them from happening.

To have a clean solution, just make the SEAMCALL always return error code by
using EXTTABLE so the SEAMCALL can be safely called in any context.  A later
patch will need to use SEAMCALL in the machine check handler.  There might be
such use cases in the future too.
"