Patchwork [v3,04/10] qemu-kvm: Clean up mpstate synchronization

login
register
mail settings
Submitter Jan Kiszka
Date Feb. 24, 2010, 2:17 p.m.
Message ID <4f4d544e5c032561bca4efa483084451683b22fd.1267021065.git.jan.kiszka@siemens.com>
Download mbox | patch
Permalink /patch/46129/
State New
Headers show

Comments

Jan Kiszka - Feb. 24, 2010, 2:17 p.m.
Push mpstate reading/writing into kvm_arch_load/save_regs and, on x86,
properly synchronize with halted in the accessor functions.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/apic.c             |    7 ---
 qemu-kvm-ia64.c       |    4 +-
 qemu-kvm-x86.c        |  102 ++++++++++++++++++++++++++++++-------------------
 qemu-kvm.c            |   30 --------------
 qemu-kvm.h            |   15 -------
 target-i386/machine.c |    6 ---
 target-ia64/machine.c |    3 +
 7 files changed, 69 insertions(+), 98 deletions(-)
Marcelo Tosatti - Feb. 24, 2010, 10:44 p.m.
On Wed, Feb 24, 2010 at 03:17:52PM +0100, Jan Kiszka wrote:
> Push mpstate reading/writing into kvm_arch_load/save_regs and, on x86,
> properly synchronize with halted in the accessor functions.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

