KVM: x86: Convert INIT and SIPI signals into synchronously handled events

PATCH In review Jan Kiszka 4 participants 25 messages Mar 12, 2013 – Mar 13, 2013

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Jan Kiszka

author

2013-03-12 11:44 UTC #0 lore raw

PATCH 1/1 KVM: x86: Convert INIT and SIPI signals into synchronously handled events

A VCPU sending INIT or SIPI to some other VCPU races for setting the remote VCPU's mp_state. When we were unlucky, KVM_MP_STATE_INIT_RECEIVED was overwritten by kvm_emulate_halt and, thus, got lost.

This introduces APIC events for those two signals, keeping them in kvm_apic until kvm_apic_accept_events is run over the target vcpu context. kvm_apic_has_events reports to kvm_arch_vcpu_runnable if there are pending events, thus if vcpu blocking should end.

The patch comes with the side effect of effectively obsoleting KVM_MP_STATE_SIPI_RECEIVED. We still accept it from user space, but immediately translate it to KVM_MP_STATE_INIT_RECEIVED + KVM_APIC_SIPI. The vcpu itself will no longer enter the KVM_MP_STATE_SIPI_RECEIVED state. That also means we no longer exit to user space after receiving a SIPI event.

Signed-off-by Jan Kiszka <[email protected]>

---

This doesn't fix the wrong behaviour on INIT for the BSP yet. Leaving this honor to Paolo.

I didn't try porting any INIT handling for nested on top yet but I think it should be feasible - once we know their semantics for sure, at least on Intel...

 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/lapic.c            |   41 +++++++++++++++++++++++++++++++-------
 arch/x86/kvm/lapic.h            |    7 ++++++
 arch/x86/kvm/x86.c              |   39 ++++++++++++++++++++----------------
 4 files changed, 63 insertions(+), 25 deletions(-)

arch/x86/include/asm/kvm_host.h +1 -0

index 348d859..2d28e76 100644
@@ -1002,6 +1002,7 @@ int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v);
 int kvm_cpu_has_interrupt(struct kvm_vcpu *vcpu);
 int kvm_arch_interrupt_allowed(struct kvm_vcpu *vcpu);
 int kvm_cpu_get_interrupt(struct kvm_vcpu *v);
+void kvm_vcpu_reset(struct kvm_vcpu *vcpu);
 
 void kvm_define_shared_msr(unsigned index, u32 msr);
 void kvm_set_shared_msr(unsigned index, u64 val, u64 mask);

arch/x86/kvm/lapic.c +33 -8

59 lines not on this page · raw message

arch/x86/kvm/lapic.h +7 -0

index 1676d34..ef3f4ef 100644
@@ -5,6 +5,9 @@
 
 #include <linux/kvm_host.h>
 
+#define KVM_APIC_INIT		0
+#define KVM_APIC_SIPI		1
+
 struct kvm_timer {
 	struct hrtimer timer;
 	s64 period; 				/* unit: ns */

@@ -32,13 +35,17 @@ struct kvm_lapic {
 	void *regs;
 	gpa_t vapic_addr;
 	struct page *vapic_page;
+	unsigned long pending_events;
+	unsigned int sipi_vector;
 };
 int kvm_create_lapic(struct kvm_vcpu *vcpu);
 void kvm_free_lapic(struct kvm_vcpu *vcpu);
 
 int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu);
+bool kvm_apic_has_events(struct kvm_vcpu *vcpu);
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu);
 int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
+void kvm_apic_accept_events(struct kvm_vcpu *vcpu);
 void kvm_lapic_reset(struct kvm_vcpu *vcpu);
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);

arch/x86/kvm/x86.c +22 ~~-17~~

index b891ac3..a0b8041 100644
@@ -162,8 +162,6 @@ u64 __read_mostly host_xcr0;
 
 static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt);
 
-static void kvm_vcpu_reset(struct kvm_vcpu *vcpu);
-
 static inline void kvm_async_pf_hash_reset(struct kvm_vcpu *vcpu)
 {
 	int i;

@@ -5663,6 +5661,11 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	bool req_immediate_exit = 0;
 
 	if (vcpu->requests) {
+		kvm_apic_accept_events(vcpu);
+		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+			r = 1;
+			goto out;
+		}
 		if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
 			kvm_mmu_unload(vcpu);
 		if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))

@@ -5847,14 +5850,6 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 	int r;
 	struct kvm *kvm = vcpu->kvm;
 
-	if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED)) {
-		pr_debug("vcpu %d received sipi with vector # %x\n",
-			 vcpu->vcpu_id, vcpu->arch.sipi_vector);
-		kvm_lapic_reset(vcpu);
-		kvm_vcpu_reset(vcpu);
-		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
-	}
-
 	vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
 	r = vapic_enter(vcpu);
 	if (r) {

@@ -5871,8 +5866,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
 			kvm_vcpu_block(vcpu);
 			vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
-			if (kvm_check_request(KVM_REQ_UNHALT, vcpu))
-			{
+			if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) {
+				kvm_apic_accept_events(vcpu);
 				switch(vcpu->arch.mp_state) {
 				case KVM_MP_STATE_HALTED:
 					vcpu->arch.mp_state =

@@ -5880,7 +5875,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 				case KVM_MP_STATE_RUNNABLE:
 					vcpu->arch.apf.halted = false;
 					break;
-				case KVM_MP_STATE_SIPI_RECEIVED:
+				case KVM_MP_STATE_INIT_RECEIVED:
+					break;
 				default:
 					r = -EINTR;
 					break;

@@ -5901,7 +5897,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			++vcpu->stat.request_irq_exits;
 		}
 
-		kvm_check_async_pf_completion(vcpu);
+		if (vcpu->arch.mp_state != KVM_MP_STATE_INIT_RECEIVED)
+			kvm_check_async_pf_completion(vcpu);
 
 		if (signal_pending(current)) {
 			r = -EINTR;

@@ -6015,6 +6012,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 
 	if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_UNINITIALIZED)) {
 		kvm_vcpu_block(vcpu);
+		kvm_apic_accept_events(vcpu);
 		clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
 		r = -EAGAIN;
 		goto out;

@@ -6171,6 +6169,7 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
+	kvm_apic_accept_events(vcpu);
 	mp_state->mp_state = vcpu->arch.mp_state;
 	return 0;
 }

@@ -6178,7 +6177,13 @@ int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
-	vcpu->arch.mp_state = mp_state->mp_state;
+	if (mp_state->mp_state == KVM_MP_STATE_SIPI_RECEIVED) {
+		if (!kvm_vcpu_has_lapic(vcpu))
+			return -EINVAL;
+		vcpu->arch.mp_state = 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_make_request(KVM_REQ_EVENT, vcpu);
 	return 0;
 }

@@ -6515,7 +6520,7 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
 	kvm_x86_ops->vcpu_free(vcpu);
 }
 
-static void kvm_vcpu_reset(struct kvm_vcpu *vcpu)
+void kvm_vcpu_reset(struct kvm_vcpu *vcpu)
 {
 	atomic_set(&vcpu->arch.nmi_queued, 0);
 	vcpu->arch.nmi_pending = 0;

@@ -6988,7 +6993,7 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
 	return (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE &&
 		!vcpu->arch.apf.halted)
 		|| !list_empty_careful(&vcpu->async_pf.done)
-		|| vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED
+		|| kvm_apic_has_events(vcpu)
 		|| atomic_read(&vcpu->arch.nmi_queued) ||
 		(kvm_arch_interrupt_allowed(vcpu) &&
 		 kvm_cpu_has_interrupt(vcpu));

Signature · 1 line

1.7.3.4

Paolo Bonzini

maintainer

2013-03-12 12:07 UTC #1 lore raw

Il 12/03/2013 12:44, Jan Kiszka ha scritto:

Quoted 49 lines

A VCPU sending INIT or SIPI to some other VCPU races for setting the remote VCPU's mp_state. When we were unlucky, KVM_MP_STATE_INIT_RECEIVED was overwritten by kvm_emulate_halt and, thus, got lost.
This introduces APIC events for those two signals, keeping them in kvm_apic until kvm_apic_accept_events is run over the target vcpu context. kvm_apic_has_events reports to kvm_arch_vcpu_runnable if there are pending events, thus if vcpu blocking should end.
The patch comes with the side effect of effectively obsoleting KVM_MP_STATE_SIPI_RECEIVED. We still accept it from user space, but immediately translate it to KVM_MP_STATE_INIT_RECEIVED + KVM_APIC_SIPI. The vcpu itself will no longer enter the KVM_MP_STATE_SIPI_RECEIVED state. That also means we no longer exit to user space after receiving a SIPI event.
Signed-off-by Jan Kiszka <[email protected]>
---
This doesn't fix the wrong behaviour on INIT for the BSP yet. Leaving this honor to Paolo.
I didn't try porting any INIT handling for nested on top yet but I think it should be feasible - once we know their semantics for sure, at least on Intel...
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/lapic.c            |   41 +++++++++++++++++++++++++++++++-------
 arch/x86/kvm/lapic.h            |    7 ++++++
 arch/x86/kvm/x86.c              |   39 ++++++++++++++++++++----------------
 4 files changed, 63 insertions(+), 25 deletions(-)
arch/x86/include/asm/kvm_host.h +1 -0
index 348d859..2d28e76 100644
@@ -1002,6 +1002,7 @@ int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v);
 int kvm_cpu_has_interrupt(struct kvm_vcpu *vcpu);
 int kvm_arch_interrupt_allowed(struct kvm_vcpu *vcpu);
 int kvm_cpu_get_interrupt(struct kvm_vcpu *v);
+void kvm_vcpu_reset(struct kvm_vcpu *vcpu);
 
 void kvm_define_shared_msr(unsigned index, u32 msr);
 void kvm_set_shared_msr(unsigned index, u64 val, u64 mask);
arch/x86/kvm/lapic.c +0 -0
index 02b51dd..4a21a6b 100644

arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, · line 734

Quoted 5 lines

arch/x86/kvm/lapic.c +1 -1

@@ -731,7 +731,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 	case APIC_DM_INIT:
 		if (!trig_mode || level) {
 			result = 1;
-			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+			set_bit(KVM_APIC_INIT, &apic->pending_events);

I think this should clear pending SIPIs, unless KVM_APIC_INIT was already set in which case it should be a no-op. Something like:

	e = apic->pending_events;
	while (!(e & KVM_APIC_INIT))
		e = cmpxchg(&apic->pending_events, e,
			    (e | KVM_APIC_INIT) & ~KVM_APIC_SIPI);

If you do this, better make pending_events an atomic_t.

Quoted 150 lines

arch/x86/kvm/lapic.c +32 -7

 			kvm_make_request(KVM_REQ_EVENT, vcpu);
 			kvm_vcpu_kick(vcpu);
 		} else {

@@ -743,13 +743,13 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 	case APIC_DM_STARTUP:
 		apic_debug("SIPI to vcpu %d vector 0x%02x\n",
 			   vcpu->vcpu_id, vector);
-		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
-			result = 1;
-			vcpu->arch.sipi_vector = vector;
-			vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
-			kvm_vcpu_kick(vcpu);
-		}
+		result = 1;
+		apic->sipi_vector = vector;
+		/* make sure sipi_vector is visible for the receiver */
+		smp_wmb();
+		set_bit(KVM_APIC_SIPI, &apic->pending_events);
+		kvm_make_request(KVM_REQ_EVENT, vcpu);
+		kvm_vcpu_kick(vcpu);
 		break;
 
 	case APIC_DM_EXTINT:

