Received-SPF: pass (google.com: domain of linux-kernel+bounces-132256-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 4 Apr 2024 16:25:37 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Chao Gao <chao.gao@intel.com>, 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
Subject: Re: [PATCH v19 108/130] KVM: TDX: Handle TDX PV HLT hypercall
Message-ID: <20240404232537.GV2444378@ls.amr.corp.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <c083430632ba9e80abd09bccd5609fb3cd9d9c63.1708933498.git.isaku.yamahata@intel.com>
 <ZgzMH3944ZaBx8B3@chao-email>
 <Zg1seIaTmM94IyR8@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <Zg1seIaTmM94IyR8@google.com>

On Wed, Apr 03, 2024 at 07:49:28AM -0700,
Sean Christopherson <seanjc@google.com> wrote:

> On Wed, Apr 03, 2024, Chao Gao wrote:
> > On Mon, Feb 26, 2024 at 12:26:50AM -0800, isaku.yamahata@intel.com wrote:
> > >From: Isaku Yamahata <isaku.yamahata@intel.com>
> > >
> > >Wire up TDX PV HLT hypercall to the KVM backend function.
> > >
> > >Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > >---
> > >v19:
> > >- move tdvps_state_non_arch_check() to this patch
> > >
> > >v18:
> > >- drop buggy_hlt_workaround and use TDH.VP.RD(TD_VCPU_STATE_DETAILS)
> > >
> > >Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > >---
> > > arch/x86/kvm/vmx/tdx.c | 26 +++++++++++++++++++++++++-
> > > arch/x86/kvm/vmx/tdx.h |  4 ++++
> > > 2 files changed, 29 insertions(+), 1 deletion(-)
> > >
> > >diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > >index eb68d6c148b6..a2caf2ae838c 100644
> > >--- a/arch/x86/kvm/vmx/tdx.c
> > >+++ b/arch/x86/kvm/vmx/tdx.c
> > >@@ -688,7 +688,18 @@ void tdx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > > 
> > > bool tdx_protected_apic_has_interrupt(struct kvm_vcpu *vcpu)
> > > {
> > >-	return pi_has_pending_interrupt(vcpu);
> > >+	bool ret = pi_has_pending_interrupt(vcpu);
> > 
> > Maybe
> > 	bool has_pending_interrupt = pi_has_pending_interrupt(vcpu);
> > 
> > "ret" isn't a good name. or even call pi_has_pending_interrupt() directly in
> > the if statement below.
> 
> Ya, or split the if-statement into multiple chucks, with comments explaining
> what each non-intuitive chunk is doing.  The pi_has_pending_interrupt(vcpu) check
> is self-explanatory, the halted thing, not so much.  They are terminal statements,
> there's zero reason to pre-check the PID.
> 
> E.g.
> 
> 	/*
> 	 * Comment explaining why KVM needs to assume a non-halted vCPU has a
> 	 * pending interrupt (KVM can't see RFLAGS.IF).
> 	 */
> 	if (vcpu->arch.mp_state != KVM_MP_STATE_HALTED)
> 		return true;
> 
> 	if (pi_has_pending_interrupt(vcpu))
> 		return;
> 
> > >+	union tdx_vcpu_state_details details;
> > >+	struct vcpu_tdx *tdx = to_tdx(vcpu);
> > >+
> > >+	if (ret || vcpu->arch.mp_state != KVM_MP_STATE_HALTED)
> > >+		return true;
> > 
> > Question: why mp_state matters here?
> > >+
> > >+	if (tdx->interrupt_disabled_hlt)
> > >+		return false;
> > 
> > Shouldn't we move this into vt_interrupt_allowed()? VMX calls the function to
> > check if interrupt is disabled.

Chao, are you suggesting to implement tdx_interrupt_allowed() as
"EXIT_REASON_HLT && a0" instead of "return true"?
I don't think it makes sense because it's rare case and we can't avoid spurious
wakeup for TDX case.


> >KVM can clear tdx->interrupt_disabled_hlt on
> > every TD-enter and set it only on TD-exit due to the guest making a
> > TDVMCALL(hlt) w/ interrupt disabled.
> 
> I'm pretty sure interrupt_disabled_hlt shouldn't exist, should "a0", a.k.a. r12,
> be preserved at this point?
> 
> 	/* Another comment explaning magic code. */
> 	if (to_vmx(vcpu)->exit_reason.basic == EXIT_REASON_HLT &&
> 	    tdvmcall_a0_read(vcpu))
> 		return false;
> 
> 
> Actually, can't this all be:
> 
> 	if (to_vmx(vcpu)->exit_reason.basic != EXIT_REASON_HLT)
> 		return true;
> 
> 	if (!tdvmcall_a0_read(vcpu))
> 		return false;
> 
> 	if (pi_has_pending_interrupt(vcpu))
> 		return true;
> 
> 	return tdx_has_pending_virtual_interrupt(vcpu);
> 

Thanks for the suggestion.  This is much cleaner.  Will update the function.
-- 
Isaku Yamahata <isaku.yamahata@intel.com>