Re: [Qemu-devel] [PATCH v2 05/11] tell kernel about all registers instead of just mp_state

[Gleb Natapov]

On Tue, Dec 01, 2009 at 10:51:31AM -0200, Glauber Costa wrote:
> This fix a bug with -smp in kvm. Since we have updated apic_base,
> we also have to tell kernel about it. So instead of just updating
> mp_state, update every regs.
> 
> It is mandatory that this happens synchronously, without waiting for
> the next vcpu run. Otherwise, if we are migrating, or initializing
> the cpu's APIC, other cpus can still see an invalid state.
> 
> Since putting registers already happen in vcpu entry, we factor
> out the required code in cpu_flush_state()
> 
> Signed-off-by: Glauber Costa <address@hidden>
> ---
>  hw/apic-kvm.c |    5 ++++-
>  kvm-all.c     |   14 +++++++++-----
>  kvm.h         |    8 ++++++++
>  3 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/apic-kvm.c b/hw/apic-kvm.c
> index e5a0bfc..9e9790f 100644
> --- a/hw/apic-kvm.c
> +++ b/hw/apic-kvm.c
> @@ -126,7 +126,10 @@ static void kvm_apic_reset(void *opaque)
>      s->cpu_env->mp_state
>              = bsp ? KVM_MP_STATE_RUNNABLE : KVM_MP_STATE_UNINITIALIZED;
>  
> -    kvm_put_mp_state(s->cpu_env);
> +    /* We have to tell the kernel about mp_state, but also save sregs, since
> +     * apic base was just updated
> +     */
> +    cpu_flush_state(s->cpu_env);
>  
>      if (bsp) {
>          /*
> diff --git a/kvm-all.c b/kvm-all.c
> index 40203f0..318a4e6 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -622,7 +622,6 @@ static void kvm_run_coalesced_mmio(CPUState *env, struct 
> kvm_run *run)
>      }
>  #endif
>  }
> -
Spurious line removing.

>  void kvm_cpu_synchronize_state(CPUState *env)
>  {
>      if (!env->kvm_state->regs_modified) {
> @@ -631,6 +630,14 @@ void kvm_cpu_synchronize_state(CPUState *env)
>      }
>  }
>  
> +void kvm_cpu_flush_state(CPUState *env)
> +{
> +    if (env->kvm_state->regs_modified) {
> +        kvm_arch_put_registers(env);
> +        env->kvm_state->regs_modified = 0;
> +    }
> +}
> +
>  int kvm_cpu_exec(CPUState *env)
>  {
>      struct kvm_run *run = env->kvm_run;
> @@ -645,10 +652,7 @@ int kvm_cpu_exec(CPUState *env)
>              break;
>          }
>  
> -        if (env->kvm_state->regs_modified) {
> -            kvm_arch_put_registers(env);
> -            env->kvm_state->regs_modified = 0;
> -        }
> +        kvm_cpu_flush_state(env);
>  
>          kvm_arch_pre_run(env, run);
>          qemu_mutex_unlock_iothread();
> diff --git a/kvm.h b/kvm.h
> index a474d95..d9af176 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -139,6 +139,7 @@ int kvm_check_extension(KVMState *s, unsigned int 
> extension);
>  uint32_t kvm_arch_get_supported_cpuid(CPUState *env, uint32_t function,
>                                        int reg);
>  void kvm_cpu_synchronize_state(CPUState *env);
> +void kvm_cpu_flush_state(CPUState *env);
>  
>  /* generic hooks - to be moved/refactored once there are more users */
>  
> @@ -149,4 +150,11 @@ static inline void cpu_synchronize_state(CPUState *env)
>      }
>  }
>  
> +static inline void cpu_flush_state(CPUState *env)
> +{
> +    if (kvm_enabled()) {
Is this ever called or intended to be called when kvm is disabled?

> +        kvm_cpu_flush_state(env);
> +    }
> +}
> +
>  #endif
> -- 
> 1.6.5.2
> 
> 

--
                        Gleb.