Skip to contentSkip to conversation

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

PATCH Superseded Jan Kiszka 4 participants 6 messages Mar 3, 2013 – Mar 4, 2013

Subsystems (3)
x86 KVM KVM x86

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

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

From: Jan Kiszka <[email protected]>

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.

Fix this by raising requests on the sender side that will then be handled synchronously over the target VCPU context.

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

---

Turned out to be simpler than expected. I'm no longer able to reproduce the race I saw before.

 arch/x86/kvm/lapic.c     |    9 ++++-----
 arch/x86/kvm/x86.c       |   16 +++++++++++++++-
 include/linux/kvm_host.h |    2 ++
 3 files changed, 21 insertions(+), 6 deletions(-)
arch/x86/kvm/lapic.c +4 -5
index 02b51dd..be1e37a 100644
@@ -731,8 +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;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_make_request(KVM_REQ_INIT, vcpu);
 			kvm_vcpu_kick(vcpu);
 		} else {
 			apic_debug("Ignoring de-assert INIT to vcpu %d\n",

@@ -743,11 +742,11 @@ 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) {
+		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED ||
+		    test_bit(KVM_REQ_INIT, &vcpu->requests)) {
 			result = 1;
 			vcpu->arch.sipi_vector = vector;
-			vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_make_request(KVM_REQ_SIPI, vcpu);
 			kvm_vcpu_kick(vcpu);
 		}
 		break;
arch/x86/kvm/x86.c +15 -1
index d0cf737..8c8843c 100644
@@ -5641,6 +5641,18 @@ static void update_eoi_exitmap(struct kvm_vcpu *vcpu)
 	kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
 }
 
+static bool kvm_check_init_and_sipi(struct kvm_vcpu *vcpu)
+{
+	if (kvm_check_request(KVM_REQ_INIT, vcpu))
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+	if (kvm_check_request(KVM_REQ_SIPI, vcpu) &&
+	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+		vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
+		return true;
+	}
+	return false;
+}
+
 static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 {
 	int r;

@@ -5649,6 +5661,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	bool req_immediate_exit = 0;
 
 	if (vcpu->requests) {
+		kvm_check_init_and_sipi(vcpu);
 		if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
 			kvm_mmu_unload(vcpu);
 		if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))

@@ -6977,10 +6990,11 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
 
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
 {
+	if (kvm_check_init_and_sipi(vcpu))
+		return 1;
 	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
 		|| atomic_read(&vcpu->arch.nmi_queued) ||
 		(kvm_arch_interrupt_allowed(vcpu) &&
 		 kvm_cpu_has_interrupt(vcpu));
include/linux/kvm_host.h +2 -0
index 722cae7..1a191c9 100644
@@ -124,6 +124,8 @@ static inline bool is_error_page(struct page *page)
 #define KVM_REQ_MCLOCK_INPROGRESS 20
 #define KVM_REQ_EPR_EXIT          21
 #define KVM_REQ_EOIBITMAP         22
+#define KVM_REQ_INIT              23
+#define KVM_REQ_SIPI              24
 
 #define KVM_USERSPACE_IRQ_SOURCE_ID		0
 #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID	1
Signature · 1 line
1.7.3.4
maintainer
#1 lore raw

Il 03/03/2013 21:21, Jan Kiszka ha scritto:

Quoted 54 lines

From: Jan Kiszka <[email protected]>

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.

Fix this by raising requests on the sender side that will then be handled synchronously over the target VCPU context.

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

---

Turned out to be simpler than expected. I'm no longer able to reproduce the race I saw before.

 arch/x86/kvm/lapic.c     |    9 ++++-----
 arch/x86/kvm/x86.c       |   16 +++++++++++++++-
 include/linux/kvm_host.h |    2 ++
 3 files changed, 21 insertions(+), 6 deletions(-)
arch/x86/kvm/lapic.c +4 -5
index 02b51dd..be1e37a 100644
@@ -731,8 +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;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_make_request(KVM_REQ_INIT, vcpu);
 			kvm_vcpu_kick(vcpu);
 		} else {
 			apic_debug("Ignoring de-assert INIT to vcpu %d\n",

@@ -743,11 +742,11 @@ 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) {
+		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED ||
+		    test_bit(KVM_REQ_INIT, &vcpu->requests)) {
 			result = 1;
 			vcpu->arch.sipi_vector = vector;
-			vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_make_request(KVM_REQ_SIPI, vcpu);
 			kvm_vcpu_kick(vcpu);
 		}
 		break;
arch/x86/kvm/x86.c +0 -0
index d0cf737..8c8843c 100644

arch/x86/kvm/x86.c static void update_eoi_exitmap(struct kvm_vcpu *vcpu) · line 5650

Quoted 10 lines
arch/x86/kvm/x86.c +7 -0
@@ -5641,6 +5641,18 @@ static void update_eoi_exitmap(struct kvm_vcpu *vcpu)
 	kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
 }
 