@@ -1860,6 +1860,31 @@ int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data)
 					 addr);
 }
 
+bool kvm_apic_has_events(struct kvm_vcpu *vcpu)
+{
+	return kvm_vcpu_has_lapic(vcpu) && vcpu->arch.apic->pending_events;
+}
+
+void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	if (!kvm_vcpu_has_lapic(vcpu))
+		return;
+
+	if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events))
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
+	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+		vcpu->arch.sipi_vector = apic->sipi_vector;
+		pr_debug("vcpu %d received sipi with vector # %x\n",
+			 vcpu->vcpu_id, vcpu->arch.sipi_vector);
+		kvm_lapic_reset(vcpu);
+		kvm_vcpu_reset(vcpu);
+		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+	}
+}
+
 void kvm_lapic_init(void)
 {
 	/* do not patch jump label more than once per second */

arch/x86/kvm/lapic.h +7 -0

index 1676d34..ef3f4ef 100644
@@ -5,6 +5,9 @@
 
 #include <linux/kvm_host.h>
 
+#define KVM_APIC_INIT		0
+#define KVM_APIC_SIPI		1
+
 struct kvm_timer {
 	struct hrtimer timer;
 	s64 period; 				/* unit: ns */

@@ -32,13 +35,17 @@ struct kvm_lapic {
 	void *regs;
 	gpa_t vapic_addr;
 	struct page *vapic_page;
+	unsigned long pending_events;
+	unsigned int sipi_vector;
 };
 int kvm_create_lapic(struct kvm_vcpu *vcpu);
 void kvm_free_lapic(struct kvm_vcpu *vcpu);
 
 int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu);
+bool kvm_apic_has_events(struct kvm_vcpu *vcpu);
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu);
 int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
+void kvm_apic_accept_events(struct kvm_vcpu *vcpu);
 void kvm_lapic_reset(struct kvm_vcpu *vcpu);
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);

arch/x86/kvm/x86.c +9 ~~-13~~

index b891ac3..a0b8041 100644
@@ -162,8 +162,6 @@ u64 __read_mostly host_xcr0;
 
 static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt);
 
-static void kvm_vcpu_reset(struct kvm_vcpu *vcpu);
-
 static inline void kvm_async_pf_hash_reset(struct kvm_vcpu *vcpu)
 {
 	int i;

@@ -5663,6 +5661,11 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	bool req_immediate_exit = 0;
 
 	if (vcpu->requests) {
+		kvm_apic_accept_events(vcpu);
+		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+			r = 1;
+			goto out;
+		}
 		if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
 			kvm_mmu_unload(vcpu);
 		if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))

@@ -5847,14 +5850,6 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 	int r;
 	struct kvm *kvm = vcpu->kvm;
 
-	if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED)) {
-		pr_debug("vcpu %d received sipi with vector # %x\n",
-			 vcpu->vcpu_id, vcpu->arch.sipi_vector);
-		kvm_lapic_reset(vcpu);
-		kvm_vcpu_reset(vcpu);
-		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
-	}
-
 	vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
 	r = vapic_enter(vcpu);
 	if (r) {

@@ -5871,8 +5866,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
 			kvm_vcpu_block(vcpu);
 			vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
-			if (kvm_check_request(KVM_REQ_UNHALT, vcpu))
-			{
+			if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) {
+				kvm_apic_accept_events(vcpu);
 				switch(vcpu->arch.mp_state) {
 				case KVM_MP_STATE_HALTED:
 					vcpu->arch.mp_state =

@@ -5880,7 +5875,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 				case KVM_MP_STATE_RUNNABLE:
 					vcpu->arch.apf.halted = false;
 					break;
-				case KVM_MP_STATE_SIPI_RECEIVED:
+				case KVM_MP_STATE_INIT_RECEIVED:
+					break;
 				default:
 					r = -EINTR;
 					break;

arch/x86/kvm/x86.c static int __vcpu_run(struct kvm_vcpu *vcpu) · line 5901

Quoted 6 lines

arch/x86/kvm/x86.c +2 -1

@@ -5901,7 +5897,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			++vcpu->stat.request_irq_exits;
 		}
 
-		kvm_check_async_pf_completion(vcpu);
+		if (vcpu->arch.mp_state != KVM_MP_STATE_INIT_RECEIVED)
+			kvm_check_async_pf_completion(vcpu);

Can you instead do another change that affects the conditions of kvm_check_async_pf_completion?

For example, should kvm_arch_interrupt_allowed return zero if the VCPU is in the INIT_RECEIVED state?

Quoted 12 lines

arch/x86/kvm/x86.c +1 -0

 		if (signal_pending(current)) {
 			r = -EINTR;

@@ -6015,6 +6012,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 
 	if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_UNINITIALIZED)) {
 		kvm_vcpu_block(vcpu);
+		kvm_apic_accept_events(vcpu);
 		clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
 		r = -EAGAIN;
 		goto out;

arch/x86/kvm/x86.c int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu, · line 6172

Quoted 4 lines

arch/x86/kvm/x86.c +1 -0

@@ -6171,6 +6169,7 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
+	kvm_apic_accept_events(vcpu);

Is this racy against __vcpu_run?

Quoted 5 lines

arch/x86/kvm/x86.c +0 -0

 	mp_state->mp_state = vcpu->arch.mp_state;
 	return 0;
 }

@@ -6178,7 +6177,13 @@ int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,

arch/x86/kvm/x86.c int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu, · line 6186

Quoted 10 lines

arch/x86/kvm/x86.c +7 -1

 				    struct kvm_mp_state *mp_state)
 {
-	vcpu->arch.mp_state = mp_state->mp_state;
+	if (mp_state->mp_state == KVM_MP_STATE_SIPI_RECEIVED) {
+		if (!kvm_vcpu_has_lapic(vcpu))
+			return -EINVAL;
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+		set_bit(KVM_APIC_SIPI, &vcpu->arch.apic->pending_events);
+	} else
+		vcpu->arch.mp_state = mp_state->mp_state;

Should INIT_RECEIVED also be invalid without an in-kernel LAPIC?

Quoted 22 lines

arch/x86/kvm/x86.c +2 -2

 	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	return 0;
 }

@@ -6515,7 +6520,7 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
 	kvm_x86_ops->vcpu_free(vcpu);
 }
 
