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

[Jan Kiszka <jan.kiszka@web.de>]

[-- 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 --]