[PATCH 1/2] Complete cpu initialization before signaling main thread.

[PATCH 1/2] Complete cpu initialization before signaling main thread.

[Gleb Natapov]

Otherwise some cpus may start executing code before others
are fully initialized.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 qemu-kvm.c |   26 ++++++++++++--------------
 1 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/qemu-kvm.c b/qemu-kvm.c
index 62ca050..3765818 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -1954,18 +1954,6 @@ static void process_irqchip_events(CPUState *env)
 
 static int kvm_main_loop_cpu(CPUState *env)
 {
-    setup_kernel_sigmask(env);
-
-    pthread_mutex_lock(&qemu_mutex);
-
-    kvm_arch_init_vcpu(env);
-#ifdef TARGET_I386
-    kvm_tpr_vcpu_start(env);
-#endif
-
-    cpu_single_env = env;
-    kvm_arch_load_regs(env);
-
     while (1) {
         int run_cpu = !is_cpu_stopped(env);
         if (run_cpu && !kvm_irqchip_in_kernel(kvm_context)) {
@@ -2003,15 +1991,25 @@ static void *ap_main_loop(void *_env)
         on_vcpu(env, kvm_arch_do_ioperm, data);
 #endif
 
-    /* signal VCPU creation */
+    setup_kernel_sigmask(env);
+
     pthread_mutex_lock(&qemu_mutex);
+    cpu_single_env = env;
+
+    kvm_arch_init_vcpu(env);
+#ifdef TARGET_I386
+    kvm_tpr_vcpu_start(env);
+#endif
+
+    kvm_arch_load_regs(env);
+
+    /* signal VCPU creation */
     current_env->created = 1;
     pthread_cond_signal(&qemu_vcpu_cond);
 
     /* and wait for machine initialization */
     while (!qemu_system_ready)
         qemu_cond_wait(&qemu_system_cond);
-    pthread_mutex_unlock(&qemu_mutex);
 
     kvm_main_loop_cpu(env);
     return NULL;
-- 
1.6.3.3

[PATCH 2/2] Don't sync mpstate to/from kernel when unneeded.

[Gleb Natapov]

mp_state, unlike other cpu state, can be changed not only from vcpu
context it belongs to, but by other vcpus too. That makes its loading
from kernel/saving back not safe if mp_state value is changed inside
kernel between load and save. For example vcpu 1 loads mp_sate into
user-space and the state is RUNNING, vcpu 0 sends INIT/SIPI to vcpu 1
so in-kernel mp_sate becomes SIPI, vcpu 1 save user-space copy into
kernel and calls vcpu_run(). SIPI sate is lost.

The patch copies mp_sate into kernel only when it is knows that
int-kernel value is outdated. This happens on reset and vmload.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 hw/apic.c             |    1 +
 monitor.c             |    2 ++
 qemu-kvm.c            |    9 ++++-----
 qemu-kvm.h            |    1 -
 target-i386/machine.c |    3 +++
 5 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index 2952675..7244449 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -512,6 +512,7 @@ void apic_init_reset(CPUState *env)
     if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
         env->mp_state
             = env->halted ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
+        kvm_load_mpstate(env);
     }
 #endif
 }
diff --git a/monitor.c b/monitor.c
index 7f0f5a9..dd8f2ca 100644
--- a/monitor.c
+++ b/monitor.c
@@ -350,6 +350,7 @@ static CPUState *mon_get_cpu(void)
         mon_set_cpu(0);
     }
     cpu_synchronize_state(cur_mon->mon_cpu);
+    kvm_save_mpstate(cur_mon->mon_cpu);
     return cur_mon->mon_cpu;
 }
 