> @@ -1290,6 +1318,7 @@ int kvm_arch_init_vcpu(CPUState *cenv)
>  #ifdef KVM_EXIT_TPR_ACCESS
>      kvm_tpr_vcpu_start(cenv);
>  #endif
> +    kvm_reset_mpstate(cenv);
>      return 0;
>  }
>  
> @@ -1363,15 +1392,10 @@ void kvm_arch_cpu_reset(CPUState *env)
>  {
>      kvm_arch_reset_vcpu(env);
>      kvm_put_vcpu_events(env);
> -    if (!cpu_is_bsp(env)) {
> -	if (kvm_irqchip_in_kernel()) {
> -#ifdef KVM_CAP_MP_STATE
> -	    kvm_reset_mpstate(env);
> -#endif
> -	} else {
> -	    env->interrupt_request &= ~CPU_INTERRUPT_HARD;
> -	    env->halted = 1;
> -	}
> +    kvm_reset_mpstate(env);
> +    if (!cpu_is_bsp(env) && !kvm_irqchip_in_kernel()) {
> +        env->interrupt_request &= ~CPU_INTERRUPT_HARD;
> +        env->halted = 1;
>      }
>  }

Why are these two needed? Now that initialization of mp_state 
happens via synchronize_state(init/reset) -> arch_load_regs?
Jan Kiszka - Feb. 25, 2010, 12:02 a.m.
Marcelo Tosatti wrote:
> On Wed, Feb 24, 2010 at 03:17:52PM +0100, Jan Kiszka wrote:
>> Push mpstate reading/writing into kvm_arch_load/save_regs and, on x86,
>> properly synchronize with halted in the accessor functions.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
>> @@ -1290,6 +1318,7 @@ int kvm_arch_init_vcpu(CPUState *cenv)
>>  #ifdef KVM_EXIT_TPR_ACCESS
>>      kvm_tpr_vcpu_start(cenv);
>>  #endif
>> +    kvm_reset_mpstate(cenv);
>>      return 0;
>>  }
>>  
>> @@ -1363,15 +1392,10 @@ void kvm_arch_cpu_reset(CPUState *env)
>>  {
>>      kvm_arch_reset_vcpu(env);
>>      kvm_put_vcpu_events(env);
>> -    if (!cpu_is_bsp(env)) {
>> -	if (kvm_irqchip_in_kernel()) {
>> -#ifdef KVM_CAP_MP_STATE
>> -	    kvm_reset_mpstate(env);
>> -#endif
>> -	} else {
>> -	    env->interrupt_request &= ~CPU_INTERRUPT_HARD;
>> -	    env->halted = 1;
>> -	}
>> +    kvm_reset_mpstate(env);
>> +    if (!cpu_is_bsp(env) && !kvm_irqchip_in_kernel()) {
>> +        env->interrupt_request &= ~CPU_INTERRUPT_HARD;
>> +        env->halted = 1;
>>      }
>>  }
> 
> Why are these two needed? Now that initialization of mp_state 
> happens via synchronize_state(init/reset) -> arch_load_regs?

Maybe correct. env->halted is also reset on load, not sure about the
interrupt_request reset impact yet. This is (or at least was) hairy
stuff, /me has to sleep about it again.

Jan
Jan Kiszka - Feb. 25, 2010, 11:56 a.m.
Jan Kiszka wrote:
> Marcelo Tosatti wrote:
>> On Wed, Feb 24, 2010 at 03:17:52PM +0100, Jan Kiszka wrote:
>>> Push mpstate reading/writing into kvm_arch_load/save_regs and, on x86,
>>> properly synchronize with halted in the accessor functions.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> @@ -1290,6 +1318,7 @@ int kvm_arch_init_vcpu(CPUState *cenv)
>>>  #ifdef KVM_EXIT_TPR_ACCESS
>>>      kvm_tpr_vcpu_start(cenv);
>>>  #endif
>>> +    kvm_reset_mpstate(cenv);
>>>      return 0;
>>>  }
>>>  
>>> @@ -1363,15 +1392,10 @@ void kvm_arch_cpu_reset(CPUState *env)
>>>  {
>>>      kvm_arch_reset_vcpu(env);
>>>      kvm_put_vcpu_events(env);
>>> -    if (!cpu_is_bsp(env)) {
>>> -	if (kvm_irqchip_in_kernel()) {
>>> -#ifdef KVM_CAP_MP_STATE
>>> -	    kvm_reset_mpstate(env);
>>> -#endif
>>> -	} else {
>>> -	    env->interrupt_request &= ~CPU_INTERRUPT_HARD;
>>> -	    env->halted = 1;
>>> -	}
>>> +    kvm_reset_mpstate(env);
>>> +    if (!cpu_is_bsp(env) && !kvm_irqchip_in_kernel()) {
>>> +        env->interrupt_request &= ~CPU_INTERRUPT_HARD;
>>> +        env->halted = 1;
>>>      }
>>>  }
>> Why are these two needed? Now that initialization of mp_state 
>> happens via synchronize_state(init/reset) -> arch_load_regs?
> 
> Maybe correct. env->halted is also reset on load, not sure about the
> interrupt_request reset impact yet. This is (or at least was) hairy
> stuff, /me has to sleep about it again.

Regarding the !irqchip bits: env->halted is actually already reset in
apic_init_reset and can be dropped. I still wonder why I (was surprised
to find this out) once introduced the interrupt_request reset,
specifically why we may need it with KVM not not with TCG. Maybe we do,
and this should better be addressed generically.

But did you also ask about the need for kvm_reset_mpstate? That function
solely brings env->mp_state into the proper state after reset, something
that does not happen elsewhere.

Jan

Patch

diff --git a/hw/apic.c b/hw/apic.c
index 3e03e10..092c61e 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -507,13 +507,6 @@  void apic_init_reset(CPUState *env)
     s->wait_for_sipi = 1;
 
     env->halted = !(s->apicbase & MSR_IA32_APICBASE_BSP);
-#ifdef KVM_CAP_MP_STATE
-    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
-        env->mp_state
-            = env->halted ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
-        kvm_load_mpstate(env);
-    }
-#endif
 }
 
 static void apic_startup(APICState *s, int vector_num)
