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

[Marcelo Tosatti <mtosatti@redhat.com>]

On Wed, Sep 10, 2008 at 03:04:10PM +0300, Avi Kivity wrote:
> Avi Kivity wrote:
>> Marcelo Tosatti wrote:
>>> 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.
>>
>
> 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?

diff --git a/libkvm/libkvm.h b/libkvm/libkvm.h
index 346eedf..f3790fa 100644
--- a/libkvm/libkvm.h
+++ b/libkvm/libkvm.h
@@ -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
diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
index 5daedd1..9983e5a 100644
--- a/qemu/qemu-kvm-x86.c
+++ b/qemu/qemu-kvm-x86.c
@@ -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;
     }
 }