Re: [PATCH] call kvm_cpu_synchronize_state() on target vcpu

[Gleb Natapov <gleb@redhat.com>]

On Wed, Sep 09, 2009 at 05:57:40PM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Wed, Sep 09, 2009 at 05:47:13PM +0200, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> regs_modified logic doesn't work if io thread calls
> >>> kvm_cpu_synchronize_state() since kvm_arch_get_registers()
> >>> returns only after vcpu thread is back to kernel. Setting
> >>> regs_modified to 1 at this stage causes loading of wrong vcpu
> >>> state on the next vcpu_run().
> >> We need this upstream too, right? Could you file the corresponding patch?
> >>
> > Upstream is single threaded. It shouldn't suffer from this bug.
> 
> Not if you enable iothread support (though I don't remember if that
It can't work with kvm since all vcpu ioctls are called on the thread
that issues them.

> works now for kvm) + you are also touching shared code here. So qemu-kvm
> would benefit from keeping the diff small.
> 
The patch doesn't touch shared code. (it is almost impossible to tell
what code is shared and what's not nowadays)

> Jan
> 
> > 
> >> Thanks,
> >> Jan
> >>
> >>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> >>> diff --git a/qemu-kvm.c b/qemu-kvm.c
> >>> index 06efd41..9ab0cec 100644
> >>> --- a/qemu-kvm.c
> >>> +++ b/qemu-kvm.c
> >>> @@ -874,14 +874,6 @@ int kvm_set_mpstate(kvm_vcpu_context_t vcpu, struct kvm_mp_state *mp_state)
> >>>  }
> >>>  #endif
> >>>  
> >>> -void kvm_cpu_synchronize_state(CPUState *env)
> >>> -{
> >>> -    if (!env->kvm_cpu_state.regs_modified) {
> >>> -        kvm_arch_get_registers(env);
> >>> -        env->kvm_cpu_state.regs_modified = 1;
> >>> -    }
> >>> -}
> >>> -
> >>>  static int handle_mmio(kvm_vcpu_context_t vcpu)
> >>>  {
> >>>      unsigned long addr = vcpu->run->mmio.phys_addr;
> >>> @@ -1539,6 +1531,22 @@ static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
> >>>          qemu_cond_wait(&qemu_work_cond);
> >>>  }
> >>>  
> >>> +static void do_kvm_cpu_synchronize_state(void *_env)
> >>> +{
> >>> +    CPUState *env = _env;
> >>> +    if (!env->kvm_cpu_state.regs_modified) {
> >>> +        kvm_arch_save_regs(env);
> >>> +        kvm_arch_load_mpstate(env);
> >>> +        env->kvm_cpu_state.regs_modified = 1;
> >>> +    }
> >>> +}
> >>> +
> >>> +void kvm_cpu_synchronize_state(CPUState *env)
> >>> +{
> >>> +    if (!env->kvm_cpu_state.regs_modified)
> >>> +        on_vcpu(env, do_kvm_cpu_synchronize_state, env);
> >>> +}
> >>> +
> >>>  static void inject_interrupt(void *data)
> >>>  {
> >>>      cpu_interrupt(current_env, (long) data);
> >>> diff --git a/qemu-kvm.h b/qemu-kvm.h
> >>> index 2c1730b..32f74b3 100644
> >>> --- a/qemu-kvm.h
> >>> +++ b/qemu-kvm.h
> >>> @@ -1153,12 +1153,6 @@ static inline int kvm_sync_vcpus(void)
> >>>      return 0;
> >>>  }
> >>>  
> >>> -static inline void kvm_arch_get_registers(CPUState *env)
> >>> -{
> >>> -    kvm_save_registers(env);
> >>> -    kvm_save_mpstate(env);
> >>> -}
> >>> -
> >>>  static inline void kvm_arch_put_registers(CPUState *env)
> >>>  {
> >>>      kvm_load_registers(env);
> >>> diff --git a/target-i386/helper.c b/target-i386/helper.c
> >>> index 4a16887..57c74a2 100644
> >>> --- a/target-i386/helper.c
> >>> +++ b/target-i386/helper.c
> >>> @@ -746,7 +746,7 @@ void cpu_dump_state(CPUState *env, FILE *f,
> >>>      static const char *seg_name[6] = { "ES", "CS", "SS", "DS", "FS", "GS" };
> >>>  
> >>>      if (kvm_enabled())
> >>> -        kvm_arch_get_registers(env);
> >>> +        kvm_cpu_synchronize_state(env);
> >>>  
> >>>      eflags = env->eflags;
> >>>  #ifdef TARGET_X86_64
> >>> --
> >>> 			Gleb.
> >> -- 
> >> Siemens AG, Corporate Technology, CT SE 2
> >> Corporate Competence Center Embedded Linux
> > 
> > --
> > 			Gleb.
> 
> -- 
> Siemens AG, Corporate Technology, CT SE 2
> Corporate Competence Center Embedded Linux

--
			Gleb.