-static void kvm_vcpu_reset(struct kvm_vcpu *vcpu)
+void kvm_vcpu_reset(struct kvm_vcpu *vcpu)
 {
 	atomic_set(&vcpu->arch.nmi_queued, 0);
 	vcpu->arch.nmi_pending = 0;

@@ -6988,7 +6993,7 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
 	return (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE &&
 		!vcpu->arch.apf.halted)
 		|| !list_empty_careful(&vcpu->async_pf.done)
-		|| vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED
+		|| kvm_apic_has_events(vcpu)
 		|| atomic_read(&vcpu->arch.nmi_queued) ||
 		(kvm_arch_interrupt_allowed(vcpu) &&
 		 kvm_cpu_has_interrupt(vcpu));

Gleb Natapov

2013-03-12 12:29 UTC #2 lore raw

Quoted 8 lines · Paolo Bonzini

arch/x86/kvm/x86.c int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu, · line 6172
Quoted 5 lines
arch/x86/kvm/x86.c +1 -0
@@ -6171,6 +6169,7 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
+	kvm_apic_accept_events(vcpu);
Is this racy against __vcpu_run?

FWIW both of them are vcpu ioctl and thus run under vcpu mutex.

--
Gleb.

Paolo Bonzini

maintainer

2013-03-12 12:29 UTC #3 lore raw

In reply to Paolo Bonzini

Il 12/03/2013 13:06, Paolo Bonzini ha scritto:

Quoted 6 lines

arch/x86/kvm/x86.c +1 -1

@@ -6178,7 +6177,13 @@ int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
-	vcpu->arch.mp_state = mp_state->mp_state;
+	if (mp_state->mp_state == KVM_MP_STATE_SIPI_RECEIVED) {

Quoted 8 lines

arch/x86/kvm/x86.c int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu, · line 6186
Quoted 6 lines
arch/x86/kvm/x86.c +6 -0
+		if (!kvm_vcpu_has_lapic(vcpu))
+			return -EINVAL;
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+		set_bit(KVM_APIC_SIPI, &vcpu->arch.apic->pending_events);
+	} else
+		vcpu->arch.mp_state = mp_state->mp_state;
Should INIT_RECEIVED also be invalid without an in-kernel LAPIC?

And since migration was brought up yesterday, do we need an interface to retrieve and set this?

And should KVM_GET/SET_VCPU_EVENTS use the new sipi_vector in the APIC rather than the old one?

Paolo

Jan Kiszka

author

2013-03-12 12:46 UTC #4 lore raw

Quoted 16 lines · Paolo Bonzini

Il 12/03/2013 13:06, Paolo Bonzini ha scritto:
Quoted 6 lines
Quoted 6 lines
arch/x86/kvm/x86.c +1 -1
@@ -6178,7 +6177,13 @@ int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
-	vcpu->arch.mp_state = mp_state->mp_state;
+	if (mp_state->mp_state == KVM_MP_STATE_SIPI_RECEIVED) {
Quoted 8 lines
arch/x86/kvm/x86.c int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu, · line 6186
Quoted 6 lines
arch/x86/kvm/x86.c +6 -0
+		if (!kvm_vcpu_has_lapic(vcpu))
+			return -EINVAL;
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+		set_bit(KVM_APIC_SIPI, &vcpu->arch.apic->pending_events);
+	} else
+		vcpu->arch.mp_state = mp_state->mp_state;
Should INIT_RECEIVED also be invalid without an in-kernel LAPIC?

Quoted 5 lines

And since migration was brought up yesterday, do we need an interface to retrieve and set this?
And should KVM_GET/SET_VCPU_EVENTS use the new sipi_vector in the APIC rather than the old one?

I hope not. The idea is that the APIC events are processed before the migration completes. Translating events on get_mpstate should ensure this.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Gleb Natapov

2013-03-12 12:49 UTC #5 lore raw

Quoted 17 lines · Jan Kiszka

Quoted 16 lines · Paolo Bonzini
Il 12/03/2013 13:06, Paolo Bonzini ha scritto:
Quoted 14 lines
arch/x86/kvm/x86.c int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu, · line 6186
Quoted 12 lines
arch/x86/kvm/x86.c +7 -1
@@ -6178,7 +6177,13 @@ int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
-	vcpu->arch.mp_state = mp_state->mp_state;
+	if (mp_state->mp_state == KVM_MP_STATE_SIPI_RECEIVED) {
+		if (!kvm_vcpu_has_lapic(vcpu))
+			return -EINVAL;
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+		set_bit(KVM_APIC_SIPI, &vcpu->arch.apic->pending_events);
+	} else
+		vcpu->arch.mp_state = mp_state->mp_state;
Should INIT_RECEIVED also be invalid without an in-kernel LAPIC?

Quoted 9 lines

Quoted 5 lines
And since migration was brought up yesterday, do we need an interface to retrieve and set this?
And should KVM_GET/SET_VCPU_EVENTS use the new sipi_vector in the APIC rather than the old one?
I hope not. The idea is that the APIC events are processed before the migration completes. Translating events on get_mpstate should ensure this.

But when you will add nested support it will not be that simple, so as part of migration with nested guests we will have to transfer pending_events too instead of processing then on (set|get)_mpstate.

--
Gleb.

Jan Kiszka

author

2013-03-12 12:52 UTC #6 lore raw

Quoted 24 lines · Gleb Natapov

Quoted 23 lines · Jan Kiszka
Quoted 21 lines · Paolo Bonzini
Il 12/03/2013 13:06, Paolo Bonzini ha scritto:
Quoted 6 lines
Quoted 6 lines
arch/x86/kvm/x86.c +1 -1
@@ -6178,7 +6177,13 @@ int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
-	vcpu->arch.mp_state = mp_state->mp_state;
+	if (mp_state->mp_state == KVM_MP_STATE_SIPI_RECEIVED) {
Quoted 8 lines
arch/x86/kvm/x86.c int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu, · line 6186
Quoted 6 lines
arch/x86/kvm/x86.c +6 -0
+		if (!kvm_vcpu_has_lapic(vcpu))
+			return -EINVAL;
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+		set_bit(KVM_APIC_SIPI, &vcpu->arch.apic->pending_events);
+	} else
+		vcpu->arch.mp_state = mp_state->mp_state;
Should INIT_RECEIVED also be invalid without an in-kernel LAPIC?
And since migration was brought up yesterday, do we need an interface to retrieve and set this?
And should KVM_GET/SET_VCPU_EVENTS use the new sipi_vector in the APIC rather than the old one?

Quoted 6 lines

Quoted 3 lines
I hope not. The idea is that the APIC events are processed before the migration completes. Translating events on get_mpstate should ensure this.
But when you will add nested support it will not be that simple, so as part of migration with nested guests we will have to transfer pending_events too instead of processing then on (set|get)_mpstate.

Right, but that can then easily become part of the to-be-defined nested vcpu state (which is likely more than vmcs12).

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Gleb Natapov

2013-03-12 12:53 UTC #7 lore raw

Quoted 28 lines · Jan Kiszka

Quoted 27 lines · Gleb Natapov
Quoted 26 lines · Jan Kiszka
Quoted 16 lines · Paolo Bonzini
Il 12/03/2013 13:06, Paolo Bonzini ha scritto:
Quoted 6 lines
Quoted 6 lines
arch/x86/kvm/x86.c +1 -1
@@ -6178,7 +6177,13 @@ int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
-	vcpu->arch.mp_state = mp_state->mp_state;
+	if (mp_state->mp_state == KVM_MP_STATE_SIPI_RECEIVED) {
Quoted 8 lines
arch/x86/kvm/x86.c int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu, · line 6186
Quoted 6 lines
arch/x86/kvm/x86.c +6 -0
+		if (!kvm_vcpu_has_lapic(vcpu))
+			return -EINVAL;
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+		set_bit(KVM_APIC_SIPI, &vcpu->arch.apic->pending_events);
+	} else
+		vcpu->arch.mp_state = mp_state->mp_state;
Should INIT_RECEIVED also be invalid without an in-kernel LAPIC?
Quoted 5 lines
And since migration was brought up yesterday, do we need an interface to retrieve and set this?
And should KVM_GET/SET_VCPU_EVENTS use the new sipi_vector in the APIC rather than the old one?
I hope not. The idea is that the APIC events are processed before the migration completes. Translating events on get_mpstate should ensure this.

Quoted 7 lines

Quoted 3 lines
But when you will add nested support it will not be that simple, so as part of migration with nested guests we will have to transfer pending_events too instead of processing then on (set|get)_mpstate.
Right, but that can then easily become part of the to-be-defined nested vcpu state (which is likely more than vmcs12).

Yes, I am not saying we have to implement it right away.

--
Gleb.

Paolo Bonzini

maintainer

2013-03-12 13:09 UTC #8 lore raw

In reply to Jan Kiszka

Il 12/03/2013 13:46, Jan Kiszka ha scritto:

Quoted 15 lines

Quoted 15 lines
Quoted 15 lines
Quoted 14 lines
Quoted 14 lines
Quoted 4 lines
Quoted 4 lines
arch/x86/kvm/x86.c +0 -0
@@ -6178,7 +6177,13 @@ int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
Quoted 8 lines
arch/x86/kvm/x86.c int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu, · line 6186
Quoted 8 lines
arch/x86/kvm/x86.c +7 -1
-	vcpu->arch.mp_state = mp_state->mp_state;
+	if (mp_state->mp_state == KVM_MP_STATE_SIPI_RECEIVED) {
+		if (!kvm_vcpu_has_lapic(vcpu))
+			return -EINVAL;
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+		set_bit(KVM_APIC_SIPI, &vcpu->arch.apic->pending_events);
+	} else
+		vcpu->arch.mp_state = mp_state->mp_state;
Should INIT_RECEIVED also be invalid without an in-kernel LAPIC?

Quoted 7 lines

Quoted 5 lines
Quoted 5 lines
And since migration was brought up yesterday, do we need an interface to retrieve and set this?
And should KVM_GET/SET_VCPU_EVENTS use the new sipi_vector in the APIC rather than the old one?
I hope not. The idea is that the APIC events are processed before the migration completes. Translating events on get_mpstate should ensure this.

What about persistent state such as "an INIT has been received and caused a vmexit, but is being latched until vmxoff"? Perhaps we could use the top 8 bits of vcpu->arch.sipi_vector for this, they are always shifted out when sipi_vector is used, and should be zero in all cases. It would then be possible to reuse KVM_GET/SET_VCPU_EVENTS.

Migration support for nested VMX is still far far away, so perhaps we do not care, but we still need a way to inject INIT from userspace.

Paolo

Jan Kiszka

author

2013-03-12 13:13 UTC #9 lore raw

Quoted 21 lines · Paolo Bonzini

Il 12/03/2013 13:46, Jan Kiszka ha scritto:
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
arch/x86/kvm/x86.c +1 -1
@@ -6178,7 +6177,13 @@ int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
-	vcpu->arch.mp_state = mp_state->mp_state;
+	if (mp_state->mp_state == KVM_MP_STATE_SIPI_RECEIVED) {
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
arch/x86/kvm/x86.c int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu, · line 6186
Quoted 6 lines
arch/x86/kvm/x86.c +6 -0
+		if (!kvm_vcpu_has_lapic(vcpu))
+			return -EINVAL;
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+		set_bit(KVM_APIC_SIPI, &vcpu->arch.apic->pending_events);
+	} else
+		vcpu->arch.mp_state = mp_state->mp_state;
Should INIT_RECEIVED also be invalid without an in-kernel LAPIC?
And since migration was brought up yesterday, do we need an interface to retrieve and set this?
And should KVM_GET/SET_VCPU_EVENTS use the new sipi_vector in the APIC rather than the old one?

Quoted 8 lines

Quoted 2 lines
I hope not. The idea is that the APIC events are processed before the migration completes. Translating events on get_mpstate should ensure this.
What about persistent state such as "an INIT has been received and caused a vmexit, but is being latched until vmxoff"? Perhaps we could use the top 8 bits of vcpu->arch.sipi_vector for this, they are always shifted out when sipi_vector is used, and should be zero in all cases. It would then be possible to reuse KVM_GET/SET_VCPU_EVENTS.

See my other mail.

Quoted 3 lines

Migration support for nested VMX is still far far away, so perhaps we do not care, but we still need a way to inject INIT from userspace.

Ack for INIT injection. In theory, you could use the VCPU event IOCTL, it is extensible. If that is elegant is another question.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Jan Kiszka

author

2013-03-12 12:58 UTC #10 lore raw

In reply to Paolo Bonzini

Quoted 59 lines · Paolo Bonzini

Il 12/03/2013 12:44, Jan Kiszka ha scritto:
Quoted 49 lines
A VCPU sending INIT or SIPI to some other VCPU races for setting the remote VCPU's mp_state. When we were unlucky, KVM_MP_STATE_INIT_RECEIVED was overwritten by kvm_emulate_halt and, thus, got lost.
This introduces APIC events for those two signals, keeping them in kvm_apic until kvm_apic_accept_events is run over the target vcpu context. kvm_apic_has_events reports to kvm_arch_vcpu_runnable if there are pending events, thus if vcpu blocking should end.
The patch comes with the side effect of effectively obsoleting KVM_MP_STATE_SIPI_RECEIVED. We still accept it from user space, but immediately translate it to KVM_MP_STATE_INIT_RECEIVED + KVM_APIC_SIPI. The vcpu itself will no longer enter the KVM_MP_STATE_SIPI_RECEIVED state. That also means we no longer exit to user space after receiving a SIPI event.
Signed-off-by Jan Kiszka <[email protected]>
---
This doesn't fix the wrong behaviour on INIT for the BSP yet. Leaving this honor to Paolo.
I didn't try porting any INIT handling for nested on top yet but I think it should be feasible - once we know their semantics for sure, at least on Intel...
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/lapic.c            |   41 +++++++++++++++++++++++++++++++-------
 arch/x86/kvm/lapic.h            |    7 ++++++
 arch/x86/kvm/x86.c              |   39 ++++++++++++++++++++----------------
 4 files changed, 63 insertions(+), 25 deletions(-)
arch/x86/include/asm/kvm_host.h +1 -0
index 348d859..2d28e76 100644
@@ -1002,6 +1002,7 @@ int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v);
 int kvm_cpu_has_interrupt(struct kvm_vcpu *vcpu);
 int kvm_arch_interrupt_allowed(struct kvm_vcpu *vcpu);
 int kvm_cpu_get_interrupt(struct kvm_vcpu *v);
+void kvm_vcpu_reset(struct kvm_vcpu *vcpu);
 
 void kvm_define_shared_msr(unsigned index, u32 msr);
 void kvm_set_shared_msr(unsigned index, u64 val, u64 mask);
arch/x86/kvm/lapic.c +0 -0
index 02b51dd..4a21a6b 100644
arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, · line 734
Quoted 5 lines
arch/x86/kvm/lapic.c +1 -1
@@ -731,7 +731,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 	case APIC_DM_INIT:
 		if (!trig_mode || level) {
 			result = 1;
-			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+			set_bit(KVM_APIC_INIT, &apic->pending_events);
I think this should clear pending SIPIs, unless KVM_APIC_INIT was already set in which case it should be a no-op. Something like:

Quoted 6 lines

	e = apic->pending_events;
	while (!(e & KVM_APIC_INIT))
		e = cmpxchg(&apic->pending_events, e,
			    (e | KVM_APIC_INIT) & ~KVM_APIC_SIPI);

If you do this, better make pending_events an atomic_t.

OK (looks ugly but necessary).

Quoted 161 lines

Quoted 151 lines

arch/x86/kvm/lapic.c +32 -7

 			kvm_make_request(KVM_REQ_EVENT, vcpu);
 			kvm_vcpu_kick(vcpu);
 		} else {

@@ -743,13 +743,13 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 	case APIC_DM_STARTUP:
 		apic_debug("SIPI to vcpu %d vector 0x%02x\n",
 			   vcpu->vcpu_id, vector);
-		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
-			result = 1;
-			vcpu->arch.sipi_vector = vector;
-			vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
-			kvm_vcpu_kick(vcpu);
-		}
+		result = 1;
+		apic->sipi_vector = vector;
+		/* make sure sipi_vector is visible for the receiver */
+		smp_wmb();
+		set_bit(KVM_APIC_SIPI, &apic->pending_events);
+		kvm_make_request(KVM_REQ_EVENT, vcpu);
+		kvm_vcpu_kick(vcpu);
 		break;
 
 	case APIC_DM_EXTINT:

