On Thu, 2024-04-25 at 15:43 -0700, Sean Christopherson wrote: > On Thu, Apr 25, 2024, Kai Huang wrote: > > On Thu, 2024-04-25 at 09:30 -0700, Sean Christopherson wrote: > > > On Tue, Apr 23, 2024, Kai Huang wrote: > > > 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? > > Correct, the #UD is likely not (immediately) fatal. > > > > 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. > > It's not about making the kernel safer, it's about helping triage/debug issues. > > > 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. > > Eh, I wouldn't worry about that too much. The only reason I brought up that > check was to call out that we can't *know* with 100% certainty that SEAMCALL > failed due to the CPU not being post-VMXON. OK (though I think we can rule out other cases by adding more checks etc). > > > > 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. > > Oh, I wasn't thinking that we'd check CR4.VMXE before *every* SEAMCALL, just > before the TDH.SYS.LP.INIT call, i.e. before the one that is most likely to fail > due to a software bug that results in the CPU not doing VMXON before enabling > TDX. > > Again, my intent is to add a simple, cheap, and targeted sanity check to help > deal with potential failures in code that historically has been less than rock > solid, and in function that has a big fat assumption that the caller has done > VMXON on the CPU. I see. (To be fair, personally I don't recall that we ever had any bug due to "cpu not in post-VMXON before SEAMCALL", but maybe it's just me. :-).) But if tdx_enable() doesn't call tdx_cpu_enable() internally, then we will have two functions need to handle. For tdx_enable(), given it's still good idea to disable CPU hotplug around it, we can still do some check for all online cpus at the beginning, like: on_each_cpu(check_cr4_vmx(), &err, 1); Btw, please also see my last reply to Chao why I don't like calling tdx_cpu_enable() inside tdx_enable(): https://lore.kernel.org/lkml/1fd17c931d5c2effcf1105b63deac8e3fb1664bc.camel@xxxxxxxxx/ That being said, I can try to add additional patch(es) to do CR4.VMX check if you want, but personally I found hard to have a strong justification to do so.
