linux-kernel - Re: [PATCH v19 108/130] KVM: TDX: Handle TDX PV HLT hypercall

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zg1seIaTmM94IyR8@google.com>
Date: Wed, 3 Apr 2024 07:49:28 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Chao Gao <chao.gao@...el.com>
Cc: isaku.yamahata@...el.com, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, isaku.yamahata@...il.com, 
	Paolo Bonzini <pbonzini@...hat.com>, erdemaktas@...gle.com, Sagi Shahar <sagis@...gle.com>, 
	Kai Huang <kai.huang@...el.com>, chen.bo@...el.com, hang.yuan@...el.com, 
	tina.zhang@...el.com
Subject: Re: [PATCH v19 108/130] KVM: TDX: Handle TDX PV HLT hypercall

On Wed, Apr 03, 2024, Chao Gao wrote:
> On Mon, Feb 26, 2024 at 12:26:50AM -0800, isaku.yamahata@...el.com wrote:
> >From: Isaku Yamahata <isaku.yamahata@...el.com>
> >
> >Wire up TDX PV HLT hypercall to the KVM backend function.
> >
> >Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.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@...el.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. 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);