* [PATCHv2 1/2] Complete cpu initialization before signaling main thread. @ 2009-10-14 13:52 Gleb Natapov 2009-10-14 13:52 ` [PATCH 2/2] Don't sync mpstate to/from kernel when unneeded Gleb Natapov 2009-10-14 18:35 ` [PATCHv2 1/2] Complete cpu initialization before signaling main thread Marcelo Tosatti 0 siblings, 2 replies; 5+ messages in thread From: Gleb Natapov @ 2009-10-14 13:52 UTC (permalink / raw) To: mtosatti; +Cc: kvm Otherwise some cpus may start executing code before others are fully initialized. Signed-off-by: Gleb Natapov <gleb@redhat.com> --- v1->v2: - reinit cpu_single_env after qemu_cond_wait() qemu-kvm.c | 29 +++++++++++++++-------------- 1 files changed, 15 insertions(+), 14 deletions(-) diff --git a/qemu-kvm.c b/qemu-kvm.c index 62ca050..a104ab8 100644 --- a/qemu-kvm.c +++ b/qemu-kvm.c @@ -1954,18 +1954,6 @@ static void process_irqchip_events(CPUState *env) static int kvm_main_loop_cpu(CPUState *env) { - setup_kernel_sigmask(env); - - pthread_mutex_lock(&qemu_mutex); - - kvm_arch_init_vcpu(env); -#ifdef TARGET_I386 - kvm_tpr_vcpu_start(env); -#endif - - cpu_single_env = env; - kvm_arch_load_regs(env); - while (1) { int run_cpu = !is_cpu_stopped(env); if (run_cpu && !kvm_irqchip_in_kernel(kvm_context)) { @@ -2003,15 +1991,28 @@ static void *ap_main_loop(void *_env) on_vcpu(env, kvm_arch_do_ioperm, data); #endif - /* signal VCPU creation */ + setup_kernel_sigmask(env); + pthread_mutex_lock(&qemu_mutex); + cpu_single_env = env; + + kvm_arch_init_vcpu(env); +#ifdef TARGET_I386 + kvm_tpr_vcpu_start(env); +#endif + + kvm_arch_load_regs(env); + + /* signal VCPU creation */ current_env->created = 1; pthread_cond_signal(&qemu_vcpu_cond); /* and wait for machine initialization */ while (!qemu_system_ready) qemu_cond_wait(&qemu_system_cond); - pthread_mutex_unlock(&qemu_mutex); + + /* re-initialize cpu_single_env after re-acquiring qemu_mutex */ + cpu_single_env = env; kvm_main_loop_cpu(env); return NULL; -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] Don't sync mpstate to/from kernel when unneeded. 2009-10-14 13:52 [PATCHv2 1/2] Complete cpu initialization before signaling main thread Gleb Natapov @ 2009-10-14 13:52 ` Gleb Natapov 2009-11-11 23:33 ` Jan Kiszka 2009-10-14 18:35 ` [PATCHv2 1/2] Complete cpu initialization before signaling main thread Marcelo Tosatti 1 sibling, 1 reply; 5+ messages in thread From: Gleb Natapov @ 2009-10-14 13:52 UTC (permalink / raw) To: mtosatti; +Cc: kvm 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. 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; } -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] Don't sync mpstate to/from kernel when unneeded. 2009-10-14 13:52 ` [PATCH 2/2] Don't sync mpstate to/from kernel when unneeded Gleb Natapov @ 2009-11-11 23:33 ` Jan Kiszka 2009-11-13 0:33 ` Glauber Costa 0 siblings, 1 reply; 5+ messages in thread From: Jan Kiszka @ 2009-11-11 23:33 UTC (permalink / raw) To: Gleb Natapov; +Cc: mtosatti, kvm, Glauber Costa [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] Don't sync mpstate to/from kernel when unneeded. 2009-11-11 23:33 ` Jan Kiszka @ 2009-11-13 0:33 ` Glauber Costa 0 siblings, 0 replies; 5+ messages in thread From: Glauber Costa @ 2009-11-13 0:33 UTC (permalink / raw) To: Jan Kiszka; +Cc: Gleb Natapov, mtosatti, kvm On Thu, Nov 12, 2009 at 12:33:07AM +0100, Jan Kiszka wrote: > 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? It does not, since upstream qemu still does not support smp. I basically did not worried about it. > > 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; > > } > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCHv2 1/2] Complete cpu initialization before signaling main thread. 2009-10-14 13:52 [PATCHv2 1/2] Complete cpu initialization before signaling main thread Gleb Natapov 2009-10-14 13:52 ` [PATCH 2/2] Don't sync mpstate to/from kernel when unneeded Gleb Natapov @ 2009-10-14 18:35 ` Marcelo Tosatti 1 sibling, 0 replies; 5+ messages in thread From: Marcelo Tosatti @ 2009-10-14 18:35 UTC (permalink / raw) To: Gleb Natapov; +Cc: kvm On Wed, Oct 14, 2009 at 03:52:31PM +0200, Gleb Natapov wrote: > Otherwise some cpus may start executing code before others > are fully initialized. > > Signed-off-by: Gleb Natapov <gleb@redhat.com> Applied both, thanks. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-11-13 0:33 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-10-14 13:52 [PATCHv2 1/2] Complete cpu initialization before signaling main thread Gleb Natapov 2009-10-14 13:52 ` [PATCH 2/2] Don't sync mpstate to/from kernel when unneeded Gleb Natapov 2009-11-11 23:33 ` Jan Kiszka 2009-11-13 0:33 ` Glauber Costa 2009-10-14 18:35 ` [PATCHv2 1/2] Complete cpu initialization before signaling main thread Marcelo Tosatti
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.