diff --git a/qemu-kvm-ia64.c b/qemu-kvm-ia64.c
index fc8110e..39bcbeb 100644
--- a/qemu-kvm-ia64.c
+++ b/qemu-kvm-ia64.c
@@ -124,7 +124,9 @@  void kvm_arch_cpu_reset(CPUState *env)
 {
     if (kvm_irqchip_in_kernel(kvm_context)) {
 #ifdef KVM_CAP_MP_STATE
-	kvm_reset_mpstate(env->kvm_cpu_state.vcpu_ctx);
+        struct kvm_mp_state mp_state = {.mp_state = KVM_MP_STATE_UNINITIALIZED
+        };
+        kvm_set_mpstate(env, &mp_state);
 #endif
     } else {
 	env->interrupt_request &= ~CPU_INTERRUPT_HARD;
diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index 47667b3..4e6ae70 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -754,6 +754,56 @@  static int get_msr_entry(struct kvm_msr_entry *entry, CPUState *env)
         return 0;
 }
 
+static void kvm_arch_save_mpstate(CPUState *env)
+{
+#ifdef KVM_CAP_MP_STATE
+    int r;
+    struct kvm_mp_state mp_state;
+
+    r = kvm_get_mpstate(env, &mp_state);
+    if (r < 0) {
+        env->mp_state = -1;
+    } else {
+        env->mp_state = mp_state.mp_state;
+        if (kvm_irqchip_in_kernel()) {
+            env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
+        }
+    }
+#else
+    env->mp_state = -1;
+#endif
+}
+
+static void kvm_arch_load_mpstate(CPUState *env)
+{
+#ifdef KVM_CAP_MP_STATE
+    struct kvm_mp_state mp_state;
+
+    /*
+     * -1 indicates that the host did not support GET_MP_STATE ioctl,
+     *  so don't touch it.
+     */
+    if (env->mp_state != -1) {
+        mp_state.mp_state = env->mp_state;
+        kvm_set_mpstate(env, &mp_state);
+    }
+#endif
+}
+
+static void kvm_reset_mpstate(CPUState *env)
+{
+#ifdef KVM_CAP_MP_STATE
+    if (kvm_check_extension(kvm_state, KVM_CAP_MP_STATE)) {
+        if (kvm_irqchip_in_kernel()) {
+            env->mp_state = cpu_is_bsp(env) ? KVM_MP_STATE_RUNNABLE :
+                                              KVM_MP_STATE_UNINITIALIZED;
+        } else {
+            env->mp_state = KVM_MP_STATE_RUNNABLE;
+        }
+    }
+#endif
+}
+
 static void set_v8086_seg(struct kvm_segment *lhs, const SegmentCache *rhs)
 {
     lhs->selector = rhs->selector;
@@ -922,6 +972,14 @@  void kvm_arch_load_regs(CPUState *env, int level)
     if (rc == -1)
         perror("kvm_set_msrs FAILED");
 
+    if (level >= KVM_PUT_RESET_STATE) {
+        kvm_arch_load_mpstate(env);
+    }
+    if (kvm_irqchip_in_kernel()) {
+        /* Avoid deadlock: no user space IRQ will ever clear it. */
+        env->halted = 0;
+    }
+
     /* must be last */
     kvm_guest_debug_workarounds(env);
 }
@@ -938,36 +996,6 @@  void kvm_load_tsc(CPUState *env)
         perror("kvm_set_tsc FAILED.\n");
 }
 
-void kvm_arch_save_mpstate(CPUState *env)
-{
-#ifdef KVM_CAP_MP_STATE
-    int r;
-    struct kvm_mp_state mp_state;
-
-    r = kvm_get_mpstate(env, &mp_state);
-    if (r < 0)
-        env->mp_state = -1;
-    else
-        env->mp_state = mp_state.mp_state;
-#else
-    env->mp_state = -1;
-#endif
-}
-
-void kvm_arch_load_mpstate(CPUState *env)
-{
-#ifdef KVM_CAP_MP_STATE
-    struct kvm_mp_state mp_state = { .mp_state = env->mp_state };
-
-    /*
-     * -1 indicates that the host did not support GET_MP_STATE ioctl,
-     *  so don't touch it.
-     */
-    if (env->mp_state != -1)
-        kvm_set_mpstate(env, &mp_state);
-#endif
-}
-
 void kvm_arch_save_regs(CPUState *env)
 {
     struct kvm_regs regs;
@@ -1290,6 +1318,7 @@  int kvm_arch_init_vcpu(CPUState *cenv)
 #ifdef KVM_EXIT_TPR_ACCESS
     kvm_tpr_vcpu_start(cenv);
 #endif
+    kvm_reset_mpstate(cenv);
     return 0;
 }
 
@@ -1363,15 +1392,10 @@  void kvm_arch_cpu_reset(CPUState *env)
 {
     kvm_arch_reset_vcpu(env);
     kvm_put_vcpu_events(env);
-    if (!cpu_is_bsp(env)) {
-	if (kvm_irqchip_in_kernel()) {
-#ifdef KVM_CAP_MP_STATE
-	    kvm_reset_mpstate(env);
-#endif
-	} else {
-	    env->interrupt_request &= ~CPU_INTERRUPT_HARD;
-	    env->halted = 1;
-	}
+    kvm_reset_mpstate(env);
+    if (!cpu_is_bsp(env) && !kvm_irqchip_in_kernel()) {
+        env->interrupt_request &= ~CPU_INTERRUPT_HARD;
+        env->halted = 1;
     }
 }
 
diff --git a/qemu-kvm.c b/qemu-kvm.c
index f6d3e4d..103e0d9 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -1590,36 +1590,6 @@  void kvm_update_interrupt_request(CPUState *env)
     }
 }
 
