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. "