+static bool kvm_check_init_and_sipi(struct kvm_vcpu *vcpu)
+{
+	if (kvm_check_request(KVM_REQ_INIT, vcpu))
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+	if (kvm_check_request(KVM_REQ_SIPI, vcpu) &&
+	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+		vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;

Do you need KVM_MP_STATE_SIPI_RECEIVED at all anymore? Perhaps you can call kvm_check_init_and_sipi from __vcpu_run, before the call to kvm_vcpu_block (and move the reset from __vcpu_run to kvm_check_init_and_sipi too)? Then you do not even need to touch kvm_arch_vcpu_runnable.

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

Quoted 13 lines
arch/x86/kvm/x86.c +6 -0
+		return true;
+	}
+	return false;
+}
+
 static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 {
 	int r;

@@ -5649,6 +5661,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	bool req_immediate_exit = 0;
 
 	if (vcpu->requests) {
+		kvm_check_init_and_sipi(vcpu);

Does this need to return 1 if kvm_check_init_and_sipi returns 1? Otherwise the guest is entered in INIT state. I think.

Paolo

Quoted 30 lines
arch/x86/kvm/x86.c +2 -1
 		if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
 			kvm_mmu_unload(vcpu);
 		if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))

@@ -6977,10 +6990,11 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
 
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
 {
+	if (kvm_check_init_and_sipi(vcpu))
+		return 1;
 	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
 		|| atomic_read(&vcpu->arch.nmi_queued) ||
 		(kvm_arch_interrupt_allowed(vcpu) &&
 		 kvm_cpu_has_interrupt(vcpu));
include/linux/kvm_host.h +2 -0
index 722cae7..1a191c9 100644
@@ -124,6 +124,8 @@ static inline bool is_error_page(struct page *page)
 #define KVM_REQ_MCLOCK_INPROGRESS 20
 #define KVM_REQ_EPR_EXIT          21
 #define KVM_REQ_EOIBITMAP         22
+#define KVM_REQ_INIT              23
+#define KVM_REQ_SIPI              24
 
 #define KVM_USERSPACE_IRQ_SOURCE_ID		0
 #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID	1
#2 lore raw
Quoted 58 lines · Paolo Bonzini

Il 03/03/2013 21:21, Jan Kiszka ha scritto:

Quoted 57 lines

From: Jan Kiszka <[email protected]>

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.

Fix this by raising requests on the sender side that will then be handled synchronously over the target VCPU context.

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

---

Turned out to be simpler than expected. I'm no longer able to reproduce the race I saw before.

 arch/x86/kvm/lapic.c     |    9 ++++-----
 arch/x86/kvm/x86.c       |   16 +++++++++++++++-
 include/linux/kvm_host.h |    2 ++
 3 files changed, 21 insertions(+), 6 deletions(-)
arch/x86/kvm/lapic.c +4 -5
index 02b51dd..be1e37a 100644
@@ -731,8 +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;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_make_request(KVM_REQ_INIT, vcpu);
 			kvm_vcpu_kick(vcpu);
 		} else {
 			apic_debug("Ignoring de-assert INIT to vcpu %d\n",

@@ -743,11 +742,11 @@ 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) {
+		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED ||
+		    test_bit(KVM_REQ_INIT, &vcpu->requests)) {
 			result = 1;
 			vcpu->arch.sipi_vector = vector;
-			vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_make_request(KVM_REQ_SIPI, vcpu);
 			kvm_vcpu_kick(vcpu);
 		}
 		break;
arch/x86/kvm/x86.c +0 -0
index d0cf737..8c8843c 100644
@@ -5641,6 +5641,18 @@ static void update_eoi_exitmap(struct kvm_vcpu *vcpu)
 	kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
 }
Quoted 13 lines

arch/x86/kvm/x86.c static void update_eoi_exitmap(struct kvm_vcpu *vcpu) · line 5650