@@ -377,6 +378,7 @@ static void do_info_cpus(Monitor *mon)
 
     for(env = first_cpu; env != NULL; env = env->next_cpu) {
         cpu_synchronize_state(env);
+        kvm_save_mpstate(env);
         monitor_printf(mon, "%c CPU #%d:",
                        (env == mon->mon_cpu) ? '*' : ' ',
                        env->cpu_index);
diff --git a/qemu-kvm.c b/qemu-kvm.c
index 3765818..2a1e0ff 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -1609,11 +1609,6 @@ static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
 void kvm_arch_get_registers(CPUState *env)
 {
 	kvm_arch_save_regs(env);
-	kvm_arch_save_mpstate(env);
-#ifdef KVM_CAP_MP_STATE
-	if (kvm_irqchip_in_kernel(kvm_context))
-		env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
-#endif
 }
 
 static void do_kvm_cpu_synchronize_state(void *_env)
@@ -1707,6 +1702,10 @@ 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(kvm_context))
+        env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
+#endif
 }
 
 void kvm_save_mpstate(CPUState *env)
diff --git a/qemu-kvm.h b/qemu-kvm.h
index d6748c7..e2a87b8 100644
--- a/qemu-kvm.h
+++ b/qemu-kvm.h
@@ -1186,7 +1186,6 @@ void kvm_arch_get_registers(CPUState *env);
 static inline void kvm_arch_put_registers(CPUState *env)
 {
     kvm_load_registers(env);
-    kvm_load_mpstate(env);
 }
 
 void kvm_cpu_synchronize_state(CPUState *env);
diff --git a/target-i386/machine.c b/target-i386/machine.c
index e640dad..16d9c57 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -324,6 +324,7 @@ static void cpu_pre_save(void *opaque)
     int i, bit;
 
     cpu_synchronize_state(env);
+    kvm_save_mpstate(env);
 
     /* FPU */
     env->fpus_vmstate = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
@@ -385,6 +386,8 @@ static int cpu_post_load(void *opaque, int version_id)
     }
 
     tlb_flush(env, 1);
+    kvm_load_mpstate(env);
+
     return 0;
 }
 
-- 
1.6.3.3

Re: [PATCH 1/2] Complete cpu initialization before signaling main thread.

[Marcelo Tosatti]

On Tue, Oct 13, 2009 at 02:17:19PM +0200, Gleb Natapov wrote:
> Otherwise some cpus may start executing code before others
> are fully initialized.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>  qemu-kvm.c |   26 ++++++++++++--------------
>  1 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/qemu-kvm.c b/qemu-kvm.c
> index 62ca050..3765818 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -1954,18 +1954,6 @@ static void process_irqchip_events(CPUState *env)
>  
>  static int kvm_main_loop_cpu(CPUState *env)
>  {
> -    setup_kernel_sigmask(env);
> -
> -    pthread_mutex_lock(&qemu_mutex);
> -
> -    kvm_arch_init_vcpu(env);
> -#ifdef TARGET_I386
> -    kvm_tpr_vcpu_start(env);
> -#endif
> -
> -    cpu_single_env = env;
> -    kvm_arch_load_regs(env);
> -
>      while (1) {
>          int run_cpu = !is_cpu_stopped(env);
>          if (run_cpu && !kvm_irqchip_in_kernel(kvm_context)) {
> @@ -2003,15 +1991,25 @@ static void *ap_main_loop(void *_env)
>          on_vcpu(env, kvm_arch_do_ioperm, data);
>  #endif
>  
> -    /* signal VCPU creation */
> +    setup_kernel_sigmask(env);
> +
>      pthread_mutex_lock(&qemu_mutex);
> +    cpu_single_env = env;
> +
> +    kvm_arch_init_vcpu(env);
> +#ifdef TARGET_I386
> +    kvm_tpr_vcpu_start(env);
> +#endif
> +
> +    kvm_arch_load_regs(env);
> +
> +    /* signal VCPU creation */
>      current_env->created = 1;
>      pthread_cond_signal(&qemu_vcpu_cond);
>  
>      /* and wait for machine initialization */
>      while (!qemu_system_ready)
>          qemu_cond_wait(&qemu_system_cond);
> -    pthread_mutex_unlock(&qemu_mutex);

You don't set cpu_single_env after reacquiring 
qemu_mutex here (via qemu_cond_wait).

Re: [PATCH 1/2] Complete cpu initialization before signaling main thread.

[Marcelo Tosatti]

On Tue, Oct 13, 2009 at 03:19:08PM -0300, Marcelo Tosatti wrote:
> > @@ -2003,15 +1991,25 @@ static void *ap_main_loop(void *_env)
> >          on_vcpu(env, kvm_arch_do_ioperm, data);
> >  #endif
> >  
> > -    /* signal VCPU creation */
> > +    setup_kernel_sigmask(env);
> > +
> >      pthread_mutex_lock(&qemu_mutex);
> > +    cpu_single_env = env;
> > +
> > +    kvm_arch_init_vcpu(env);
> > +#ifdef TARGET_I386
> > +    kvm_tpr_vcpu_start(env);
> > +#endif
> > +
> > +    kvm_arch_load_regs(env);
> > +
> > +    /* signal VCPU creation */
> >      current_env->created = 1;
> >      pthread_cond_signal(&qemu_vcpu_cond);
> >  
> >      /* and wait for machine initialization */
> >      while (!qemu_system_ready)
> >          qemu_cond_wait(&qemu_system_cond);
> > -    pthread_mutex_unlock(&qemu_mutex);
> 
> You don't set cpu_single_env after reacquiring 
> qemu_mutex here (via qemu_cond_wait).
> 

Also i'm curious about the failure.

Why say, bsp should care about other cpu's register state while doing MP
init?

MP state is set via apic_reset, which happens before qemu_system_ready
is set.

Re: [PATCH 1/2] Complete cpu initialization before signaling main thread.

[Gleb Natapov]

On Tue, Oct 13, 2009 at 03:19:08PM -0300, Marcelo Tosatti wrote:
> On Tue, Oct 13, 2009 at 02:17:19PM +0200, Gleb Natapov wrote:
> > Otherwise some cpus may start executing code before others
> > are fully initialized.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > ---
> >  qemu-kvm.c |   26 ++++++++++++--------------
> >  1 files changed, 12 insertions(+), 14 deletions(-)
> > 
> > diff --git a/qemu-kvm.c b/qemu-kvm.c
> > index 62ca050..3765818 100644
> > --- a/qemu-kvm.c
> > +++ b/qemu-kvm.c
> > @@ -1954,18 +1954,6 @@ static void process_irqchip_events(CPUState *env)
> >  
> >  static int kvm_main_loop_cpu(CPUState *env)
> >  {
> > -    setup_kernel_sigmask(env);
> > -
> > -    pthread_mutex_lock(&qemu_mutex);
> > -
> > -    kvm_arch_init_vcpu(env);
> > -#ifdef TARGET_I386
> > -    kvm_tpr_vcpu_start(env);
> > -#endif
> > -
> > -    cpu_single_env = env;
> > -    kvm_arch_load_regs(env);
> > -
> >      while (1) {
> >          int run_cpu = !is_cpu_stopped(env);
> >          if (run_cpu && !kvm_irqchip_in_kernel(kvm_context)) {
> > @@ -2003,15 +1991,25 @@ static void *ap_main_loop(void *_env)
> >          on_vcpu(env, kvm_arch_do_ioperm, data);
> >  #endif
> >  
> > -    /* signal VCPU creation */
> > +    setup_kernel_sigmask(env);
> > +
> >      pthread_mutex_lock(&qemu_mutex);
> > +    cpu_single_env = env;
> > +
> > +    kvm_arch_init_vcpu(env);
> > +#ifdef TARGET_I386
> > +    kvm_tpr_vcpu_start(env);
> > +#endif
> > +
> > +    kvm_arch_load_regs(env);
> > +
> > +    /* signal VCPU creation */
> >      current_env->created = 1;
> >      pthread_cond_signal(&qemu_vcpu_cond);
> >  
> >      /* and wait for machine initialization */
> >      while (!qemu_system_ready)
> >          qemu_cond_wait(&qemu_system_cond);
> > -    pthread_mutex_unlock(&qemu_mutex);
> 
> You don't set cpu_single_env after reacquiring 
> qemu_mutex here (via qemu_cond_wait).
Hmm, as far as I can see it is not used any more until kvm_run call.
But may we should set it anyway.

--
			Gleb.

Re: [PATCH 1/2] Complete cpu initialization before signaling main thread.

[Gleb Natapov]

On Tue, Oct 13, 2009 at 03:23:48PM -0300, Marcelo Tosatti wrote:
> On Tue, Oct 13, 2009 at 03:19:08PM -0300, Marcelo Tosatti wrote:
> > > @@ -2003,15 +1991,25 @@ static void *ap_main_loop(void *_env)
> > >          on_vcpu(env, kvm_arch_do_ioperm, data);
> > >  #endif
> > >  
> > > -    /* signal VCPU creation */
> > > +    setup_kernel_sigmask(env);
> > > +
> > >      pthread_mutex_lock(&qemu_mutex);
> > > +    cpu_single_env = env;
> > > +
> > > +    kvm_arch_init_vcpu(env);
> > > +#ifdef TARGET_I386
> > > +    kvm_tpr_vcpu_start(env);
> > > +#endif
> > > +
> > > +    kvm_arch_load_regs(env);
> > > +
> > > +    /* signal VCPU creation */
> > >      current_env->created = 1;
> > >      pthread_cond_signal(&qemu_vcpu_cond);
> > >  
> > >      /* and wait for machine initialization */
> > >      while (!qemu_system_ready)
> > >          qemu_cond_wait(&qemu_system_cond);
> > > -    pthread_mutex_unlock(&qemu_mutex);
> > 
> > You don't set cpu_single_env after reacquiring 
> > qemu_mutex here (via qemu_cond_wait).
> > 
> 
> Also i'm curious about the failure.
This patch by itself doesn't fix the bug. Next one does. This one
rearrange code to make more sense. CPU is created only when it is
initialized and ready to run.

> 
> Why say, bsp should care about other cpu's register state while doing MP
> init?
> 
Because vcpu init will reset MP state, so if bsp will send sipi to vcpu1
before vcpu1 is initialized sipi will be lost.

> MP state is set via apic_reset, which happens before qemu_system_ready
> is set.
> 
Without my next patch MP state is set (by set I mean ioctl(mp_state)) only on vcpu_run.

--
			Gleb.

Re: [PATCH 2/2] Don't sync mpstate to/from kernel when unneeded.

[Marcelo Tosatti]

On Tue, Oct 13, 2009 at 02:17:20PM +0200, Gleb Natapov wrote:
> mp_state, unlike other cpu state, can be changed not only from vcpu
> context it belongs to, but by other vcpus too. That makes its loading
> from kernel/saving back not safe if mp_state value is changed inside
> kernel between load and save. For example vcpu 1 loads mp_sate into
> user-space and the state is RUNNING, vcpu 0 sends INIT/SIPI to vcpu 1
> so in-kernel mp_sate becomes SIPI, vcpu 1 save user-space copy into
> kernel and calls vcpu_run(). SIPI sate is lost.
> 
> The patch copies mp_sate into kernel only when it is knows that
> int-kernel value is outdated. This happens on reset and vmload.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>  hw/apic.c             |    1 +
>  monitor.c             |    2 ++
>  qemu-kvm.c            |    9 ++++-----
>  qemu-kvm.h            |    1 -
>  target-i386/machine.c |    3 +++
>  5 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/apic.c b/hw/apic.c
> index 2952675..7244449 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -512,6 +512,7 @@ void apic_init_reset(CPUState *env)
>      if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
>          env->mp_state
>              = env->halted ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
> +        kvm_load_mpstate(env);
>      }
>  #endif
>  }
> diff --git a/monitor.c b/monitor.c
> index 7f0f5a9..dd8f2ca 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -350,6 +350,7 @@ static CPUState *mon_get_cpu(void)
>          mon_set_cpu(0);
>      }
>      cpu_synchronize_state(cur_mon->mon_cpu);
> +    kvm_save_mpstate(cur_mon->mon_cpu);
>      return cur_mon->mon_cpu;
>  }
>  
> @@ -377,6 +378,7 @@ static void do_info_cpus(Monitor *mon)
>  
>      for(env = first_cpu; env != NULL; env = env->next_cpu) {
>          cpu_synchronize_state(env);
> +        kvm_save_mpstate(env);
>          monitor_printf(mon, "%c CPU #%d:",
>                         (env == mon->mon_cpu) ? '*' : ' ',
>                         env->cpu_index);
> diff --git a/qemu-kvm.c b/qemu-kvm.c
> index 3765818..2a1e0ff 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -1609,11 +1609,6 @@ static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
>  void kvm_arch_get_registers(CPUState *env)
>  {
>  	kvm_arch_save_regs(env);
> -	kvm_arch_save_mpstate(env);
> -#ifdef KVM_CAP_MP_STATE
> -	if (kvm_irqchip_in_kernel(kvm_context))
> -		env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
> -#endif

Why don't you keep saving it here (so there's no need to do it
explicitly elsewhere), and only explictly loading?

Re: [PATCH 2/2] Don't sync mpstate to/from kernel when unneeded.

[Gleb Natapov]

On Tue, Oct 13, 2009 at 03:36:13PM -0300, Marcelo Tosatti wrote:
> On Tue, Oct 13, 2009 at 02:17:20PM +0200, Gleb Natapov wrote:
> > mp_state, unlike other cpu state, can be changed not only from vcpu
> > context it belongs to, but by other vcpus too. That makes its loading
> > from kernel/saving back not safe if mp_state value is changed inside
> > kernel between load and save. For example vcpu 1 loads mp_sate into
> > user-space and the state is RUNNING, vcpu 0 sends INIT/SIPI to vcpu 1
> > so in-kernel mp_sate becomes SIPI, vcpu 1 save user-space copy into
> > kernel and calls vcpu_run(). SIPI sate is lost.
> > 
> > The patch copies mp_sate into kernel only when it is knows that
> > int-kernel value is outdated. This happens on reset and vmload.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > ---
> >  hw/apic.c             |    1 +
> >  monitor.c             |    2 ++
> >  qemu-kvm.c            |    9 ++++-----
> >  qemu-kvm.h            |    1 -
> >  target-i386/machine.c |    3 +++
> >  5 files changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/apic.c b/hw/apic.c
> > index 2952675..7244449 100644
> > --- a/hw/apic.c
> > +++ b/hw/apic.c
> > @@ -512,6 +512,7 @@ void apic_init_reset(CPUState *env)
> >      if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
> >          env->mp_state
> >              = env->halted ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
> > +        kvm_load_mpstate(env);
> >      }
> >  #endif
> >  }
> > diff --git a/monitor.c b/monitor.c
> > index 7f0f5a9..dd8f2ca 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -350,6 +350,7 @@ static CPUState *mon_get_cpu(void)
> >          mon_set_cpu(0);
> >      }
> >      cpu_synchronize_state(cur_mon->mon_cpu);
> > +    kvm_save_mpstate(cur_mon->mon_cpu);
> >      return cur_mon->mon_cpu;
> >  }
> >  
> > @@ -377,6 +378,7 @@ static void do_info_cpus(Monitor *mon)
> >  
> >      for(env = first_cpu; env != NULL; env = env->next_cpu) {
> >          cpu_synchronize_state(env);
> > +        kvm_save_mpstate(env);
> >          monitor_printf(mon, "%c CPU #%d:",
> >                         (env == mon->mon_cpu) ? '*' : ' ',
> >                         env->cpu_index);
> > diff --git a/qemu-kvm.c b/qemu-kvm.c
> > index 3765818..2a1e0ff 100644
> > --- a/qemu-kvm.c
> > +++ b/qemu-kvm.c
> > @@ -1609,11 +1609,6 @@ static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
> >  void kvm_arch_get_registers(CPUState *env)
> >  {
> >  	kvm_arch_save_regs(env);
> > -	kvm_arch_save_mpstate(env);
> > -#ifdef KVM_CAP_MP_STATE
> > -	if (kvm_irqchip_in_kernel(kvm_context))
> > -		env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
> > -#endif
> 
> Why don't you keep saving it here (so there's no need to do it
> explicitly elsewhere), and only explictly loading?
To keep kvm_arch_get_registers/kvm_arch_set_registers symmetric I guess.

--
			Gleb.

[PATCH 2/2] Don't sync mpstate to/from kernel when unneeded.

[Gleb Natapov]

mp_state, unlike other cpu state, can be changed not only from vcpu
context it belongs to, but by other vcpus too. That makes its loading
from kernel/saving back not safe if mp_state value is changed inside
kernel between load and save. For example vcpu 1 loads mp_sate into
user-space and the state is RUNNING, vcpu 0 sends INIT/SIPI to vcpu 1
so in-kernel mp_sate becomes SIPI, vcpu 1 save user-space copy into
kernel and calls vcpu_run(). SIPI sate is lost.

The patch copies mp_sate into kernel only when it is knows that
int-kernel value is outdated. This happens on reset and vmload.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 hw/apic.c             |    1 +
 monitor.c             |    2 ++
 qemu-kvm.c            |    9 ++++-----
 qemu-kvm.h            |    1 -
 target-i386/machine.c |    3 +++
 5 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index 2952675..7244449 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -512,6 +512,7 @@ void apic_init_reset(CPUState *env)
     if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
         env->mp_state
             = env->halted ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
+        kvm_load_mpstate(env);
     }
 #endif
 }
