Re: [PATCH v19 029/130] KVM: TDX: Add C wrapper functions for SEAMCALLs to the TDX module

[Isaku Yamahata]

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>