Quoted 7 lines
arch/x86/kvm/x86.c +7 -0
+static bool kvm_check_init_and_sipi(struct kvm_vcpu *vcpu)
+{
+	if (kvm_check_request(KVM_REQ_INIT, vcpu))
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+	if (kvm_check_request(KVM_REQ_SIPI, vcpu) &&
+	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+		vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;

Do you need KVM_MP_STATE_SIPI_RECEIVED at all anymore? Perhaps you can call kvm_check_init_and_sipi from __vcpu_run, before the call to kvm_vcpu_block (and move the reset from __vcpu_run to kvm_check_init_and_sipi too)? Then you do not even need to touch kvm_arch_vcpu_runnable.

Haven't thought about this in details yet as I first wanted to fix within the existing infrastructure. But maybe the change below requires more refactoring anyway. Let's see.

Quoted 12 lines
Quoted 12 lines
arch/x86/kvm/x86.c +5 -0
+		return true;
+	}
+	return false;
+}
+
 static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 {
 	int r;

@@ -5649,6 +5661,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	bool req_immediate_exit = 0;
Quoted 5 lines

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

Quoted 2 lines
arch/x86/kvm/x86.c +1 -0
 	if (vcpu->requests) {
+		kvm_check_init_and_sipi(vcpu);

Does this need to return 1 if kvm_check_init_and_sipi returns 1? Otherwise the guest is entered in INIT state. I think.

Hmm, true... Need to refactor things a bit more as kvm_check_init_and_sipi is designed to return true only for wait-on-sipi->runnable transition.

Thanks,
Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
author
#3 lore raw
In reply to Paolo Bonzini
Quoted 58 lines · Paolo Bonzini

Il 03/03/2013 21:21, Jan Kiszka ha scritto:

Quoted 57 lines

From: Jan Kiszka <[email protected]>

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.

Fix this by raising requests on the sender side that will then be handled synchronously over the target VCPU context.

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

---

Turned out to be simpler than expected. I'm no longer able to reproduce the race I saw before.

 arch/x86/kvm/lapic.c     |    9 ++++-----
 arch/x86/kvm/x86.c       |   16 +++++++++++++++-
 include/linux/kvm_host.h |    2 ++
 3 files changed, 21 insertions(+), 6 deletions(-)
arch/x86/kvm/lapic.c +4 -5
index 02b51dd..be1e37a 100644
@@ -731,8 +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;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_make_request(KVM_REQ_INIT, vcpu);
 			kvm_vcpu_kick(vcpu);
 		} else {
 			apic_debug("Ignoring de-assert INIT to vcpu %d\n",

@@ -743,11 +742,11 @@ 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) {
+		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED ||
+		    test_bit(KVM_REQ_INIT, &vcpu->requests)) {
 			result = 1;
 			vcpu->arch.sipi_vector = vector;
-			vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_make_request(KVM_REQ_SIPI, vcpu);
 			kvm_vcpu_kick(vcpu);
 		}
 		break;
arch/x86/kvm/x86.c +0 -0
index d0cf737..8c8843c 100644
@@ -5641,6 +5641,18 @@ static void update_eoi_exitmap(struct kvm_vcpu *vcpu)
 	kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
 }
Quoted 9 lines

arch/x86/kvm/x86.c static void update_eoi_exitmap(struct kvm_vcpu *vcpu) · line 5650

Quoted 7 lines
arch/x86/kvm/x86.c +7 -0
+static bool kvm_check_init_and_sipi(struct kvm_vcpu *vcpu)
+{
+	if (kvm_check_request(KVM_REQ_INIT, vcpu))
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+	if (kvm_check_request(KVM_REQ_SIPI, vcpu) &&
+	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+		vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;

Do you need KVM_MP_STATE_SIPI_RECEIVED at all anymore?

Unfortunately, we cannot kill it as it was user-visible:

When a VCPU receives KVM_MP_STATE_SIPI_RECEIVED, it leaves __vcpu_run with -EINTR and, thus, KVM_RUN. We actually return to userspace, allowing it to see this mp_state and also migrate the guest in this state.

I could avoid this userspace exit (not sure what it is good for) but we will have to keep the logic to accept and convert the state into KVM_MP_STATE_RUNNABLE. So there is not much to simplify here, I'm afraid.

Jan

#4 lore raw
In reply to Jan Kiszka
Quoted 83 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

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.

Fix this by raising requests on the sender side that will then be handled synchronously over the target VCPU context.

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

---

Turned out to be simpler than expected. I'm no longer able to reproduce the race I saw before.

 arch/x86/kvm/lapic.c     |    9 ++++-----
 arch/x86/kvm/x86.c       |   16 +++++++++++++++-
 include/linux/kvm_host.h |    2 ++
 3 files changed, 21 insertions(+), 6 deletions(-)
arch/x86/kvm/lapic.c +4 -5
index 02b51dd..be1e37a 100644
@@ -731,8 +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;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_make_request(KVM_REQ_INIT, vcpu);
 			kvm_vcpu_kick(vcpu);
 		} else {
 			apic_debug("Ignoring de-assert INIT to vcpu %d\n",

@@ -743,11 +742,11 @@ 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) {
+		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED ||
+		    test_bit(KVM_REQ_INIT, &vcpu->requests)) {
 			result = 1;
 			vcpu->arch.sipi_vector = vector;
-			vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_make_request(KVM_REQ_SIPI, vcpu);
 			kvm_vcpu_kick(vcpu);
 		}
 		break;
arch/x86/kvm/x86.c +13 -0
index d0cf737..8c8843c 100644
@@ -5641,6 +5641,18 @@ static void update_eoi_exitmap(struct kvm_vcpu *vcpu)
 	kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
 }
 
+static bool kvm_check_init_and_sipi(struct kvm_vcpu *vcpu)
+{
+	if (kvm_check_request(KVM_REQ_INIT, vcpu))
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+	if (kvm_check_request(KVM_REQ_SIPI, vcpu) &&
+	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+		vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
+		return true;
+	}
+	return false;
+}
+
 static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 {
 	int r;

@@ -5649,6 +5661,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	bool req_immediate_exit = 0;
 
 	if (vcpu->requests) {
+		kvm_check_init_and_sipi(vcpu);
 		if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
 			kvm_mmu_unload(vcpu);
 		if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))

@@ -6977,10 +6990,11 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
 
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)

arch/x86/kvm/x86.c void kvm_arch_flush_shadow_memslot(struct kvm *kvm, · line 7000

Quoted 10 lines
arch/x86/kvm/x86.c +2 -1
 {
+	if (kvm_check_init_and_sipi(vcpu))
+		return 1;
 	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
 		|| atomic_read(&vcpu->arch.nmi_queued) ||
 		(kvm_arch_interrupt_allowed(vcpu) &&
 		 kvm_cpu_has_interrupt(vcpu));

This makes two subsequent calls to kvm_arch_vcpu_runnable() return different values if SIPI is pending. While it may not cause problem to current code (I haven't thought it through) with such semantics you gonna have a bad time.

--
Gleb.

#5 lore raw
Quoted 83 lines · Gleb Natapov
Quoted 82 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

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.

Fix this by raising requests on the sender side that will then be handled synchronously over the target VCPU context.

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

---

Turned out to be simpler than expected. I'm no longer able to reproduce the race I saw before.

 arch/x86/kvm/lapic.c     |    9 ++++-----
 arch/x86/kvm/x86.c       |   16 +++++++++++++++-
 include/linux/kvm_host.h |    2 ++
 3 files changed, 21 insertions(+), 6 deletions(-)
arch/x86/kvm/lapic.c +4 -5
index 02b51dd..be1e37a 100644
@@ -731,8 +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;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_make_request(KVM_REQ_INIT, vcpu);
 			kvm_vcpu_kick(vcpu);
 		} else {
 			apic_debug("Ignoring de-assert INIT to vcpu %d\n",

@@ -743,11 +742,11 @@ 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) {
+		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED ||
+		    test_bit(KVM_REQ_INIT, &vcpu->requests)) {
 			result = 1;
 			vcpu->arch.sipi_vector = vector;
-			vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_make_request(KVM_REQ_SIPI, vcpu);
 			kvm_vcpu_kick(vcpu);
 		}
 		break;
arch/x86/kvm/x86.c +13 -0
index d0cf737..8c8843c 100644
@@ -5641,6 +5641,18 @@ static void update_eoi_exitmap(struct kvm_vcpu *vcpu)
 	kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
 }
 
+static bool kvm_check_init_and_sipi(struct kvm_vcpu *vcpu)
+{
+	if (kvm_check_request(KVM_REQ_INIT, vcpu))
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+	if (kvm_check_request(KVM_REQ_SIPI, vcpu) &&
+	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+		vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
+		return true;
+	}
+	return false;
+}
+
 static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 {
 	int r;

@@ -5649,6 +5661,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	bool req_immediate_exit = 0;
 
 	if (vcpu->requests) {
+		kvm_check_init_and_sipi(vcpu);
 		if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
 			kvm_mmu_unload(vcpu);
 		if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))

@@ -6977,10 +6990,11 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
Quoted 15 lines

arch/x86/kvm/x86.c void kvm_arch_flush_shadow_memslot(struct kvm *kvm, · line 7000

Quoted 11 lines
arch/x86/kvm/x86.c +2 -1
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
 {
+	if (kvm_check_init_and_sipi(vcpu))
+		return 1;
 	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
 		|| atomic_read(&vcpu->arch.nmi_queued) ||
 		(kvm_arch_interrupt_allowed(vcpu) &&
 		 kvm_cpu_has_interrupt(vcpu));

This makes two subsequent calls to kvm_arch_vcpu_runnable() return different values if SIPI is pending. While it may not cause problem to current code (I haven't thought it through) with such semantics you gonna have a bad time.

If I manage to follow Paolo's suggestion to eliminate the SIPI_RECEIVED state and all the staged logic around it, that might change. Will be more invasive but likely cleaner in its result.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux