[PATCH 0/2] KVM: x86: Clean up MP_STATE transitions

[PATCH 0/2] KVM: x86: Clean up MP_STATE transitions

[Jim Mattson]

Introduce a generic setter, kvm_set_mp_state(), and use that to ensure that
pv_unhalted is cleared on all transitions to KVM_MP_STATE_RUNNABLE.

Jim Mattson (2):
  KVM: x86: Introduce kvm_set_mp_state()
  KVM: x86: Clear pv_unhalted on all transitions to
    KVM_MP_STATE_RUNNABLE

 arch/x86/kvm/lapic.c      |  6 +++---
 arch/x86/kvm/svm/nested.c |  2 +-
 arch/x86/kvm/svm/sev.c    |  5 ++---
 arch/x86/kvm/vmx/nested.c |  4 ++--
 arch/x86/kvm/x86.c        | 18 ++++++++----------
 arch/x86/kvm/x86.h        |  7 +++++++
 arch/x86/kvm/xen.c        |  4 ++--
 7 files changed, 25 insertions(+), 21 deletions(-)


base-commit: c45323b7560ec87c37c729b703c86ee65f136d75
-- 
2.47.1.688.g23fc6f90ad-goog

[PATCH 1/2] KVM: x86: Introduce kvm_set_mp_state()

[Jim Mattson]

Replace all open-coded assignments to vcpu->arch.mp_state with calls
to a new helper, kvm_set_mp_state(), to centralize all changes to
mp_state.

No functional change intended.

Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/lapic.c      |  6 +++---
 arch/x86/kvm/svm/nested.c |  2 +-
 arch/x86/kvm/svm/sev.c    |  4 ++--
 arch/x86/kvm/vmx/nested.c |  4 ++--
 arch/x86/kvm/x86.c        | 17 ++++++++---------
 arch/x86/kvm/x86.h        |  5 +++++
 arch/x86/kvm/xen.c        |  4 ++--
 7 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 3c83951c619e..bfbc4bc70595 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -3392,9 +3392,9 @@ int kvm_apic_accept_events(struct kvm_vcpu *vcpu)
 	if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events)) {
 		kvm_vcpu_reset(vcpu, true);
 		if (kvm_vcpu_is_bsp(apic->vcpu))
-			vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+			kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
 		else
-			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+			kvm_set_mp_state(vcpu, KVM_MP_STATE_INIT_RECEIVED);
 	}
 	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events)) {
 		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
@@ -3403,7 +3403,7 @@ int kvm_apic_accept_events(struct kvm_vcpu *vcpu)
 			sipi_vector = apic->sipi_vector;
 			kvm_x86_call(vcpu_deliver_sipi_vector)(vcpu,
 							       sipi_vector);
-			vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+			kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
 		}
 	}
 	return 0;
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index b708bdf7eaff..f47906fd9b03 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -994,7 +994,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 	kvm_clear_request(KVM_REQ_GET_NESTED_STATE_PAGES, vcpu);
 
 	/* in case we halted in L2 */
-	svm->vcpu.arch.mp_state = KVM_MP_STATE_RUNNABLE;
+	kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
 
 	/* Give the current vmcb to the guest */
 
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 943bd074a5d3..b4d9efd7537d 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3831,7 +3831,7 @@ static int __sev_snp_update_protected_guest_state(struct kvm_vcpu *vcpu)
 
 	/* Mark the vCPU as offline and not runnable */
 	vcpu->arch.pv.pv_unhalted = false;
-	vcpu->arch.mp_state = KVM_MP_STATE_HALTED;
+	kvm_set_mp_state(vcpu, KVM_MP_STATE_HALTED);
 
 	/* Clear use of the VMSA */
 	svm->vmcb->control.vmsa_pa = INVALID_PAGE;
@@ -3870,7 +3870,7 @@ static int __sev_snp_update_protected_guest_state(struct kvm_vcpu *vcpu)
 
 		/* Mark the vCPU as runnable */
 		vcpu->arch.pv.pv_unhalted = false;
-		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+		kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
 
 		svm->sev_es.snp_vmsa_gpa = INVALID_PAGE;
 
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index aa78b6f38dfe..d53bf4a5ad99 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -3791,7 +3791,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
 		break;
 	case GUEST_ACTIVITY_WAIT_SIPI:
 		vmx->nested.nested_run_pending = 0;