@@ -1860,6 +1860,31 @@ int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data)
 					 addr);
 }
 
+bool kvm_apic_has_events(struct kvm_vcpu *vcpu)
+{
+	return kvm_vcpu_has_lapic(vcpu) && vcpu->arch.apic->pending_events;
+}
+
+void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	if (!kvm_vcpu_has_lapic(vcpu))
+		return;
+
+	if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events))
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
+	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+		vcpu->arch.sipi_vector = apic->sipi_vector;
+		pr_debug("vcpu %d received sipi with vector # %x\n",
+			 vcpu->vcpu_id, vcpu->arch.sipi_vector);
+		kvm_lapic_reset(vcpu);
+		kvm_vcpu_reset(vcpu);
+		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+	}
+}
+
 void kvm_lapic_init(void)
 {
 	/* do not patch jump label more than once per second */

arch/x86/kvm/lapic.h +7 -0

index 1676d34..ef3f4ef 100644
@@ -5,6 +5,9 @@
 
 #include <linux/kvm_host.h>
 
+#define KVM_APIC_INIT		0
+#define KVM_APIC_SIPI		1
+
 struct kvm_timer {
 	struct hrtimer timer;
 	s64 period; 				/* unit: ns */

@@ -32,13 +35,17 @@ struct kvm_lapic {
 	void *regs;
 	gpa_t vapic_addr;
 	struct page *vapic_page;
+	unsigned long pending_events;
+	unsigned int sipi_vector;
 };
 int kvm_create_lapic(struct kvm_vcpu *vcpu);
 void kvm_free_lapic(struct kvm_vcpu *vcpu);
 
 int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu);
+bool kvm_apic_has_events(struct kvm_vcpu *vcpu);
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu);
 int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
+void kvm_apic_accept_events(struct kvm_vcpu *vcpu);
 void kvm_lapic_reset(struct kvm_vcpu *vcpu);
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);

arch/x86/kvm/x86.c +9 ~~-13~~

index b891ac3..a0b8041 100644
@@ -162,8 +162,6 @@ u64 __read_mostly host_xcr0;
 
 static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt);
 
-static void kvm_vcpu_reset(struct kvm_vcpu *vcpu);
-
 static inline void kvm_async_pf_hash_reset(struct kvm_vcpu *vcpu)
 {
 	int i;

@@ -5663,6 +5661,11 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	bool req_immediate_exit = 0;
 
 	if (vcpu->requests) {
+		kvm_apic_accept_events(vcpu);
+		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+			r = 1;
+			goto out;
+		}
 		if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
 			kvm_mmu_unload(vcpu);
 		if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))

@@ -5847,14 +5850,6 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 	int r;
 	struct kvm *kvm = vcpu->kvm;
 
-	if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED)) {
-		pr_debug("vcpu %d received sipi with vector # %x\n",
-			 vcpu->vcpu_id, vcpu->arch.sipi_vector);
-		kvm_lapic_reset(vcpu);
-		kvm_vcpu_reset(vcpu);
-		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
-	}
-
 	vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
 	r = vapic_enter(vcpu);
 	if (r) {

@@ -5871,8 +5866,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
 			kvm_vcpu_block(vcpu);
 			vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
-			if (kvm_check_request(KVM_REQ_UNHALT, vcpu))
-			{
+			if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) {
+				kvm_apic_accept_events(vcpu);
 				switch(vcpu->arch.mp_state) {
 				case KVM_MP_STATE_HALTED:
 					vcpu->arch.mp_state =

@@ -5880,7 +5875,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 				case KVM_MP_STATE_RUNNABLE:
 					vcpu->arch.apf.halted = false;
 					break;
-				case KVM_MP_STATE_SIPI_RECEIVED:
+				case KVM_MP_STATE_INIT_RECEIVED:
+					break;
 				default:
 					r = -EINTR;
 					break;

arch/x86/kvm/x86.c static int __vcpu_run(struct kvm_vcpu *vcpu) · line 5901

Quoted 6 lines

arch/x86/kvm/x86.c +2 -1

@@ -5901,7 +5897,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			++vcpu->stat.request_irq_exits;
 		}
 
-		kvm_check_async_pf_completion(vcpu);
+		if (vcpu->arch.mp_state != KVM_MP_STATE_INIT_RECEIVED)
+			kvm_check_async_pf_completion(vcpu);

Can you instead do another change that affects the conditions of kvm_check_async_pf_completion?

Quoted 2 lines

For example, should kvm_arch_interrupt_allowed return zero if the VCPU is in the INIT_RECEIVED state?

Yeah, that probably makes sense beyond async_pf.

Quoted 6 lines

arch/x86/kvm/x86.c +0 -0

 		if (signal_pending(current)) {
 			r = -EINTR;

@@ -6015,6 +6012,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)

Quoted 13 lines

arch/x86/kvm/x86.c int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu, · line 6172

Quoted 11 lines

arch/x86/kvm/x86.c +2 -0

 	if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_UNINITIALIZED)) {
 		kvm_vcpu_block(vcpu);
+		kvm_apic_accept_events(vcpu);
 		clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
 		r = -EAGAIN;
 		goto out;

@@ -6171,6 +6169,7 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
+	kvm_apic_accept_events(vcpu);

Is this racy against __vcpu_run?

No, as Gleb already explained.

Quoted 10 lines

arch/x86/kvm/x86.c +1 -1

 	mp_state->mp_state = vcpu->arch.mp_state;
 	return 0;
 }

@@ -6178,7 +6177,13 @@ int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
-	vcpu->arch.mp_state = mp_state->mp_state;
+	if (mp_state->mp_state == KVM_MP_STATE_SIPI_RECEIVED) {

Quoted 8 lines

arch/x86/kvm/x86.c int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu, · line 6186
Quoted 6 lines
arch/x86/kvm/x86.c +6 -0
+		if (!kvm_vcpu_has_lapic(vcpu))
+			return -EINVAL;
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+		set_bit(KVM_APIC_SIPI, &vcpu->arch.apic->pending_events);
+	} else
+		vcpu->arch.mp_state = mp_state->mp_state;
Should INIT_RECEIVED also be invalid without an in-kernel LAPIC?

Good question. But, then, how do we reset those CPUs (BSPs only)?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Jan Kiszka

author

2013-03-12 13:01 UTC #11 lore raw

Quoted 230 lines · Jan Kiszka

Quoted 59 lines · Paolo Bonzini

Il 12/03/2013 12:44, Jan Kiszka ha scritto:
Quoted 49 lines
A VCPU sending INIT or SIPI to some other VCPU races for setting the remote VCPU's mp_state. When we were unlucky, KVM_MP_STATE_INIT_RECEIVED was overwritten by kvm_emulate_halt and, thus, got lost.
This introduces APIC events for those two signals, keeping them in kvm_apic until kvm_apic_accept_events is run over the target vcpu context. kvm_apic_has_events reports to kvm_arch_vcpu_runnable if there are pending events, thus if vcpu blocking should end.
The patch comes with the side effect of effectively obsoleting KVM_MP_STATE_SIPI_RECEIVED. We still accept it from user space, but immediately translate it to KVM_MP_STATE_INIT_RECEIVED + KVM_APIC_SIPI. The vcpu itself will no longer enter the KVM_MP_STATE_SIPI_RECEIVED state. That also means we no longer exit to user space after receiving a SIPI event.
Signed-off-by Jan Kiszka <[email protected]>
---
This doesn't fix the wrong behaviour on INIT for the BSP yet. Leaving this honor to Paolo.
I didn't try porting any INIT handling for nested on top yet but I think it should be feasible - once we know their semantics for sure, at least on Intel...
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/lapic.c            |   41 +++++++++++++++++++++++++++++++-------
 arch/x86/kvm/lapic.h            |    7 ++++++
 arch/x86/kvm/x86.c              |   39 ++++++++++++++++++++----------------
 4 files changed, 63 insertions(+), 25 deletions(-)
arch/x86/include/asm/kvm_host.h +1 -0
index 348d859..2d28e76 100644
@@ -1002,6 +1002,7 @@ int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v);
 int kvm_cpu_has_interrupt(struct kvm_vcpu *vcpu);
 int kvm_arch_interrupt_allowed(struct kvm_vcpu *vcpu);
 int kvm_cpu_get_interrupt(struct kvm_vcpu *v);
+void kvm_vcpu_reset(struct kvm_vcpu *vcpu);
 
 void kvm_define_shared_msr(unsigned index, u32 msr);
 void kvm_set_shared_msr(unsigned index, u64 val, u64 mask);
arch/x86/kvm/lapic.c +0 -0
index 02b51dd..4a21a6b 100644
arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, · line 734
Quoted 5 lines
arch/x86/kvm/lapic.c +1 -1
@@ -731,7 +731,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 	case APIC_DM_INIT:
 		if (!trig_mode || level) {
 			result = 1;
-			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+			set_bit(KVM_APIC_INIT, &apic->pending_events);
I think this should clear pending SIPIs, unless KVM_APIC_INIT was already set in which case it should be a no-op. Something like:

Quoted 6 lines

	e = apic->pending_events;
	while (!(e & KVM_APIC_INIT))
		e = cmpxchg(&apic->pending_events, e,
			    (e | KVM_APIC_INIT) & ~KVM_APIC_SIPI);

If you do this, better make pending_events an atomic_t.

OK (looks ugly but necessary).

Quoted 161 lines

Quoted 151 lines

arch/x86/kvm/lapic.c +32 -7

 			kvm_make_request(KVM_REQ_EVENT, vcpu);
 			kvm_vcpu_kick(vcpu);
 		} else {

@@ -743,13 +743,13 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 	case APIC_DM_STARTUP:
 		apic_debug("SIPI to vcpu %d vector 0x%02x\n",
 			   vcpu->vcpu_id, vector);
-		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
-			result = 1;
-			vcpu->arch.sipi_vector = vector;
-			vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
-			kvm_vcpu_kick(vcpu);
-		}
+		result = 1;
+		apic->sipi_vector = vector;
+		/* make sure sipi_vector is visible for the receiver */
+		smp_wmb();
+		set_bit(KVM_APIC_SIPI, &apic->pending_events);
+		kvm_make_request(KVM_REQ_EVENT, vcpu);
+		kvm_vcpu_kick(vcpu);
 		break;
 
 	case APIC_DM_EXTINT:

@@ -1860,6 +1860,31 @@ int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data)
 					 addr);
 }
 
