| Message ID | 20090909153309.GD22885@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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> -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html