-		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+		kvm_set_mp_state(vcpu, KVM_MP_STATE_INIT_RECEIVED);
 		break;
 	default:
 		break;
@@ -5055,7 +5055,7 @@ void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,
 		vmx->nested.need_vmcs12_to_shadow_sync = true;
 
 	/* in case we halted in L2 */
-	vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+	kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
 
 	if (likely(!vmx->fail)) {
 		if (vm_exit_reason != -1)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c79a8cc57ba4..d6679df95a75 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11208,8 +11208,7 @@ static inline int vcpu_block(struct kvm_vcpu *vcpu)
 	case KVM_MP_STATE_HALTED:
 	case KVM_MP_STATE_AP_RESET_HOLD:
 		vcpu->arch.pv.pv_unhalted = false;
-		vcpu->arch.mp_state =
-			KVM_MP_STATE_RUNNABLE;
+		kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
 		fallthrough;
 	case KVM_MP_STATE_RUNNABLE:
 		vcpu->arch.apf.halted = false;
@@ -11288,7 +11287,7 @@ static int __kvm_emulate_halt(struct kvm_vcpu *vcpu, int state, int reason)
 		if (kvm_vcpu_has_events(vcpu))
 			vcpu->arch.pv.pv_unhalted = false;
 		else
-			vcpu->arch.mp_state = state;
+			kvm_set_mp_state(vcpu, state);
 		return 1;
 	} else {
 		vcpu->run->exit_reason = reason;
@@ -11804,10 +11803,10 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 		goto out;
 
 	if (mp_state->mp_state == KVM_MP_STATE_SIPI_RECEIVED) {
-		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+		kvm_set_mp_state(vcpu, KVM_MP_STATE_INIT_RECEIVED);
 		set_bit(KVM_APIC_SIPI, &vcpu->arch.apic->pending_events);
 	} else
-		vcpu->arch.mp_state = mp_state->mp_state;
+		kvm_set_mp_state(vcpu, mp_state->mp_state);
 	kvm_make_request(KVM_REQ_EVENT, vcpu);
 
 	ret = 0;
@@ -11934,7 +11933,7 @@ static int __set_sregs_common(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs,
 	if (kvm_vcpu_is_bsp(vcpu) && kvm_rip_read(vcpu) == 0xfff0 &&
 	    sregs->cs.selector == 0xf000 && sregs->cs.base == 0xffff0000 &&
 	    !is_protmode(vcpu))
-		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+		kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
 
 	return 0;
 }
@@ -12237,9 +12236,9 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 	kvm_gpc_init(&vcpu->arch.pv_time, vcpu->kvm);
 
 	if (!irqchip_in_kernel(vcpu->kvm) || kvm_vcpu_is_reset_bsp(vcpu))
-		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+		kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
 	else
-		vcpu->arch.mp_state = KVM_MP_STATE_UNINITIALIZED;
+		kvm_set_mp_state(vcpu, KVM_MP_STATE_UNINITIALIZED);
 
 	r = kvm_mmu_create(vcpu);
 	if (r < 0)
@@ -13459,7 +13458,7 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 	}
 
 	vcpu->arch.apf.halted = false;
-	vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+	kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
 }
 
 void kvm_arch_async_page_present_queued(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index ec623d23d13d..bc3b5a9490c6 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -121,6 +121,11 @@ static inline bool kvm_vcpu_has_run(struct kvm_vcpu *vcpu)
 	return vcpu->arch.last_vmentry_cpu != -1;
 }
 
