Received-SPF: pass (google.com: domain of linux-kernel+bounces-104739-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 15 Mar 2024 10:41:28 -0700
In-Reply-To: <7cfd33d896fce7b49bcf4b7179d0ded22c06b8c2.1708933498.git.isaku.yamahata@intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <cover.1708933498.git.isaku.yamahata@intel.com> <7cfd33d896fce7b49bcf4b7179d0ded22c06b8c2.1708933498.git.isaku.yamahata@intel.com>
Message-ID: <ZfSISAC0sIYXewqG@google.com>
Subject: Re: [PATCH v19 029/130] KVM: TDX: Add C wrapper functions for
 SEAMCALLs to the TDX module
From: Sean Christopherson <seanjc@google.com>
To: isaku.yamahata@intel.com
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, 
	isaku.yamahata@gmail.com, Paolo Bonzini <pbonzini@redhat.com>, erdemaktas@google.com, 
	Sagi Shahar <sagis@google.com>, Kai Huang <kai.huang@intel.com>, chen.bo@intel.com, 
	hang.yuan@intel.com, tina.zhang@intel.com, 
	Sean Christopherson <sean.j.christopherson@intel.com>, Binbin Wu <binbin.wu@linux.intel.com>, 
	Yuan Yao <yuan.yao@intel.com>
Content-Type: text/plain; charset="us-ascii"

On Mon, Feb 26, 2024, isaku.yamahata@intel.com 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().