diff --git a/monitor.c b/monitor.c
index 7f0f5a9..dd8f2ca 100644
--- a/monitor.c
+++ b/monitor.c
@@ -350,6 +350,7 @@ static CPUState *mon_get_cpu(void)
         mon_set_cpu(0);
     }
     cpu_synchronize_state(cur_mon->mon_cpu);
+    kvm_save_mpstate(cur_mon->mon_cpu);
     return cur_mon->mon_cpu;
 }
 
@@ -377,6 +378,7 @@ static void do_info_cpus(Monitor *mon)
 
     for(env = first_cpu; env != NULL; env = env->next_cpu) {
         cpu_synchronize_state(env);
+        kvm_save_mpstate(env);
         monitor_printf(mon, "%c CPU #%d:",
                        (env == mon->mon_cpu) ? '*' : ' ',
                        env->cpu_index);
diff --git a/qemu-kvm.c b/qemu-kvm.c
index a104ab8..267222d 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -1609,11 +1609,6 @@ static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
 void kvm_arch_get_registers(CPUState *env)
 {
 	kvm_arch_save_regs(env);
-	kvm_arch_save_mpstate(env);
-#ifdef KVM_CAP_MP_STATE
-	if (kvm_irqchip_in_kernel(kvm_context))
-		env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
-#endif
 }
 
 static void do_kvm_cpu_synchronize_state(void *_env)
@@ -1707,6 +1702,10 @@ 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(kvm_context))
+        env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
+#endif
 }
 
 void kvm_save_mpstate(CPUState *env)
