On Fri, Jun 30, 2023 at 06:14:31PM +0800,
Chao Gao <chao.gao@xxxxxxxxx> wrote:
> On Fri, Jun 30, 2023 at 03:26:12PM +0800, Qi Ai wrote:
> >--- a/arch/x86/include/asm/kvm_host.h
> >+++ b/arch/x86/include/asm/kvm_host.h
> >@@ -1731,6 +1731,8 @@ struct kvm_x86_ops {
> > * Returns vCPU specific APICv inhibit reasons
> > */
> > unsigned long (*vcpu_get_apicv_inhibit_reasons)(struct kvm_vcpu *vcpu);
> >+
> >+ void (*clear_hlt)(struct kvm_vcpu *vcpu);
>
> add this new ops to arch/x86/include/asm/kvm-x86-ops.h, and declare it
> optional, i.e.,
>
> KVM_X86_OP_OPTIONAL(...)
>
>
> > };
> >
> > struct kvm_x86_nested_ops {
> >diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> >index 44fb619803b8..11c2fde1ad98 100644
> >--- a/arch/x86/kvm/vmx/vmx.c
> >+++ b/arch/x86/kvm/vmx/vmx.c
> >@@ -8266,6 +8266,8 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
> > .complete_emulated_msr = kvm_complete_insn_gp,
> >
> > .vcpu_deliver_sipi_vector = kvm_vcpu_deliver_sipi_vector,
> >+
> >+ .clear_hlt = vmx_clear_hlt,
> > };
> >
> > static unsigned int vmx_handle_intel_pt_intr(void)
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index 7f70207e8689..21360f5ed006 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -11258,6 +11258,12 @@ static void __set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
> > vcpu->arch.exception_vmexit.pending = false;
> >
> > kvm_make_request(KVM_REQ_EVENT, vcpu);
> >+
> >+ if (kvm_x86_ops.clear_hlt) {
> >+ if (kvm_vcpu_is_bsp(vcpu) && regs->rip == 0xfff0 &&
>
> it is weird this applies to BSP only.
>
> >+ !is_protmode(vcpu))
> >+ kvm_x86_ops.clear_hlt(vcpu);
>
> Use static_call_cond(kvm_x86_clear_hlt)(vcpu) instead.
>
> It looks incorrect that we add this side-effect heuristically here. I
> am wondering if we can link vcpu->arch.mp_state to VMCS activity state,
> i.e., when mp_state is set to RUNNABLE in KVM_SET_MP_STATE ioctl, KVM
> sets VMCS activity state to active.
Yes, when vcpu is reset should be checked by mp-state change. Strictly we should
clear other guest non-register state strictly. interruptibility state, pending
debug exceptions, and guest interrupt status. In practice, those status other
than hlt wouldn't matter much, though.
--
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