+bool kvm_apic_has_events(struct kvm_vcpu *vcpu)
+{
+	return kvm_vcpu_has_lapic(vcpu) && vcpu->arch.apic->pending_events;
+}
+
+void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	if (!kvm_vcpu_has_lapic(vcpu))
+		return;
+
+	if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events))
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
+	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+		vcpu->arch.sipi_vector = apic->sipi_vector;
+		pr_debug("vcpu %d received sipi with vector # %x\n",
+			 vcpu->vcpu_id, vcpu->arch.sipi_vector);
+		kvm_lapic_reset(vcpu);
+		kvm_vcpu_reset(vcpu);
+		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+	}
+}
+
 void kvm_lapic_init(void)
 {
 	/* do not patch jump label more than once per second */

arch/x86/kvm/lapic.h +7 -0

index 1676d34..ef3f4ef 100644
@@ -5,6 +5,9 @@
 
 #include <linux/kvm_host.h>
 
+#define KVM_APIC_INIT		0
+#define KVM_APIC_SIPI		1
+
 struct kvm_timer {
 	struct hrtimer timer;
 	s64 period; 				/* unit: ns */

@@ -32,13 +35,17 @@ struct kvm_lapic {
 	void *regs;
 	gpa_t vapic_addr;
 	struct page *vapic_page;
+	unsigned long pending_events;
+	unsigned int sipi_vector;
 };
 int kvm_create_lapic(struct kvm_vcpu *vcpu);
 void kvm_free_lapic(struct kvm_vcpu *vcpu);
 
 int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu);
+bool kvm_apic_has_events(struct kvm_vcpu *vcpu);
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu);
 int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
+void kvm_apic_accept_events(struct kvm_vcpu *vcpu);
 void kvm_lapic_reset(struct kvm_vcpu *vcpu);
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);

arch/x86/kvm/x86.c +9 ~~-13~~

index b891ac3..a0b8041 100644
@@ -162,8 +162,6 @@ u64 __read_mostly host_xcr0;
 
 static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt);
 
-static void kvm_vcpu_reset(struct kvm_vcpu *vcpu);
-
 static inline void kvm_async_pf_hash_reset(struct kvm_vcpu *vcpu)
 {
 	int i;

@@ -5663,6 +5661,11 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	bool req_immediate_exit = 0;
 
 	if (vcpu->requests) {
+		kvm_apic_accept_events(vcpu);
+		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+			r = 1;
+			goto out;
+		}
 		if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
 			kvm_mmu_unload(vcpu);
 		if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))

@@ -5847,14 +5850,6 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 	int r;
 	struct kvm *kvm = vcpu->kvm;
 
-	if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED)) {
-		pr_debug("vcpu %d received sipi with vector # %x\n",
-			 vcpu->vcpu_id, vcpu->arch.sipi_vector);
-		kvm_lapic_reset(vcpu);
-		kvm_vcpu_reset(vcpu);
-		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
-	}
-
 	vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
 	r = vapic_enter(vcpu);
 	if (r) {

@@ -5871,8 +5866,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
 			kvm_vcpu_block(vcpu);
 			vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
-			if (kvm_check_request(KVM_REQ_UNHALT, vcpu))
-			{
+			if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) {
+				kvm_apic_accept_events(vcpu);
 				switch(vcpu->arch.mp_state) {
 				case KVM_MP_STATE_HALTED:
 					vcpu->arch.mp_state =

@@ -5880,7 +5875,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 				case KVM_MP_STATE_RUNNABLE:
 					vcpu->arch.apf.halted = false;
 					break;
-				case KVM_MP_STATE_SIPI_RECEIVED:
+				case KVM_MP_STATE_INIT_RECEIVED:
+					break;
 				default:
 					r = -EINTR;
 					break;

arch/x86/kvm/x86.c static int __vcpu_run(struct kvm_vcpu *vcpu) · line 5901

Quoted 6 lines

arch/x86/kvm/x86.c +2 -1

@@ -5901,7 +5897,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			++vcpu->stat.request_irq_exits;
 		}
 
-		kvm_check_async_pf_completion(vcpu);
+		if (vcpu->arch.mp_state != KVM_MP_STATE_INIT_RECEIVED)
+			kvm_check_async_pf_completion(vcpu);

Can you instead do another change that affects the conditions of kvm_check_async_pf_completion?

Quoted 4 lines

Quoted 2 lines
For example, should kvm_arch_interrupt_allowed return zero if the VCPU is in the INIT_RECEIVED state?
Yeah, that probably makes sense beyond async_pf.

Wait: If you perform a proper reset on INIT already, we would clear IF thus prevent also async_pf injections. On the other hand, kvm_arch_can_inject_async_page_present returns true if apf.msr_val & KVM_ASYNC_PF_ENABLED is not set - shouldn't that be cleared on reset as well? Hmm...

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Paolo Bonzini

maintainer

2013-03-12 13:13 UTC #12 lore raw

Il 12/03/2013 14:01, Jan Kiszka ha scritto:

Quoted 9 lines

Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
For example, should kvm_arch_interrupt_allowed return zero if the VCPU is in the INIT_RECEIVED state?
Yeah, that probably makes sense beyond async_pf.
Wait: If you perform a proper reset on INIT already, we would clear IF thus prevent also async_pf injections. On the other hand, kvm_arch_can_inject_async_page_present returns true if apf.msr_val & KVM_ASYNC_PF_ENABLED is not set - shouldn't that be cleared on reset as well? Hmm...

So if you split the reset (which is done on INIT) from setting up CS on SIPI, you kill two birds with a stone, or at least one and a half: you clear IF as you wrote above, and INIT on BSP almost works (the only missing bit should be setting the mp_state to KVM_MP_STATE_RUNNABLE).

Paolo

Jan Kiszka

author

2013-03-12 13:16 UTC #13 lore raw

Quoted 11 lines · Paolo Bonzini

Il 12/03/2013 14:01, Jan Kiszka ha scritto:
Quoted 9 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
For example, should kvm_arch_interrupt_allowed return zero if the VCPU is in the INIT_RECEIVED state?
Yeah, that probably makes sense beyond async_pf.
Wait: If you perform a proper reset on INIT already, we would clear IF thus prevent also async_pf injections. On the other hand, kvm_arch_can_inject_async_page_present returns true if apf.msr_val & KVM_ASYNC_PF_ENABLED is not set - shouldn't that be cleared on reset as well? Hmm...

Quoted 4 lines

So if you split the reset (which is done on INIT) from setting up CS on SIPI, you kill two birds with a stone, or at least one and a half: you clear IF as you wrote above, and INIT on BSP almost works (the only missing bit should be setting the mp_state to KVM_MP_STATE_RUNNABLE).

...unless the async_pf MSR is cleared on reset as well.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Gleb Natapov

2013-03-12 13:25 UTC #14 lore raw

Quoted 12 lines · Jan Kiszka

Quoted 11 lines · Paolo Bonzini
Il 12/03/2013 14:01, Jan Kiszka ha scritto:
Quoted 9 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
For example, should kvm_arch_interrupt_allowed return zero if the VCPU is in the INIT_RECEIVED state?
Yeah, that probably makes sense beyond async_pf.
Wait: If you perform a proper reset on INIT already, we would clear IF thus prevent also async_pf injections. On the other hand, kvm_arch_can_inject_async_page_present returns true if apf.msr_val & KVM_ASYNC_PF_ENABLED is not set - shouldn't that be cleared on reset as well? Hmm...

Quoted 7 lines

Quoted 4 lines
So if you split the reset (which is done on INIT) from setting up CS on SIPI, you kill two birds with a stone, or at least one and a half: you clear IF as you wrote above, and INIT on BSP almost works (the only missing bit should be setting the mp_state to KVM_MP_STATE_RUNNABLE).
...unless the async_pf MSR is cleared on reset as well.

It is:
vcpu->arch.apf.msr_val = 0;
--
Gleb.

Jan Kiszka

author

2013-03-12 13:27 UTC #15 lore raw

Quoted 18 lines · Gleb Natapov

Quoted 17 lines · Jan Kiszka
Quoted 15 lines · Paolo Bonzini
Il 12/03/2013 14:01, Jan Kiszka ha scritto:
Quoted 9 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
For example, should kvm_arch_interrupt_allowed return zero if the VCPU is in the INIT_RECEIVED state?
Yeah, that probably makes sense beyond async_pf.
Wait: If you perform a proper reset on INIT already, we would clear IF thus prevent also async_pf injections. On the other hand, kvm_arch_can_inject_async_page_present returns true if apf.msr_val & KVM_ASYNC_PF_ENABLED is not set - shouldn't that be cleared on reset as well? Hmm...
So if you split the reset (which is done on INIT) from setting up CS on SIPI, you kill two birds with a stone, or at least one and a half: you clear IF as you wrote above, and INIT on BSP almost works (the only missing bit should be setting the mp_state to KVM_MP_STATE_RUNNABLE).

Quoted 4 lines

Quoted 2 lines
...unless the async_pf MSR is cleared on reset as well.
It is:
vcpu->arch.apf.msr_val = 0;

Can you explain the "if !(apf.msr_val & KVM_ASYNC_PF_ENABLED) return true;" in kvm_arch_can_inject_async_page_present?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Gleb Natapov

2013-03-12 13:37 UTC #16 lore raw

Quoted 19 lines · Jan Kiszka

Quoted 18 lines · Gleb Natapov
Quoted 17 lines · Jan Kiszka
Quoted 15 lines · Paolo Bonzini
Il 12/03/2013 14:01, Jan Kiszka ha scritto:
Quoted 9 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
For example, should kvm_arch_interrupt_allowed return zero if the VCPU is in the INIT_RECEIVED state?
Yeah, that probably makes sense beyond async_pf.
Wait: If you perform a proper reset on INIT already, we would clear IF thus prevent also async_pf injections. On the other hand, kvm_arch_can_inject_async_page_present returns true if apf.msr_val & KVM_ASYNC_PF_ENABLED is not set - shouldn't that be cleared on reset as well? Hmm...
So if you split the reset (which is done on INIT) from setting up CS on SIPI, you kill two birds with a stone, or at least one and a half: you clear IF as you wrote above, and INIT on BSP almost works (the only missing bit should be setting the mp_state to KVM_MP_STATE_RUNNABLE).

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
...unless the async_pf MSR is cleared on reset as well.
It is:
vcpu->arch.apf.msr_val = 0;
Can you explain the "if !(apf.msr_val & KVM_ASYNC_PF_ENABLED) return true;" in kvm_arch_can_inject_async_page_present?

APF works even with non-PV guests. If non PV guest access swapped out page the page is swapped in asynchronously and vcpu enters apf.halted state. In this sate it can still receive interrupt. Completion of apf cause vcpu exit apf.halted state.

On vcpu reset we call kvm_clear_async_pf_completion_queue() so vcpu->async_pf.done will be empty and kvm_check_async_pf_completion() will be nop if vcpu is properly reset on INIT.

--
Gleb.

Jan Kiszka

2013-03-13 07:39 UTC #17 lore raw

In reply to Paolo Bonzini

Quoted 14 lines · Paolo Bonzini

arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, · line 734
Quoted 10 lines
arch/x86/kvm/lapic.c +1 -1
index 02b51dd..4a21a6b 100644
@@ -731,7 +731,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 	case APIC_DM_INIT:
 		if (!trig_mode || level) {
 			result = 1;
-			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+			set_bit(KVM_APIC_INIT, &apic->pending_events);
I think this should clear pending SIPIs, unless KVM_APIC_INIT was already set in which case it should be a no-op. Something like:

Quoted 6 lines

	e = apic->pending_events;
	while (!(e & KVM_APIC_INIT))
		e = cmpxchg(&apic->pending_events, e,
			    (e | KVM_APIC_INIT) & ~KVM_APIC_SIPI);

If you do this, better make pending_events an atomic_t.

Quick question: Why atomic_t? It becomes a bit ugly to poke into the atomic counter for bitops, and cmpxchg is mapped on an interlocked version.

Jan

Paolo Bonzini

maintainer

2013-03-13 09:03 UTC #18 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 13/03/2013 08:39, Jan Kiszka ha scritto:

Quoted 8 lines

Quoted 8 lines
I think this should clear pending SIPIs, unless KVM_APIC_INIT was already set in which case it should be a no-op. Something like:
e = apic->pending_events; while (!(e & KVM_APIC_INIT)) e = cmpxchg(&apic->pending_events, e, (e | KVM_APIC_INIT) & ~KVM_APIC_SIPI);

Quoted 5 lines

Quoted 1 line
If you do this, better make pending_events an atomic_t.
Quick question: Why atomic_t? It becomes a bit ugly to poke into the atomic counter for bitops, and cmpxchg is mapped on an interlocked version.

It's a bit safer because it forces you to use atomic_read, and generally points out to reviewers to think about memory barriers. But indeed the lack of atomic_set_bit and atomic_clear_bit makes the code uglier. And as long as there are no other bits in apic->pending_events, the above loop is actually the same as just

  apic->pending_events = 1 << KVM_APIC_INIT;

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRQEDHAAoJEBvWZb6bTYbyjhcP/iiky7ZedYXuAA3nI9rVNI79 YUNL9wWzK7EEa7sT54/ky6D5cY+6f/Zjy3TBQexpNscJWJvdPWgU33nA4cA4YnXn cguOUbZ63AKe1a/fFmyc8Kb45Y2tJVJ3tLOH0WZHBh1gPE8BWOCSx/cUncb15oBc CTWS7l6ZjTN5wSJytR/nbGSBvdzBmrlKgMpFb9eiP0lhnMLhrtX/ihdsyLzk8mJV nyTH9LFu3jJoVsSMDn341egx6QGaWiZ/1yzBg8zS9YbbqamxYsUKhBZtF+jwdwR/ hDQsQJuk6d3zJxJysAb1ESQGA18ulKZ2FAa0ozUIn6W1BqKihzW9smZ5gDrs7ZNW zQaraN6swhxvN1UN/Ik1r6esBc2b/LCGtKAgl/Vjm+Kyf5skhVxREUZAGHLOY+Q6 e3TuEAY0N4zLtS4zTB/1tom+MzpWdS2Zrla8pTP8hDGj54vopxCK1fbonv13dwzH hkd+ddzWqRzclDy6MIDNW9e/hzn4g37UMIr4ZkezwciFHz/Oxti5qj8+DOOO0R9r k0CNpyRFwyC+IccS00omdivX9sHW7tbYw2LVKuw4rWvgW5RnGCcRUj9TdxtO1twy 66DhSobVUZr6RHJsYCWtOzyZacKcRHzYGKuizSq9idTRqMpArTVn8jIt69nruG04 LFGiA06mwKP+OGLmmAbN
=Y3al
-----END PGP SIGNATURE-----

Jan Kiszka

author

2013-03-13 09:08 UTC #19 lore raw

Quoted 15 lines · Paolo Bonzini

Il 13/03/2013 08:39, Jan Kiszka ha scritto:
Quoted 8 lines
Quoted 8 lines
I think this should clear pending SIPIs, unless KVM_APIC_INIT was already set in which case it should be a no-op. Something like:
e = apic->pending_events; while (!(e & KVM_APIC_INIT)) e = cmpxchg(&apic->pending_events, e, (e | KVM_APIC_INIT) & ~KVM_APIC_SIPI);
Quoted 5 lines
Quoted 1 line
If you do this, better make pending_events an atomic_t.
Quick question: Why atomic_t? It becomes a bit ugly to poke into the atomic counter for bitops, and cmpxchg is mapped on an interlocked version.

Quoted 7 lines

It's a bit safer because it forces you to use atomic_read, and generally points out to reviewers to think about memory barriers. But indeed the lack of atomic_set_bit and atomic_clear_bit makes the code uglier. And as long as there are no other bits in apic->pending_events, the above loop is actually the same as just
  apic->pending_events = 1 << KVM_APIC_INIT;

Yeah, can simplify this in the next version of the just posted patch.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Gleb Natapov

2013-03-12 13:12 UTC #20 lore raw

In reply to Jan Kiszka

Quoted 79 lines · Jan Kiszka

A VCPU sending INIT or SIPI to some other VCPU races for setting the remote VCPU's mp_state. When we were unlucky, KVM_MP_STATE_INIT_RECEIVED was overwritten by kvm_emulate_halt and, thus, got lost.
This introduces APIC events for those two signals, keeping them in kvm_apic until kvm_apic_accept_events is run over the target vcpu context. kvm_apic_has_events reports to kvm_arch_vcpu_runnable if there are pending events, thus if vcpu blocking should end.
The patch comes with the side effect of effectively obsoleting KVM_MP_STATE_SIPI_RECEIVED. We still accept it from user space, but immediately translate it to KVM_MP_STATE_INIT_RECEIVED + KVM_APIC_SIPI. The vcpu itself will no longer enter the KVM_MP_STATE_SIPI_RECEIVED state. That also means we no longer exit to user space after receiving a SIPI event.
Signed-off-by Jan Kiszka <[email protected]>
---
This doesn't fix the wrong behaviour on INIT for the BSP yet. Leaving this honor to Paolo.
I didn't try porting any INIT handling for nested on top yet but I think it should be feasible - once we know their semantics for sure, at least on Intel...
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/lapic.c            |   41 +++++++++++++++++++++++++++++++-------
 arch/x86/kvm/lapic.h            |    7 ++++++
 arch/x86/kvm/x86.c              |   39 ++++++++++++++++++++----------------
 4 files changed, 63 insertions(+), 25 deletions(-)
arch/x86/include/asm/kvm_host.h +1 -0
index 348d859..2d28e76 100644
@@ -1002,6 +1002,7 @@ int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v);
 int kvm_cpu_has_interrupt(struct kvm_vcpu *vcpu);
 int kvm_arch_interrupt_allowed(struct kvm_vcpu *vcpu);
 int kvm_cpu_get_interrupt(struct kvm_vcpu *v);
+void kvm_vcpu_reset(struct kvm_vcpu *vcpu);
 
 void kvm_define_shared_msr(unsigned index, u32 msr);
 void kvm_set_shared_msr(unsigned index, u64 val, u64 mask);
arch/x86/kvm/lapic.c +8 -8
index 02b51dd..4a21a6b 100644
@@ -731,7 +731,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 	case APIC_DM_INIT:
 		if (!trig_mode || level) {
 			result = 1;
-			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+			set_bit(KVM_APIC_INIT, &apic->pending_events);
 			kvm_make_request(KVM_REQ_EVENT, vcpu);
 			kvm_vcpu_kick(vcpu);
 		} else {
@@ -743,13 +743,13 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 	case APIC_DM_STARTUP:
 		apic_debug("SIPI to vcpu %d vector 0x%02x\n",
 			   vcpu->vcpu_id, vector);
-		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
-			result = 1;
-			vcpu->arch.sipi_vector = vector;
-			vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
-			kvm_vcpu_kick(vcpu);
-		}
+		result = 1;
+		apic->sipi_vector = vector;
+		/* make sure sipi_vector is visible for the receiver */
+		smp_wmb();
+		set_bit(KVM_APIC_SIPI, &apic->pending_events);
+		kvm_make_request(KVM_REQ_EVENT, vcpu);
+		kvm_vcpu_kick(vcpu);
 		break;
 
 	case APIC_DM_EXTINT:

arch/x86/kvm/lapic.c int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data) · line 1865

Quoted 6 lines

arch/x86/kvm/lapic.c +3 -0

@@ -1860,6 +1860,31 @@ int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data)
 					 addr);
 }
 