diff --git a/qemu-kvm.h b/qemu-kvm.h
index d6748c7..e2a87b8 100644
--- a/qemu-kvm.h
+++ b/qemu-kvm.h
@@ -1186,7 +1186,6 @@ void kvm_arch_get_registers(CPUState *env);
 static inline void kvm_arch_put_registers(CPUState *env)
 {
     kvm_load_registers(env);
-    kvm_load_mpstate(env);
 }
 
 void kvm_cpu_synchronize_state(CPUState *env);
diff --git a/target-i386/machine.c b/target-i386/machine.c
index e640dad..16d9c57 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -324,6 +324,7 @@ static void cpu_pre_save(void *opaque)
     int i, bit;
 
     cpu_synchronize_state(env);
+    kvm_save_mpstate(env);
 
     /* FPU */
     env->fpus_vmstate = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
@@ -385,6 +386,8 @@ static int cpu_post_load(void *opaque, int version_id)
     }
 
     tlb_flush(env, 1);
+    kvm_load_mpstate(env);
+
     return 0;
 }
 
-- 
1.6.3.3

Re: [PATCH 2/2] Don't sync mpstate to/from kernel when unneeded.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4221 bytes --]

Gleb Natapov wrote:
> mp_state, unlike other cpu state, can be changed not only from vcpu
> context it belongs to, but by other vcpus too. That makes its loading
> from kernel/saving back not safe if mp_state value is changed inside
> kernel between load and save. For example vcpu 1 loads mp_sate into
> user-space and the state is RUNNING, vcpu 0 sends INIT/SIPI to vcpu 1
> so in-kernel mp_sate becomes SIPI, vcpu 1 save user-space copy into
> kernel and calls vcpu_run(). SIPI sate is lost.
> 
> The patch copies mp_sate into kernel only when it is knows that
> int-kernel value is outdated. This happens on reset and vmload.

