KVM: x86: do not execute halted vcpus (resend)

PATCH Marcelo Tosatti 2 participants 5 messages Sep 8, 2008 – Sep 10, 2008

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Marcelo Tosatti

author

2008-09-08 18:24 UTC #0 lore raw

Offline or uninitialized vcpu's can be executed if requested to perform userspace work.

Follow Avi's suggestion to handle halted vcpu's in the main loop, simplifying kvm_emulate_halt(). Introduce a new vcpu->requests bit to indicate events that promote state from halted to running.

Also standardize vcpu wake sites.

Avi, please confirm that this does not break Windows reboot (which I can't reproduce).

Signed-off-by Marcelo Tosatti <mtosatti <at> redhat.com>

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

index 4cb4430..634132a 100644
@@ -200,10 +200,9 @@ static int __pit_timer_fn(struct kvm_kpit_state *ps)
 
 	if (!atomic_inc_and_test(&pt->pending))
 		set_bit(KVM_REQ_PENDING_TIMER, &vcpu0->requests);
-	if (vcpu0 && waitqueue_active(&vcpu0->wq)) {
-		vcpu0->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+
+	if (vcpu0 && waitqueue_active(&vcpu0->wq))
 		wake_up_interruptible(&vcpu0->wq);
-	}
 
 	pt->timer.expires = ktime_add_ns(pt->timer.expires, pt->period);
 	pt->scheduled = ktime_to_ns(pt->timer.expires);

arch/x86/kvm/lapic.c +4 ~~-12~~

index be94f93..fd00f69 100644
@@ -339,13 +339,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 		} else
 			apic_clear_vector(vector, apic->regs + APIC_TMR);
 
-		if (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE)
-			kvm_vcpu_kick(vcpu);
-		else if (vcpu->arch.mp_state == KVM_MP_STATE_HALTED) {
-			vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
-			if (waitqueue_active(&vcpu->wq))
-				wake_up_interruptible(&vcpu->wq);
-		}
+		kvm_vcpu_kick(vcpu);
 
 		result = (orig_irr == 0);
 		break;

@@ -384,8 +378,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
 			vcpu->arch.sipi_vector = vector;
 			vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
-			if (waitqueue_active(&vcpu->wq))
-				wake_up_interruptible(&vcpu->wq);
+			kvm_vcpu_kick(vcpu);
 		}
 		break;

@@ -950,10 +943,9 @@ static int __apic_timer_fn(struct kvm_lapic *apic)
 
 	if(!atomic_inc_and_test(&apic->timer.pending))
 		set_bit(KVM_REQ_PENDING_TIMER, &apic->vcpu->requests);
