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

[Marcelo Tosatti <mtosatti@redhat.com>]

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?