+bool kvm_apic_has_events(struct kvm_vcpu *vcpu)
+{
+	return kvm_vcpu_has_lapic(vcpu) && vcpu->arch.apic->pending_events;

The function is called only from kvm_arch_vcpu_runnable() and kvm_arch_vcpu_runnable() is called only if irqchip is in kernel, so I think kvm_vcpu_has_lapic() can be dropped.

Quoted 8 lines

arch/x86/kvm/lapic.c +8 -0

+}
+
+void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	if (!kvm_vcpu_has_lapic(vcpu))
+		return;

arch/x86/kvm/lapic.c int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data) · line 1883

Quoted 10 lines

arch/x86/kvm/lapic.c +10 -0

+
+	if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events))
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
+	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+		vcpu->arch.sipi_vector = apic->sipi_vector;
+		pr_debug("vcpu %d received sipi with vector # %x\n",
+			 vcpu->vcpu_id, vcpu->arch.sipi_vector);
+		kvm_lapic_reset(vcpu);
+		kvm_vcpu_reset(vcpu);

Shouldn't we reset on KVM_APIC_INIT?

Quoted 53 lines

arch/x86/kvm/lapic.c +4 -0

+		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+	}
+}
+
 void kvm_lapic_init(void)
 {
 	/* do not patch jump label more than once per second */

arch/x86/kvm/lapic.h +7 -0

index 1676d34..ef3f4ef 100644
@@ -5,6 +5,9 @@
 
 #include <linux/kvm_host.h>
 
+#define KVM_APIC_INIT		0
+#define KVM_APIC_SIPI		1
+
 struct kvm_timer {
 	struct hrtimer timer;
 	s64 period; 				/* unit: ns */

@@ -32,13 +35,17 @@ struct kvm_lapic {
 	void *regs;
 	gpa_t vapic_addr;
 	struct page *vapic_page;
+	unsigned long pending_events;
+	unsigned int sipi_vector;
 };
 int kvm_create_lapic(struct kvm_vcpu *vcpu);
 void kvm_free_lapic(struct kvm_vcpu *vcpu);
 
 int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu);
+bool kvm_apic_has_events(struct kvm_vcpu *vcpu);
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu);
 int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
+void kvm_apic_accept_events(struct kvm_vcpu *vcpu);
 void kvm_lapic_reset(struct kvm_vcpu *vcpu);
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);

arch/x86/kvm/x86.c +0 -2

index b891ac3..a0b8041 100644
@@ -162,8 +162,6 @@ u64 __read_mostly host_xcr0;
 
 static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt);
 
