On Fri, Mar 15, 2024 at 10:41:28AM -0700,
Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> On Mon, Feb 26, 2024, isaku.yamahata@xxxxxxxxx wrote:
> > +static inline u64 tdx_seamcall(u64 op, struct tdx_module_args *in,
> > + struct tdx_module_args *out)
> > +{
> > + u64 ret;
> > +
> > + if (out) {
> > + *out = *in;
> > + ret = seamcall_ret(op, out);
> > + } else
> > + ret = seamcall(op, in);
> > +
> > + if (unlikely(ret == TDX_SEAMCALL_UD)) {
> > + /*
> > + * SEAMCALLs fail with TDX_SEAMCALL_UD returned when VMX is off.
> > + * This can happen when the host gets rebooted or live
> > + * updated. In this case, the instruction execution is ignored
> > + * as KVM is shut down, so the error code is suppressed. Other
> > + * than this, the error is unexpected and the execution can't
> > + * continue as the TDX features reply on VMX to be on.
> > + */
> > + kvm_spurious_fault();
> > + return 0;
>
> This is nonsensical. The reason KVM liberally uses BUG_ON(!kvm_rebooting) is
> because it *greatly* simpifies the overall code by obviating the need for KVM to
> check for errors that should never happen in practice. On, and
>
> But KVM quite obviously needs to check the return code for all SEAMCALLs, and
> the SEAMCALLs are (a) wrapped in functions and (b) preserve host state, i.e. we
> don't need to worry about KVM consuming garbage or running with unknown hardware
> state because something like INVVPID or INVEPT faulted.
>
> Oh, and the other critical aspect of all of this is that unlike VMREAD, VMWRITE,
> etc., SEAMCALLs almost always require a TDR or TDVPR, i.e. need a VM or vCPU.
> Now that we've abandoned the macro shenanigans that allowed things like
> tdh_mem_page_add() to be pure translators to their respective SEAMCALL, I don't
> see any reason to take the physical addresses of the TDR/TDVPR in the helpers.
>
> I.e. if we do:
>
> u64 tdh_mng_addcx(struct kvm *kvm, hpa_t addr)
>
> then the intermediate wrapper to the SEAMCALL assembly has the vCPU or VM and
> thus can precisely terminate the one problematic VM.
>
> So unless I'm missing something, I think that kvm_spurious_fault() should be
> persona non grata for TDX, and that KVM should instead use KVM_BUG_ON().
Thank you for the feedback. As I don't see any issues to do so, I'll convert
those wrappers to take struct kvm_tdx or struct vcpu_tdx, and eliminate
kvm_spurious_fault() in favor of KVM_BUG_ON().
--
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