Just stumbled over this commit as it breaks kvm-disabled builds
(layering violations...). I guess we need this upstream too once the
irqchips go in. Or do your patches, Glauber, already include this? If
not, anyone already thought about how to move the kvm logic behind
qemu's state sync curtain?

Jan

> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>  hw/apic.c             |    1 +
>  monitor.c             |    2 ++
>  qemu-kvm.c            |    9 ++++-----
>  qemu-kvm.h            |    1 -
>  target-i386/machine.c |    3 +++
>  5 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/apic.c b/hw/apic.c
> index 2952675..7244449 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -512,6 +512,7 @@ void apic_init_reset(CPUState *env)
>      if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
>          env->mp_state
>              = env->halted ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
> +        kvm_load_mpstate(env);
>      }
>  #endif
>  }
> diff --git a/monitor.c b/monitor.c
> index 7f0f5a9..dd8f2ca 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -350,6 +350,7 @@ static CPUState *mon_get_cpu(void)
>          mon_set_cpu(0);
>      }
>      cpu_synchronize_state(cur_mon->mon_cpu);
> +    kvm_save_mpstate(cur_mon->mon_cpu);
>      return cur_mon->mon_cpu;
>  }
>  
> @@ -377,6 +378,7 @@ static void do_info_cpus(Monitor *mon)
>  
>      for(env = first_cpu; env != NULL; env = env->next_cpu) {
>          cpu_synchronize_state(env);
> +        kvm_save_mpstate(env);
>          monitor_printf(mon, "%c CPU #%d:",
>                         (env == mon->mon_cpu) ? '*' : ' ',
>                         env->cpu_index);
> diff --git a/qemu-kvm.c b/qemu-kvm.c
> index a104ab8..267222d 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -1609,11 +1609,6 @@ static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
>  void kvm_arch_get_registers(CPUState *env)
>  {
>  	kvm_arch_save_regs(env);
> -	kvm_arch_save_mpstate(env);
> -#ifdef KVM_CAP_MP_STATE
> -	if (kvm_irqchip_in_kernel(kvm_context))
> -		env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
> -#endif
>  }
>  
>  static void do_kvm_cpu_synchronize_state(void *_env)
> @@ -1707,6 +1702,10 @@ 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(kvm_context))
> +        env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
> +#endif
>  }
>  
>  void kvm_save_mpstate(CPUState *env)
> diff --git a/qemu-kvm.h b/qemu-kvm.h
> index d6748c7..e2a87b8 100644
> --- a/qemu-kvm.h
> +++ b/qemu-kvm.h
> @@ -1186,7 +1186,6 @@ void kvm_arch_get_registers(CPUState *env);
>  static inline void kvm_arch_put_registers(CPUState *env)
>  {
>      kvm_load_registers(env);
> -    kvm_load_mpstate(env);
>  }
>  
>  void kvm_cpu_synchronize_state(CPUState *env);
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index e640dad..16d9c57 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -324,6 +324,7 @@ static void cpu_pre_save(void *opaque)
>      int i, bit;
>  
>      cpu_synchronize_state(env);
> +    kvm_save_mpstate(env);
>  
>      /* FPU */
>      env->fpus_vmstate = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
> @@ -385,6 +386,8 @@ static int cpu_post_load(void *opaque, int version_id)
>      }
>  
>      tlb_flush(env, 1);
> +    kvm_load_mpstate(env);
> +
>      return 0;
>  }
>  


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH 2/2] Don't sync mpstate to/from kernel when unneeded.