-static void kvm_vcpu_reset(struct kvm_vcpu *vcpu);
-
 static inline void kvm_async_pf_hash_reset(struct kvm_vcpu *vcpu)
 {
 	int i;

arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5668

Quoted 8 lines

arch/x86/kvm/x86.c +5 -0

@@ -5663,6 +5661,11 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	bool req_immediate_exit = 0;
 
 	if (vcpu->requests) {
+		kvm_apic_accept_events(vcpu);
+		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+			r = 1;
+			goto out;
+		}

Why not call it under kvm_check_request(KVM_REQ_EVENT, vcpu) bellow?

Quoted 40 lines

arch/x86/kvm/x86.c +4 ~~-11~~

 		if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
 			kvm_mmu_unload(vcpu);
 		if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))

@@ -5847,14 +5850,6 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 	int r;
 	struct kvm *kvm = vcpu->kvm;
 
-	if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED)) {
-		pr_debug("vcpu %d received sipi with vector # %x\n",
-			 vcpu->vcpu_id, vcpu->arch.sipi_vector);
-		kvm_lapic_reset(vcpu);
-		kvm_vcpu_reset(vcpu);
-		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
-	}
-
 	vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
 	r = vapic_enter(vcpu);
 	if (r) {

@@ -5871,8 +5866,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
 			kvm_vcpu_block(vcpu);
 			vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
-			if (kvm_check_request(KVM_REQ_UNHALT, vcpu))
-			{
+			if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) {
+				kvm_apic_accept_events(vcpu);
 				switch(vcpu->arch.mp_state) {
 				case KVM_MP_STATE_HALTED:
 					vcpu->arch.mp_state =

@@ -5880,7 +5875,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 				case KVM_MP_STATE_RUNNABLE:
 					vcpu->arch.apf.halted = false;
 					break;
-				case KVM_MP_STATE_SIPI_RECEIVED:
+				case KVM_MP_STATE_INIT_RECEIVED:
+					break;
 				default:
 					r = -EINTR;
 					break;

arch/x86/kvm/x86.c static int __vcpu_run(struct kvm_vcpu *vcpu) · line 5901

Quoted 6 lines

arch/x86/kvm/x86.c +2 -1

@@ -5901,7 +5897,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			++vcpu->stat.request_irq_exits;
 		}
 
-		kvm_check_async_pf_completion(vcpu);
+		if (vcpu->arch.mp_state != KVM_MP_STATE_INIT_RECEIVED)
+			kvm_check_async_pf_completion(vcpu);

Do not understand why is this 'if' needed.

Quoted 54 lines

arch/x86/kvm/x86.c +11 -3

 		if (signal_pending(current)) {
 			r = -EINTR;

@@ -6015,6 +6012,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 
 	if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_UNINITIALIZED)) {
 		kvm_vcpu_block(vcpu);
+		kvm_apic_accept_events(vcpu);
 		clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
 		r = -EAGAIN;
 		goto out;

@@ -6171,6 +6169,7 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
+	kvm_apic_accept_events(vcpu);
 	mp_state->mp_state = vcpu->arch.mp_state;
 	return 0;
 }

@@ -6178,7 +6177,13 @@ int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
-	vcpu->arch.mp_state = mp_state->mp_state;
+	if (mp_state->mp_state == KVM_MP_STATE_SIPI_RECEIVED) {
+		if (!kvm_vcpu_has_lapic(vcpu))
+			return -EINVAL;
+		vcpu->arch.mp_state = 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_make_request(KVM_REQ_EVENT, vcpu);
 	return 0;
 }

@@ -6515,7 +6520,7 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
 	kvm_x86_ops->vcpu_free(vcpu);
 }
 
-static void kvm_vcpu_reset(struct kvm_vcpu *vcpu)
+void kvm_vcpu_reset(struct kvm_vcpu *vcpu)
 {
 	atomic_set(&vcpu->arch.nmi_queued, 0);
 	vcpu->arch.nmi_pending = 0;

@@ -6988,7 +6993,7 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
 	return (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE &&
 		!vcpu->arch.apf.halted)
 		|| !list_empty_careful(&vcpu->async_pf.done)
-		|| vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED
+		|| kvm_apic_has_events(vcpu)
 		|| atomic_read(&vcpu->arch.nmi_queued) ||
 		(kvm_arch_interrupt_allowed(vcpu) &&
 		 kvm_cpu_has_interrupt(vcpu));

Signature · 1 line

1.7.3.4

--
Gleb.

Jan Kiszka

author

2013-03-12 13:21 UTC #21 lore raw

Quoted 83 lines · Gleb Natapov

Quoted 82 lines · Jan Kiszka
A VCPU sending INIT or SIPI to some other VCPU races for setting the remote VCPU's mp_state. When we were unlucky, KVM_MP_STATE_INIT_RECEIVED was overwritten by kvm_emulate_halt and, thus, got lost.
This introduces APIC events for those two signals, keeping them in kvm_apic until kvm_apic_accept_events is run over the target vcpu context. kvm_apic_has_events reports to kvm_arch_vcpu_runnable if there are pending events, thus if vcpu blocking should end.
The patch comes with the side effect of effectively obsoleting KVM_MP_STATE_SIPI_RECEIVED. We still accept it from user space, but immediately translate it to KVM_MP_STATE_INIT_RECEIVED + KVM_APIC_SIPI. The vcpu itself will no longer enter the KVM_MP_STATE_SIPI_RECEIVED state. That also means we no longer exit to user space after receiving a SIPI event.
Signed-off-by Jan Kiszka <[email protected]>
---
This doesn't fix the wrong behaviour on INIT for the BSP yet. Leaving this honor to Paolo.
I didn't try porting any INIT handling for nested on top yet but I think it should be feasible - once we know their semantics for sure, at least on Intel...
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/lapic.c            |   41 +++++++++++++++++++++++++++++++-------
 arch/x86/kvm/lapic.h            |    7 ++++++
 arch/x86/kvm/x86.c              |   39 ++++++++++++++++++++----------------
 4 files changed, 63 insertions(+), 25 deletions(-)
arch/x86/include/asm/kvm_host.h +1 -0
index 348d859..2d28e76 100644
@@ -1002,6 +1002,7 @@ int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v);
 int kvm_cpu_has_interrupt(struct kvm_vcpu *vcpu);
 int kvm_arch_interrupt_allowed(struct kvm_vcpu *vcpu);
 int kvm_cpu_get_interrupt(struct kvm_vcpu *v);
+void kvm_vcpu_reset(struct kvm_vcpu *vcpu);
 
 void kvm_define_shared_msr(unsigned index, u32 msr);
 void kvm_set_shared_msr(unsigned index, u64 val, u64 mask);
arch/x86/kvm/lapic.c +8 -8
index 02b51dd..4a21a6b 100644
@@ -731,7 +731,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 	case APIC_DM_INIT:
 		if (!trig_mode || level) {
 			result = 1;
-			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+			set_bit(KVM_APIC_INIT, &apic->pending_events);
 			kvm_make_request(KVM_REQ_EVENT, vcpu);
 			kvm_vcpu_kick(vcpu);
 		} else {
@@ -743,13 +743,13 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 	case APIC_DM_STARTUP:
 		apic_debug("SIPI to vcpu %d vector 0x%02x\n",
 			   vcpu->vcpu_id, vector);
-		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
-			result = 1;
-			vcpu->arch.sipi_vector = vector;
-			vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
-			kvm_vcpu_kick(vcpu);
-		}
+		result = 1;
+		apic->sipi_vector = vector;
+		/* make sure sipi_vector is visible for the receiver */
+		smp_wmb();
+		set_bit(KVM_APIC_SIPI, &apic->pending_events);
+		kvm_make_request(KVM_REQ_EVENT, vcpu);
+		kvm_vcpu_kick(vcpu);
 		break;
 
 	case APIC_DM_EXTINT:
@@ -1860,6 +1860,31 @@ int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data)
 					 addr);
 }
 

Quoted 6 lines

arch/x86/kvm/lapic.c int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data) · line 1865
Quoted 3 lines
arch/x86/kvm/lapic.c +3 -0
+bool kvm_apic_has_events(struct kvm_vcpu *vcpu)
+{
+	return kvm_vcpu_has_lapic(vcpu) && vcpu->arch.apic->pending_events;
The function is called only from kvm_arch_vcpu_runnable() and kvm_arch_vcpu_runnable() is called only if irqchip is in kernel, so I think kvm_vcpu_has_lapic() can be dropped.

Likely, will check again.

Quoted 12 lines

arch/x86/kvm/lapic.c +11 -0

+}
+
+void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	if (!kvm_vcpu_has_lapic(vcpu))
+		return;
+
+	if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events))
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;

Quoted 8 lines

arch/x86/kvm/lapic.c int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data) · line 1883
Quoted 7 lines
arch/x86/kvm/lapic.c +7 -0
+	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
+	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+		vcpu->arch.sipi_vector = apic->sipi_vector;
+		pr_debug("vcpu %d received sipi with vector # %x\n",
+			 vcpu->vcpu_id, vcpu->arch.sipi_vector);
+		kvm_lapic_reset(vcpu);
+		kvm_vcpu_reset(vcpu);
Shouldn't we reset on KVM_APIC_INIT?

It is reset a few lines above. Yes, there could be another INIT pending by now, but that is racing with SIPI anyway and could also arrive a few instruction later. So I don't think we need to worry.

Quoted 56 lines

arch/x86/kvm/lapic.c +4 -0

+		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+	}
+}
+
 void kvm_lapic_init(void)
 {
 	/* do not patch jump label more than once per second */

arch/x86/kvm/lapic.h +7 -0

index 1676d34..ef3f4ef 100644
@@ -5,6 +5,9 @@
 
 #include <linux/kvm_host.h>
 
+#define KVM_APIC_INIT		0
+#define KVM_APIC_SIPI		1
+
 struct kvm_timer {
 	struct hrtimer timer;
 	s64 period; 				/* unit: ns */

@@ -32,13 +35,17 @@ struct kvm_lapic {
 	void *regs;
 	gpa_t vapic_addr;
 	struct page *vapic_page;
+	unsigned long pending_events;
+	unsigned int sipi_vector;
 };
 int kvm_create_lapic(struct kvm_vcpu *vcpu);
 void kvm_free_lapic(struct kvm_vcpu *vcpu);
 
 int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu);
+bool kvm_apic_has_events(struct kvm_vcpu *vcpu);
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu);
 int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
+void kvm_apic_accept_events(struct kvm_vcpu *vcpu);
 void kvm_lapic_reset(struct kvm_vcpu *vcpu);
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);

arch/x86/kvm/x86.c +0 -2

index b891ac3..a0b8041 100644
@@ -162,8 +162,6 @@ u64 __read_mostly host_xcr0;
 
 static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt);
 
-static void kvm_vcpu_reset(struct kvm_vcpu *vcpu);
-
 static inline void kvm_async_pf_hash_reset(struct kvm_vcpu *vcpu)
 {
 	int i;

@@ -5663,6 +5661,11 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	bool req_immediate_exit = 0;

Quoted 7 lines

arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5668
Quoted 6 lines
arch/x86/kvm/x86.c +5 -0
 	if (vcpu->requests) {
+		kvm_apic_accept_events(vcpu);
+		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+			r = 1;
+			goto out;
+		}
Why not call it under kvm_check_request(KVM_REQ_EVENT, vcpu) bellow?

I probably had the desire to handle this event and potential exit reason early. Not sure if it makes any difference.

Quoted 44 lines

arch/x86/kvm/x86.c +4 ~~-11~~

 		if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
 			kvm_mmu_unload(vcpu);
 		if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))