+static inline void kvm_set_mp_state(struct kvm_vcpu *vcpu, int mp_state)
+{
+	vcpu->arch.mp_state = mp_state;
+}
+
 static inline bool kvm_is_exception_pending(struct kvm_vcpu *vcpu)
 {
 	return vcpu->arch.exception.pending ||
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index a909b817b9c0..f65ca27888e9 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -1480,7 +1480,7 @@ static bool kvm_xen_schedop_poll(struct kvm_vcpu *vcpu, bool longmode,
 	set_bit(vcpu->vcpu_idx, vcpu->kvm->arch.xen.poll_mask);
 
 	if (!wait_pending_event(vcpu, sched_poll.nr_ports, ports)) {
-		vcpu->arch.mp_state = KVM_MP_STATE_HALTED;
+		kvm_set_mp_state(vcpu, KVM_MP_STATE_HALTED);
 
 		if (sched_poll.timeout)
 			mod_timer(&vcpu->arch.xen.poll_timer,
@@ -1491,7 +1491,7 @@ static bool kvm_xen_schedop_poll(struct kvm_vcpu *vcpu, bool longmode,
 		if (sched_poll.timeout)
 			del_timer(&vcpu->arch.xen.poll_timer);
 
-		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+		kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
 	}
 
 	vcpu->arch.xen.poll_evtchn = 0;
-- 
2.47.1.688.g23fc6f90ad-goog

[PATCH 2/2] KVM: x86: Clear pv_unhalted on all transitions to KVM_MP_STATE_RUNNABLE

[Jim Mattson]

In kvm_set_mp_state(), ensure that vcpu->arch.pv.pv_unhalted is always
cleared on a transition to KVM_MP_STATE_RUNNABLE, so that the next HLT
instruction will be respected.

The "fixes" list may be incompplete.

Fixes: 6aef266c6e17 ("kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks")
Fixes: b6b8a1451fc4 ("KVM: nVMX: Rework interception of IRQs and NMIs")
Fixes: 38c0b192bd6d ("KVM: SVM: leave halted state on vmexit")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/svm/sev.c | 1 -
 arch/x86/kvm/x86.c     | 1 -
 arch/x86/kvm/x86.h     | 2 ++
 3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index b4d9efd7537d..73e15e7658c2 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3869,7 +3869,6 @@ static int __sev_snp_update_protected_guest_state(struct kvm_vcpu *vcpu)
 		svm->vmcb->control.vmsa_pa = pfn_to_hpa(pfn);
 
 		/* Mark the vCPU as runnable */
-		vcpu->arch.pv.pv_unhalted = false;
 		kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
 
 		svm->sev_es.snp_vmsa_gpa = INVALID_PAGE;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d6679df95a75..4b21ed6803c6 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11207,7 +11207,6 @@ static inline int vcpu_block(struct kvm_vcpu *vcpu)
 	switch(vcpu->arch.mp_state) {
 	case KVM_MP_STATE_HALTED:
 	case KVM_MP_STATE_AP_RESET_HOLD:
-		vcpu->arch.pv.pv_unhalted = false;
 		kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
 		fallthrough;
 	case KVM_MP_STATE_RUNNABLE:
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index bc3b5a9490c6..cc06631027bd 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -124,6 +124,8 @@ static inline bool kvm_vcpu_has_run(struct kvm_vcpu *vcpu)
 static inline void kvm_set_mp_state(struct kvm_vcpu *vcpu, int mp_state)
 {
 	vcpu->arch.mp_state = mp_state;
+	if (mp_state == KVM_MP_STATE_RUNNABLE)
+		vcpu->arch.pv.pv_unhalted = false;
 }
 
 static inline bool kvm_is_exception_pending(struct kvm_vcpu *vcpu)
-- 
2.47.1.688.g23fc6f90ad-goog

Re: [PATCH 2/2] KVM: x86: Clear pv_unhalted on all transitions to KVM_MP_STATE_RUNNABLE

[Jim Mattson]

On Mon, Jan 13, 2025 at 12:02 PM Jim Mattson <jmattson@google.com> wrote:
>
> In kvm_set_mp_state(), ensure that vcpu->arch.pv.pv_unhalted is always
> cleared on a transition to KVM_MP_STATE_RUNNABLE, so that the next HLT
> instruction will be respected.
>
> The "fixes" list may be incomplete.

The only commit I'm not sure of is commit 1a65105a5aba ("KVM: x86/xen:
handle PV spinlocks slowpath"). That commit introduces an mp_state
transition to KVM_MP_STATE_RUNNABLE  without clearing pv_unhalted, so
perhaps it should be in the "fixes" list. OTOH, this seems to be an
independent implementation of PV spinlocks, so maybe it's not a
problem.

> Fixes: 6aef266c6e17 ("kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks")
> Fixes: b6b8a1451fc4 ("KVM: nVMX: Rework interception of IRQs and NMIs")
> Fixes: 38c0b192bd6d ("KVM: SVM: leave halted state on vmexit")
> Signed-off-by: Jim Mattson <jmattson@google.com>

Re: [PATCH 1/2] KVM: x86: Introduce kvm_set_mp_state()

[Sean Christopherson]

On Mon, Jan 13, 2025, Jim Mattson wrote:
> @@ -11288,7 +11287,7 @@ static int __kvm_emulate_halt(struct kvm_vcpu *vcpu, int state, int reason)
>  		if (kvm_vcpu_has_events(vcpu))
>  			vcpu->arch.pv.pv_unhalted = false;
>  		else
> -			vcpu->arch.mp_state = state;
> +			kvm_set_mp_state(vcpu, state);

It wouldn't be appropriate in this patch, but I think it makes sense to invoke
kvm_set_mp_state() instead of open coding the pv.pv_unhalted change.  E.g. if
the vCPU is somehow not already RUNNABLE (which is a bug?), then depending on
when pv_unhalted is set, KVM could either leave the vCPU in the non-RUNNABLE
state (set before __kvm_emulate_halt()), or transition the vCPU to HALTED and
then RUNNABLE (pv_unhalted set after the kvm_vcpu_has_events() check).

Untested, but this?  I'll test and post a patch (assuming it works).

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0aca2a5dac7e..c51499c66cfa 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11292,9 +11292,8 @@ static int __kvm_emulate_halt(struct kvm_vcpu *vcpu, int state, int reason)
        ++vcpu->stat.halt_exits;
        if (lapic_in_kernel(vcpu)) {
                if (kvm_vcpu_has_events(vcpu))
-                       vcpu->arch.pv.pv_unhalted = false;
-               else
-                       kvm_set_mp_state(vcpu, state);
+                       state = KVM_MP_STATE_RUNNABLE;
+               kvm_set_mp_state(vcpu, state);
                return 1;
        } else {
                vcpu->run->exit_reason = reason;

Re: [PATCH 2/2] KVM: x86: Clear pv_unhalted on all transitions to KVM_MP_STATE_RUNNABLE

[Sean Christopherson]

On Wed, Feb 05, 2025, Jim Mattson wrote:
> On Mon, Jan 13, 2025 at 12:02 PM Jim Mattson <jmattson@google.com> wrote:
> >
> > In kvm_set_mp_state(), ensure that vcpu->arch.pv.pv_unhalted is always
> > cleared on a transition to KVM_MP_STATE_RUNNABLE, so that the next HLT
> > instruction will be respected.
> >
> > The "fixes" list may be incomplete.
> 
> The only commit I'm not sure of is commit 1a65105a5aba ("KVM: x86/xen:
> handle PV spinlocks slowpath"). That commit introduces an mp_state
> transition to KVM_MP_STATE_RUNNABLE  without clearing pv_unhalted, so
> perhaps it should be in the "fixes" list. OTOH, this seems to be an
> independent implementation of PV spinlocks, so maybe it's not a
> problem.

I'll add it to the list, and drop the "list may be incomplete" line.
KVM_HC_KICK_CPU is unreachable if Xen hypercalls are enabled, but nothing would
prevent a clever guest from sending an virtual IPI with APIC_DM_REMRD.  Whether
or not that makes the Xen code a KVM bug is definitely debatable, but I can't
imagine will care about Fixes being slightly overzealous.

Re: [PATCH 0/2] KVM: x86: Clean up MP_STATE transitions

[Sean Christopherson]

On Mon, 13 Jan 2025 12:01:42 -0800, Jim Mattson wrote:
> Introduce a generic setter, kvm_set_mp_state(), and use that to ensure that
> pv_unhalted is cleared on all transitions to KVM_MP_STATE_RUNNABLE.
> 
> Jim Mattson (2):
>   KVM: x86: Introduce kvm_set_mp_state()
>   KVM: x86: Clear pv_unhalted on all transitions to
>     KVM_MP_STATE_RUNNABLE
> 
> [...]

Applied to kvm-x86 misc, thanks!

[1/2] KVM: x86: Introduce kvm_set_mp_state()
      https://github.com/kvm-x86/linux/commit/c9e5f3fa9039
[2/2] KVM: x86: Clear pv_unhalted on all transitions to KVM_MP_STATE_RUNNABLE
      https://github.com/kvm-x86/linux/commit/e9cb61055fee

--
https://github.com/kvm-x86/linux/tree/next