[Glauber Costa]

On Thu, Nov 12, 2009 at 12:33:07AM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > mp_state, unlike other cpu state, can be changed not only from vcpu
> > context it belongs to, but by other vcpus too. That makes its loading
> > from kernel/saving back not safe if mp_state value is changed inside
> > kernel between load and save. For example vcpu 1 loads mp_sate into
> > user-space and the state is RUNNING, vcpu 0 sends INIT/SIPI to vcpu 1
> > so in-kernel mp_sate becomes SIPI, vcpu 1 save user-space copy into
> > kernel and calls vcpu_run(). SIPI sate is lost.
> > 
> > The patch copies mp_sate into kernel only when it is knows that
> > int-kernel value is outdated. This happens on reset and vmload.
> 
> Just stumbled over this commit as it breaks kvm-disabled builds
> (layering violations...). I guess we need this upstream too once the
> irqchips go in. Or do your patches, Glauber, already include this? If
> not, anyone already thought about how to move the kvm logic behind
> qemu's state sync curtain?
It does not, since upstream qemu still does not support smp.

I basically did not worried about it.

> 
> Jan
> 
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > ---
> >  hw/apic.c             |    1 +
> >  monitor.c             |    2 ++
> >  qemu-kvm.c            |    9 ++++-----
> >  qemu-kvm.h            |    1 -
> >  target-i386/machine.c |    3 +++
> >  5 files changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/apic.c b/hw/apic.c
> > index 2952675..7244449 100644
> > --- a/hw/apic.c
> > +++ b/hw/apic.c
> > @@ -512,6 +512,7 @@ void apic_init_reset(CPUState *env)
> >      if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
> >          env->mp_state
> >              = env->halted ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
> > +        kvm_load_mpstate(env);
> >      }
> >  #endif
> >  }
> > diff --git a/monitor.c b/monitor.c
> > index 7f0f5a9..dd8f2ca 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -350,6 +350,7 @@ static CPUState *mon_get_cpu(void)
> >          mon_set_cpu(0);
> >      }
> >      cpu_synchronize_state(cur_mon->mon_cpu);
> > +    kvm_save_mpstate(cur_mon->mon_cpu);
> >      return cur_mon->mon_cpu;
> >  }
> >  
> > @@ -377,6 +378,7 @@ static void do_info_cpus(Monitor *mon)
> >  
> >      for(env = first_cpu; env != NULL; env = env->next_cpu) {
> >          cpu_synchronize_state(env);
> > +        kvm_save_mpstate(env);
> >          monitor_printf(mon, "%c CPU #%d:",
> >                         (env == mon->mon_cpu) ? '*' : ' ',
> >                         env->cpu_index);
> > diff --git a/qemu-kvm.c b/qemu-kvm.c
> > index a104ab8..267222d 100644
> > --- a/qemu-kvm.c
> > +++ b/qemu-kvm.c
> > @@ -1609,11 +1609,6 @@ static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
> >  void kvm_arch_get_registers(CPUState *env)
> >  {
> >  	kvm_arch_save_regs(env);
> > -	kvm_arch_save_mpstate(env);
> > -#ifdef KVM_CAP_MP_STATE
> > -	if (kvm_irqchip_in_kernel(kvm_context))
> > -		env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
> > -#endif
> >  }
> >  
> >  static void do_kvm_cpu_synchronize_state(void *_env)
> > @@ -1707,6 +1702,10 @@ 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(kvm_context))
> > +        env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
> > +#endif
> >  }
> >  
> >  void kvm_save_mpstate(CPUState *env)
> > diff --git a/qemu-kvm.h b/qemu-kvm.h
> > index d6748c7..e2a87b8 100644
> > --- a/qemu-kvm.h
> > +++ b/qemu-kvm.h
> > @@ -1186,7 +1186,6 @@ void kvm_arch_get_registers(CPUState *env);
> >  static inline void kvm_arch_put_registers(CPUState *env)
> >  {
> >      kvm_load_registers(env);
> > -    kvm_load_mpstate(env);
> >  }
> >  
> >  void kvm_cpu_synchronize_state(CPUState *env);
> > diff --git a/target-i386/machine.c b/target-i386/machine.c
> > index e640dad..16d9c57 100644
> > --- a/target-i386/machine.c
> > +++ b/target-i386/machine.c
> > @@ -324,6 +324,7 @@ static void cpu_pre_save(void *opaque)
> >      int i, bit;
> >  
> >      cpu_synchronize_state(env);
> > +    kvm_save_mpstate(env);
> >  
> >      /* FPU */
> >      env->fpus_vmstate = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
> > @@ -385,6 +386,8 @@ static int cpu_post_load(void *opaque, int version_id)
> >      }
> >  
> >      tlb_flush(env, 1);
> > +    kvm_load_mpstate(env);
> > +
> >      return 0;
> >  }
> >  
>