-	if (waitqueue_active(q)) {
-		apic->vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+	if (waitqueue_active(q))
 		wake_up_interruptible(q);
-	}
+
 	if (apic_lvtt_period(apic)) {
 		result = 1;
 		apic->timer.dev.expires = ktime_add_ns(

arch/x86/kvm/x86.c +55 ~~-45~~

144 lines not on this page · raw message

include/linux/kvm_host.h +1 -0

index a18aaad..4b03643 100644
@@ -34,6 +34,7 @@
 #define KVM_REQ_MMU_RELOAD         3
 #define KVM_REQ_TRIPLE_FAULT       4
 #define KVM_REQ_PENDING_TIMER      5
+#define KVM_REQ_UNHALT             6
 
 struct kvm_vcpu;
 extern struct kmem_cache *kvm_vcpu_cache;

virt/kvm/kvm_main.c +5 -5

index de3b029..63e661b 100644
@@ -980,12 +980,12 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 	for (;;) {
 		prepare_to_wait(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
 
-		if (kvm_cpu_has_interrupt(vcpu))
-			break;
-		if (kvm_cpu_has_pending_timer(vcpu))
-			break;
-		if (kvm_arch_vcpu_runnable(vcpu))
+		if (kvm_cpu_has_interrupt(vcpu) ||
+		    kvm_cpu_has_pending_timer(vcpu) ||
+		    kvm_arch_vcpu_runnable(vcpu)) {
+			set_bit(KVM_REQ_UNHALT, &vcpu->requests);
 			break;
+		}
 		if (signal_pending(current))
 			break;

Avi Kivity

2008-09-09 14:15 UTC #1 lore raw

Quoted 12 lines · Marcelo Tosatti

Offline or uninitialized vcpu's can be executed if requested to perform userspace work.
Follow Avi's suggestion to handle halted vcpu's in the main loop, simplifying kvm_emulate_halt(). Introduce a new vcpu->requests bit to indicate events that promote state from halted to running.
Also standardize vcpu wake sites.
Avi, please confirm that this does not break Windows reboot (which I can't reproduce).

I couldn't reproduce it either (probably mistested earlier), so I applied it. Sorry about the drops.

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2008-09-10 12:04 UTC #2 lore raw

Quoted 7 lines · Avi Kivity

Quoted 6 lines · Marcelo Tosatti
Offline or uninitialized vcpu's can be executed if requested to perform userspace work.
Follow Avi's suggestion to handle halted vcpu's in the main loop, simplifying kvm_emulate_halt(). Introduce a new vcpu->requests bit to indicate events that promote state from halted to running.

Quoted 8 lines

Quoted 5 lines
Also standardize vcpu wake sites.
Avi, please confirm that this does not break Windows reboot (which I can't reproduce).
I couldn't reproduce it either (probably mistested earlier), so I applied it. Sorry about the drops.

Actually it does reproduce. If you reboot twice, the second reboot will hang during reset (with cs:ip = f000:fff0). Plain Windows XP.

Signature · 1 line

error compiling committee.c: too many arguments to function

Marcelo Tosatti

author

2008-09-10 18:38 UTC #3 lore raw

Quoted 15 lines · Avi Kivity

Quoted 14 lines · Avi Kivity
Quoted 12 lines · Marcelo Tosatti
Offline or uninitialized vcpu's can be executed if requested to perform userspace work.
Follow Avi's suggestion to handle halted vcpu's in the main loop, simplifying kvm_emulate_halt(). Introduce a new vcpu->requests bit to indicate events that promote state from halted to running.
Also standardize vcpu wake sites.
Avi, please confirm that this does not break Windows reboot (which I can't
reproduce).

Quoted 6 lines

Quoted 3 lines
I couldn't reproduce it either (probably mistested earlier), so I applied it. Sorry about the drops.
Actually it does reproduce. If you reboot twice, the second reboot will hang during reset (with cs:ip = f000:fff0). Plain Windows XP.

OK, easily reproducible by adding a sleep at the start of qemu_kvm_system_reset.

The problem is that the vcpu HLT's between the KBD ioport write and the actual system reset by the IO thread. By then time there will be no event that takes it out of STATE_HALTED.

The following fixes it, however older userspace will remain borked. Alternative is to special case f000:fff0 in-kernel. What is preferred?

libkvm/libkvm.h +7 -1

index 346eedf..f3790fa 100644
@@ -330,7 +330,13 @@ int kvm_set_mpstate(kvm_context_t kvm, int vcpu,
  */
 static inline int kvm_reset_mpstate(kvm_context_t kvm, int vcpu)
 {
-    struct kvm_mp_state mp_state = {.mp_state = KVM_MP_STATE_UNINITIALIZED};
+    struct kvm_mp_state mp_state;
+
+    if (vcpu)
+        mp_state.mp_state = KVM_MP_STATE_UNINITIALIZED;
+    else
+        mp_state.mp_state = KVM_MP_STATE_RUNNABLE;
+
     return kvm_set_mpstate(kvm, vcpu, &mp_state);
 }
 #endif

qemu/qemu-kvm-x86.c +6 -8

index 5daedd1..9983e5a 100644
@@ -673,15 +673,13 @@ int handle_tpr_access(void *opaque, int vcpu,
 void kvm_arch_cpu_reset(CPUState *env)
 {
     kvm_arch_load_regs(env);
-    if (env->cpu_index != 0) {
-	if (kvm_irqchip_in_kernel(kvm_context)) {
+    if (kvm_irqchip_in_kernel(kvm_context)) {
 #ifdef KVM_CAP_MP_STATE
-	    kvm_reset_mpstate(kvm_context, env->cpu_index);
+        kvm_reset_mpstate(kvm_context, env->cpu_index);
 #endif
-	} else {
-	    env->interrupt_request &= ~CPU_INTERRUPT_HARD;
-	    env->halted = 1;
-	    env->exception_index = EXCP_HLT;
-	}
+    } else if (env->cpu_index != 0) {
+        env->interrupt_request &= ~CPU_INTERRUPT_HARD;
+        env->halted = 1;
+        env->exception_index = EXCP_HLT;
     }
 }

Avi Kivity

2008-09-10 18:44 UTC #4 lore raw

Quoted 7 lines · Marcelo Tosatti

Quoted 3 lines
Actually it does reproduce. If you reboot twice, the second reboot will hang during reset (with cs:ip = f000:fff0). Plain Windows XP.
OK, easily reproducible by adding a sleep at the start of qemu_kvm_system_reset.

Quoted 8 lines

The problem is that the vcpu HLT's between the KBD ioport write and the actual system reset by the IO thread. By then time there will be no event that takes it out of STATE_HALTED.
The following fixes it, however older userspace will remain borked. Alternative is to special case f000:fff0 in-kernel. What is preferred?

Special-case the kvm_set_sregs ioctl with cs:eip f000:fff0 (and cr0.pe=0 for good measure).

Signature · 1 line

Do not meddle in the internals of kernels, for they are subtle and quick to panic.