Messages in this thread | | | Date | Wed, 3 Apr 2024 07:49:28 -0700 | Subject | Re: [PATCH v19 108/130] KVM: TDX: Handle TDX PV HLT hypercall | From | Sean Christopherson <> |
| |
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. 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);
| |