On Thu, 2024-04-25 at 09:30 -0700, Sean Christopherson wrote: > On Tue, Apr 23, 2024, Kai Huang wrote: > > On Tue, 2024-04-23 at 22:59 +0000, Huang, Kai wrote: > > > > Right, but that doesn't say why the #UD occurred. The macro dresses it up in > > > > TDX_SW_ERROR so that KVM only needs a single parser, but at the end of the day > > > > KVM is still only going to see that SEAMCALL hit a #UD. > > > > > > Right. But is there any problem here? I thought the point was we can > > > just use the error code to tell what went wrong. > > > > Oh, I guess I was replying too quickly. From the spec, #UD happens when > > > > IF not in VMX operation or inSMM or inSEAM or > > ((IA32_EFER.LMA & CS.L) == 0) > > THEN #UD; > > > > Are you worried about #UD was caused by other cases rather than "not in > > VMX operation"? > > Yes. > > > But it's quite obvious the other 3 cases are not possible, correct? > > The spec I'm looking at also has: > > If IA32_VMX_PROCBASED_CTLS3[5] is 0. Ah, now I see this too. It's not in the pseudo code of SEAMCALL instruction, but is at the "64-Bit Mode Exceptions" section which is after the pseudo code. And this bit 5 is to report the capability to allow to control the "shard bit" in the 5-level EPT. > > And anecdotally, I know of at least one crash in our production environment where > a VMX instruction hit a seemingly spurious #UD, i.e. it's not impossible for a > ucode bug or hardware defect to cause problems. That's obviously _extremely_ > unlikely, but that's why I emphasized that sanity checking CR4.VMXE is cheap. Yeah I agree it could happen although very unlikely. But just to be sure: I believe the #UD itself doesn't crash the kernel/machine, but should be the kernel unable to handle #UD in such case? If so, I am not sure whether the CR4.VMX check can make the kernel any safer, because we can already handle the #UD for the SEAMCALL instruction. Yeah we can clearly dump message saying "CPU isn't in VMX operation" and return failure if we have the check, but if we don't, the worst situation is we might mistakenly report "CPU isn't in VMX operation" (currently code just treats #UD as CPU not in VMX operation) when CPU doesn't IA32_VMX_PROCBASED_CTLS3[5]. And for the IA32_VMX_PROCBASED_CTLS3[5] we can easily do some pre-check in KVM code during module loading to rule out this case. And in practice, I even believe the BIOS cannot turn on TDX if the IA32_VMX_PROCBASED_CTLS3[5] is not supported. I can check on this. > Practically speaking it costs nothing, so IMO it's worth adding even if the odds > of it ever being helpful are one-in-and-million. I think we will need to do below at somewhere for the common SEAMCALL function: unsigned long flags; int ret = -EINVAL; local_irq_save(flags); if (WARN_ON_ONCE(!(__read_cr4() & X86_CR4_VMXE))) goto out; ret = seamcall(); out: local_irq_restore(flags); return ret; to make it IRQ safe. And the odd is currently the common SEAMCALL functions, a.k.a, __seamcall() and seamcall() (the latter is a mocro actually), both return u64, so if we want to have such CR4.VMX check code in the common code, we need to invent a new error code for it. That being said, although I agree it can make the code a little bit clearer, I am not sure whether it can make the code any safer -- even w/o it, the worst case is to incorrectly report "CPU is not in VMX operation", but shouldn't crash kernel etc.
