[PATCH] call kvm_cpu_synchronize_state() on target vcpu

[PATCH] call kvm_cpu_synchronize_state() on target vcpu

[Gleb Natapov]

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().

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.

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

[Avi Kivity]

On 09/09/2009 06:33 PM, 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().
>
> -static inline void kvm_arch_get_registers(CPUState *env)
> -{
> -    kvm_save_registers(env);
> -    kvm_save_mpstate(env);
> -}
> -
>    

Please don't drop this, it's part of upstream kvm support which we're 
supported to be moving towards.

-- 
error compiling committee.c: too many arguments to function

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

[Jan Kiszka]

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?

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

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

[Gleb Natapov]

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.

> 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.

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

[Jan Kiszka]

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
works now for kvm) + you are also touching shared code here. So qemu-kvm
would benefit from keeping the diff small.

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

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

[Gleb Natapov]

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.

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

[Jan Kiszka]

Gleb Natapov wrote:
> 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.

Yeah, I just recalled all that on_vcpu fuzz and that upstream is still
horribly broken /wrt iothread+kvm. But once that is fixed, we also need
this fix here.

> 
>> 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)

cpu_dump_state() is definitely shared.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

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

[Gleb Natapov]

On Wed, Sep 09, 2009 at 06:21:48PM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > 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.
> 
> Yeah, I just recalled all that on_vcpu fuzz and that upstream is still
> horribly broken /wrt iothread+kvm. But once that is fixed, we also need
> this fix here.
> 
This will be done as part of transition to on_vcpu() for vcpu ioctls.

> > 
> >> 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)
> 
> cpu_dump_state() is definitely shared.
> 
Ah this one line. Yes it is. But I have not good commit message for this
one liner change for upstream :)

--
			Gleb.

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

[Jan Kiszka]

Gleb Natapov wrote:
> On Wed, Sep 09, 2009 at 06:21:48PM +0200, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> 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.
>> Yeah, I just recalled all that on_vcpu fuzz and that upstream is still
>> horribly broken /wrt iothread+kvm. But once that is fixed, we also need
>> this fix here.
>>
> This will be done as part of transition to on_vcpu() for vcpu ioctls.
> 
>>>> 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)
>> cpu_dump_state() is definitely shared.
>>
> Ah this one line. Yes it is. But I have not good commit message for this
> one liner change for upstream :)

That's why I suggested to post the corresponding change also for
upstream. Even if it doesn't need it now, it will one day. :)

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

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

[Gleb Natapov]

On Wed, Sep 09, 2009 at 06:32:57PM +0200, Jan Kiszka wrote:
> >>> The patch doesn't touch shared code. (it is almost impossible to tell
> >>> what code is shared and what's not nowadays)
> >> cpu_dump_state() is definitely shared.
> >>
> > Ah this one line. Yes it is. But I have not good commit message for this
> > one liner change for upstream :)
> 
> That's why I suggested to post the corresponding change also for
> upstream. Even if it doesn't need it now, it will one day. :)
> 
I looked at the code. This changes doesn't make sense for upstream right
now IMHO. I do agree that this one linear should go into upstream. I'll
think about commit message tonight.

--
			Gleb.