-static void kvm_do_load_mpstate(void *_env)
-{
-    CPUState *env = _env;
-
-    kvm_arch_load_mpstate(env);
-}
-
-void kvm_load_mpstate(CPUState *env)
-{
-    if (kvm_enabled() && qemu_system_ready && kvm_vcpu_inited(env))
-        on_vcpu(env, kvm_do_load_mpstate, env);
-}
-
-static void kvm_do_save_mpstate(void *_env)
-{
-    CPUState *env = _env;
-
-    kvm_arch_save_mpstate(env);
-#ifdef KVM_CAP_MP_STATE
-    if (kvm_irqchip_in_kernel())
-        env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
-#endif
-}
-
-void kvm_save_mpstate(CPUState *env)
-{
-    if (kvm_enabled())
-        on_vcpu(env, kvm_do_save_mpstate, env);
-}
-
 int kvm_cpu_exec(CPUState *env)
 {
     int r;
diff --git a/qemu-kvm.h b/qemu-kvm.h
index fab930d..c6ff652 100644
--- a/qemu-kvm.h
+++ b/qemu-kvm.h
@@ -299,16 +299,6 @@  int kvm_get_mpstate(CPUState *env, struct kvm_mp_state *mp_state);
  *
  */
 int kvm_set_mpstate(CPUState *env, struct kvm_mp_state *mp_state);
-/*!
- *  * \brief Reset VCPU MP state
- *
- */
-static inline int kvm_reset_mpstate(CPUState *env)
-{
-    struct kvm_mp_state mp_state = {.mp_state = KVM_MP_STATE_UNINITIALIZED
-    };
-    return kvm_set_mpstate(env, &mp_state);
-}
 #endif
 
 /*!
@@ -874,8 +864,6 @@  static inline void kvm_inject_x86_mce(CPUState *cenv, int bank,
 int kvm_main_loop(void);
 int kvm_init_ap(void);
 int kvm_vcpu_inited(CPUState *env);
-void kvm_load_mpstate(CPUState *env);
-void kvm_save_mpstate(CPUState *env);
 void kvm_apic_init(CPUState *env);
 /* called from vcpu initialization */
 void qemu_kvm_load_lapic(CPUState *env);
@@ -909,8 +897,6 @@  int kvm_arch_qemu_create_context(void);
 
 void kvm_arch_save_regs(CPUState *env);
 void kvm_arch_load_regs(CPUState *env, int level);
-void kvm_arch_load_mpstate(CPUState *env);
-void kvm_arch_save_mpstate(CPUState *env);
 int kvm_arch_has_work(CPUState *env);
 void kvm_arch_process_irqchip_events(CPUState *env);
 int kvm_arch_try_push_interrupts(void *opaque);
@@ -979,7 +965,6 @@  void kvm_load_tsc(CPUState *env);
 #ifdef TARGET_I386
 #define qemu_kvm_has_pit_state2() (0)
 #endif
-#define kvm_save_mpstate(env)   do {} while(0)
 #define qemu_kvm_cpu_stop(env) do {} while(0)
 static inline void kvm_load_tsc(CPUState *env)
 {
diff --git a/target-i386/machine.c b/target-i386/machine.c
index bebde82..61e6a87 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -323,7 +323,6 @@  static void cpu_pre_save(void *opaque)
     int i;
 
     if (kvm_enabled()) {
-        kvm_save_mpstate(env);
         kvm_get_vcpu_events(env);
     }
 
@@ -362,12 +361,7 @@  static int cpu_post_load(void *opaque, int version_id)
     tlb_flush(env, 1);
 
     if (kvm_enabled()) {
-        /* when in-kernel irqchip is used, env->halted causes deadlock
-           because no userspace IRQs will ever clear this flag */
-        env->halted = 0;
-
         kvm_load_tsc(env);
-        kvm_load_mpstate(env);
         kvm_put_vcpu_events(env);
     }
 
diff --git a/target-ia64/machine.c b/target-ia64/machine.c
index fdbeeef..8cf5bdd 100644
--- a/target-ia64/machine.c
+++ b/target-ia64/machine.c
@@ -4,6 +4,9 @@ 
 #include "exec-all.h"
 #include "qemu-kvm.h"
 
+void kvm_arch_save_mpstate(CPUState *env);
+void kvm_arch_load_mpstate(CPUState *env);
+
 void cpu_save(QEMUFile *f, void *opaque)
 {
     CPUState *env = opaque;