Received-SPF: pass (google.com: domain of linux-kernel+bounces-169983-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 6 May 2024 07:21:57 -0700
In-Reply-To: <ZjiE+O9fct5zI4Sf@chao-email>
Precedence: bulk
Mime-Version: 1.0
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <f05b978021522d70a259472337e0b53658d47636.1708933498.git.isaku.yamahata@intel.com>
 <Zgoz0sizgEZhnQ98@chao-email> <20240403184216.GJ2444378@ls.amr.corp.intel.com>
 <43cbaf90-7af3-4742-97b7-2ea587b16174@intel.com> <20240501155620.GA13783@ls.amr.corp.intel.com>
 <399cec29-ddf4-4dd5-a34b-ffec72cbfa26@intel.com> <ZjiE+O9fct5zI4Sf@chao-email>
Message-ID: <ZjjnhfI1vvhdRWGK@google.com>
Subject: Re: [PATCH v19 101/130] KVM: TDX: handle ept violation/misconfig exit
From: Sean Christopherson <seanjc@google.com>
To: Chao Gao <chao.gao@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>, Isaku Yamahata <isaku.yamahata@intel.com>, 
	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, isaku.yamahata@linux.intel.com
Content-Type: text/plain; charset="us-ascii"

On Mon, May 06, 2024, Chao Gao wrote:
> On Wed, May 01, 2024 at 09:54:07AM -0700, Reinette Chatre wrote:
> >diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> >index 499c6cd9633f..ba81e6f68c97 100644
> >--- a/arch/x86/kvm/vmx/tdx.c
> >+++ b/arch/x86/kvm/vmx/tdx.c
> >@@ -1305,11 +1305,20 @@ static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
> > 	} else {
> > 		exit_qual = tdexit_exit_qual(vcpu);
> > 		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
> >+			union tdx_exit_reason exit_reason = to_tdx(vcpu)->exit_reason;
> >+
> >+			/*
> >+			 * Instruction fetch in TD from shared memory
> >+			 * causes a #PF.
> >+			 */
> 
> It is better to hoist this comment above the if-statement.
> 
> 		/*
> 		 * Instruction fetch in TD from shared memory never causes EPT
> 		 * violation. Warn if such an EPT violation occurs as the CPU
> 		 * probably is buggy.
> 		 */
>  		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
> 		...
> 		}
> 
> 
> but, I am wondering why KVM needs to perform this check. I prefer to drop this
> check if KVM doesn't rely on it to handle EPT violation correctly.
> 
> > 			pr_warn("kvm: TDX instr fetch to shared GPA = 0x%lx @ RIP = 0x%lx\n",
> > 				tdexit_gpa(vcpu), kvm_rip_read(vcpu));
> 
> how about using vcpu_unimpl()? it is a wrapper for printing such a message.

This isn't an "unimplemented" scenario in KVM, this is a "hardware is broken"
scenario.  Just WARN_ON_ONCE() and move on.  Or I suppose since EPT misconfig
needs to do something as well:

		if (KVM_BUG_ON(exit_qual & EPT_VIOLATION_ACC_INSTR, vcpu->kvm))
			return -EIO;