| Message ID | 1259671897-22232-6-git-send-email-glommer@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <glommer@redhat.com> > --- > 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.
>> +{ >> + if (kvm_enabled()) { > Is this ever called or intended to be called when kvm is disabled? > >> + kvm_cpu_flush_state(env); >> + } I don't think so. But this is here for consistency with its synchronize brother.
Glauber Costa wrote: >>> +{ >>> + if (kvm_enabled()) { >> Is this ever called or intended to be called when kvm is disabled? >> >>> + kvm_cpu_flush_state(env); >>> + } > > I don't think so. But this is here for consistency with its synchronize brother. Ack. As soon as someone starts to use this generic service outside code that is only built under CONFIG_KVM or called when kvm is enabled, things will break. Jan
On Wed, Dec 02, 2009 at 01:10:22PM +0100, Jan Kiszka wrote: > Glauber Costa wrote: > >>> +{ > >>> + if (kvm_enabled()) { > >> Is this ever called or intended to be called when kvm is disabled? > >> > >>> + kvm_cpu_flush_state(env); > >>> + } > > > > I don't think so. But this is here for consistency with its synchronize brother. > > Ack. As soon as someone starts to use this generic service outside code > that is only built under CONFIG_KVM or called when kvm is enabled, > things will break. > We can add the check when adding first such user. Code outside CONFIG_KVM can't use this since the function is in kvm.h header. I don't have really strong opinion about that. I personally use kvm_enabled() as a hint that code is generic for kvm and non-kvm case when reading code. -- Gleb.
Gleb Natapov wrote: > On Wed, Dec 02, 2009 at 01:10:22PM +0100, Jan Kiszka wrote: >> Glauber Costa wrote: >>>>> +{ >>>>> + if (kvm_enabled()) { >>>> Is this ever called or intended to be called when kvm is disabled? >>>> >>>>> + kvm_cpu_flush_state(env); >>>>> + } >>> I don't think so. But this is here for consistency with its synchronize brother. >> Ack. As soon as someone starts to use this generic service outside code >> that is only built under CONFIG_KVM or called when kvm is enabled, >> things will break. >> > We can add the check when adding first such user. Code outside > CONFIG_KVM can't use this since the function is in kvm.h header. > I don't have really strong opinion about that. I personally use > kvm_enabled() as a hint that code is generic for kvm and non-kvm > case when reading code. It's the same case as with cpu_synchronize_state, just that the latter is already used outside kvm code. Both services logically belong into a different header but are in kvm.h for simplicity reasons. Jan
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 } - 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()) { + kvm_cpu_flush_state(env); + } +} + #endif
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 <glommer@redhat.com> --- hw/apic-kvm.c | 5 ++++- kvm-all.c | 14 +++++++++----- kvm.h | 8 ++++++++ 3 files changed, 21 insertions(+), 6 deletions(-)