@@ -5847,14 +5850,6 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 	int r;
 	struct kvm *kvm = vcpu->kvm;
 
-	if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED)) {
-		pr_debug("vcpu %d received sipi with vector # %x\n",
-			 vcpu->vcpu_id, vcpu->arch.sipi_vector);
-		kvm_lapic_reset(vcpu);
-		kvm_vcpu_reset(vcpu);
-		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
-	}
-
 	vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
 	r = vapic_enter(vcpu);
 	if (r) {

@@ -5871,8 +5866,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
 			kvm_vcpu_block(vcpu);
 			vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
-			if (kvm_check_request(KVM_REQ_UNHALT, vcpu))
-			{
+			if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) {
+				kvm_apic_accept_events(vcpu);
 				switch(vcpu->arch.mp_state) {
 				case KVM_MP_STATE_HALTED:
 					vcpu->arch.mp_state =

@@ -5880,7 +5875,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 				case KVM_MP_STATE_RUNNABLE:
 					vcpu->arch.apf.halted = false;
 					break;
-				case KVM_MP_STATE_SIPI_RECEIVED:
+				case KVM_MP_STATE_INIT_RECEIVED:
+					break;
 				default:
 					r = -EINTR;
 					break;

@@ -5901,7 +5897,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			++vcpu->stat.request_irq_exits;
 		}

Quoted 4 lines

arch/x86/kvm/x86.c static int __vcpu_run(struct kvm_vcpu *vcpu) · line 5901
Quoted 3 lines
arch/x86/kvm/x86.c +2 -1
-		kvm_check_async_pf_completion(vcpu);
+		if (vcpu->arch.mp_state != KVM_MP_STATE_INIT_RECEIVED)
+			kvm_check_async_pf_completion(vcpu);
Do not understand why is this 'if' needed.

To prevent spurious async-pf event injection and mp-state corruption. See the other mail thread with Paolo on how to avoid this, potentially.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Gleb Natapov

2013-03-12 13:41 UTC #22 lore raw

Quoted 114 lines · Jan Kiszka

Quoted 83 lines · Gleb Natapov
Quoted 82 lines · Jan Kiszka
A VCPU sending INIT or SIPI to some other VCPU races for setting the remote VCPU's mp_state. When we were unlucky, KVM_MP_STATE_INIT_RECEIVED was overwritten by kvm_emulate_halt and, thus, got lost.
This introduces APIC events for those two signals, keeping them in kvm_apic until kvm_apic_accept_events is run over the target vcpu context. kvm_apic_has_events reports to kvm_arch_vcpu_runnable if there are pending events, thus if vcpu blocking should end.
The patch comes with the side effect of effectively obsoleting KVM_MP_STATE_SIPI_RECEIVED. We still accept it from user space, but immediately translate it to KVM_MP_STATE_INIT_RECEIVED + KVM_APIC_SIPI. The vcpu itself will no longer enter the KVM_MP_STATE_SIPI_RECEIVED state. That also means we no longer exit to user space after receiving a SIPI event.
Signed-off-by Jan Kiszka <[email protected]>
---
This doesn't fix the wrong behaviour on INIT for the BSP yet. Leaving this honor to Paolo.
I didn't try porting any INIT handling for nested on top yet but I think it should be feasible - once we know their semantics for sure, at least on Intel...
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/lapic.c            |   41 +++++++++++++++++++++++++++++++-------
 arch/x86/kvm/lapic.h            |    7 ++++++
 arch/x86/kvm/x86.c              |   39 ++++++++++++++++++++----------------
 4 files changed, 63 insertions(+), 25 deletions(-)
arch/x86/include/asm/kvm_host.h +1 -0
index 348d859..2d28e76 100644
@@ -1002,6 +1002,7 @@ int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v);
 int kvm_cpu_has_interrupt(struct kvm_vcpu *vcpu);
 int kvm_arch_interrupt_allowed(struct kvm_vcpu *vcpu);
 int kvm_cpu_get_interrupt(struct kvm_vcpu *v);
+void kvm_vcpu_reset(struct kvm_vcpu *vcpu);
 
 void kvm_define_shared_msr(unsigned index, u32 msr);
 void kvm_set_shared_msr(unsigned index, u64 val, u64 mask);
arch/x86/kvm/lapic.c +8 -8
index 02b51dd..4a21a6b 100644
@@ -731,7 +731,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 	case APIC_DM_INIT:
 		if (!trig_mode || level) {
 			result = 1;
-			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+			set_bit(KVM_APIC_INIT, &apic->pending_events);
 			kvm_make_request(KVM_REQ_EVENT, vcpu);
 			kvm_vcpu_kick(vcpu);
 		} else {
@@ -743,13 +743,13 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 	case APIC_DM_STARTUP:
 		apic_debug("SIPI to vcpu %d vector 0x%02x\n",
 			   vcpu->vcpu_id, vector);
-		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
-			result = 1;
-			vcpu->arch.sipi_vector = vector;
-			vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
-			kvm_vcpu_kick(vcpu);
-		}
+		result = 1;
+		apic->sipi_vector = vector;
+		/* make sure sipi_vector is visible for the receiver */
+		smp_wmb();
+		set_bit(KVM_APIC_SIPI, &apic->pending_events);
+		kvm_make_request(KVM_REQ_EVENT, vcpu);
+		kvm_vcpu_kick(vcpu);
 		break;
 
 	case APIC_DM_EXTINT:
@@ -1860,6 +1860,31 @@ int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data)
 					 addr);
 }
 
Quoted 6 lines
arch/x86/kvm/lapic.c int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data) · line 1865
Quoted 3 lines
arch/x86/kvm/lapic.c +3 -0
+bool kvm_apic_has_events(struct kvm_vcpu *vcpu)
+{
+	return kvm_vcpu_has_lapic(vcpu) && vcpu->arch.apic->pending_events;
The function is called only from kvm_arch_vcpu_runnable() and kvm_arch_vcpu_runnable() is called only if irqchip is in kernel, so I think kvm_vcpu_has_lapic() can be dropped.
Likely, will check again.
Quoted 12 lines
Quoted 12 lines
arch/x86/kvm/lapic.c +11 -0
+}
+
+void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	if (!kvm_vcpu_has_lapic(vcpu))
+		return;
+
+	if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events))
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
Quoted 8 lines
arch/x86/kvm/lapic.c int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data) · line 1883
Quoted 7 lines
arch/x86/kvm/lapic.c +7 -0
+	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
+	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+		vcpu->arch.sipi_vector = apic->sipi_vector;
+		pr_debug("vcpu %d received sipi with vector # %x\n",
+			 vcpu->vcpu_id, vcpu->arch.sipi_vector);
+		kvm_lapic_reset(vcpu);
+		kvm_vcpu_reset(vcpu);
Shouldn't we reset on KVM_APIC_INIT?

Quoted 4 lines

It is reset a few lines above. Yes, there could be another INIT pending by now, but that is racing with SIPI anyway and could also arrive a few instruction later. So I don't think we need to worry.

Not sure I understand. I am saying the code should be:

if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events)) {
	vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
	kvm_lapic_reset(vcpu);
	kvm_vcpu_reset(vcpu);
}
if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
		vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
	vcpu->arch.sipi_vector = apic->sipi_vector;
}

Quoted 64 lines

Quoted 56 lines

arch/x86/kvm/lapic.c +4 -0

+		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+	}
+}
+
 void kvm_lapic_init(void)
 {
 	/* do not patch jump label more than once per second */

arch/x86/kvm/lapic.h +7 -0

index 1676d34..ef3f4ef 100644
@@ -5,6 +5,9 @@
 
 #include <linux/kvm_host.h>
 
+#define KVM_APIC_INIT		0
+#define KVM_APIC_SIPI		1
+
 struct kvm_timer {
 	struct hrtimer timer;
 	s64 period; 				/* unit: ns */

@@ -32,13 +35,17 @@ struct kvm_lapic {
 	void *regs;
 	gpa_t vapic_addr;
 	struct page *vapic_page;
+	unsigned long pending_events;
+	unsigned int sipi_vector;
 };
 int kvm_create_lapic(struct kvm_vcpu *vcpu);
 void kvm_free_lapic(struct kvm_vcpu *vcpu);
 
 int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu);
+bool kvm_apic_has_events(struct kvm_vcpu *vcpu);
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu);
 int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
+void kvm_apic_accept_events(struct kvm_vcpu *vcpu);
 void kvm_lapic_reset(struct kvm_vcpu *vcpu);
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);

arch/x86/kvm/x86.c +0 -2

index b891ac3..a0b8041 100644
@@ -162,8 +162,6 @@ u64 __read_mostly host_xcr0;
 
 static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt);
 
-static void kvm_vcpu_reset(struct kvm_vcpu *vcpu);
-
 static inline void kvm_async_pf_hash_reset(struct kvm_vcpu *vcpu)
 {
 	int i;

@@ -5663,6 +5661,11 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	bool req_immediate_exit = 0;

Quoted 7 lines

arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5668
Quoted 6 lines
arch/x86/kvm/x86.c +5 -0
 	if (vcpu->requests) {
+		kvm_apic_accept_events(vcpu);
+		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+			r = 1;
+			goto out;
+		}
Why not call it under kvm_check_request(KVM_REQ_EVENT, vcpu) bellow?

Quoted 3 lines

I probably had the desire to handle this event and potential exit reason early. Not sure if it makes any difference.

It shouldn't, so better to handle with all other apic events.

--
Gleb.

Paolo Bonzini

maintainer

2013-03-12 13:43 UTC #23 lore raw

Il 12/03/2013 14:41, Gleb Natapov ha scritto:

Quoted 12 lines

Not sure I understand. I am saying the code should be:

if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events)) {
	vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
	kvm_lapic_reset(vcpu);
	kvm_vcpu_reset(vcpu);
}
if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
		vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
	vcpu->arch.sipi_vector = apic->sipi_vector;
}

Yes, this is also discussed in the async_pf thread. But kvm_vcpu_reset is what sets CS based on the sipi_vector, so some more changes are needed (or you can just reset the VCPU twice, but that's ugly...).

Paolo

Gleb Natapov

2013-03-12 13:52 UTC #24 lore raw

Quoted 14 lines · Paolo Bonzini

Il 12/03/2013 14:41, Gleb Natapov ha scritto:

Quoted 12 lines

Not sure I understand. I am saying the code should be:

if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events)) {
	vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
	kvm_lapic_reset(vcpu);
	kvm_vcpu_reset(vcpu);
}
if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
		vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
	vcpu->arch.sipi_vector = apic->sipi_vector;
}

Quoted 4 lines

Yes, this is also discussed in the async_pf thread. But kvm_vcpu_reset is what sets CS based on the sipi_vector, so some more changes are needed (or you can just reset the VCPU twice, but that's ugly...).

Ah correct. We can start from reseting twice and documenting why are we doing it. Then we should move sregs register initialization to common code and factor out CS handling in separate function which will be called on SIPI. Or just call kvm_set_segment() on SIPI.

--
Gleb.