Re: [PATCH 15/21] qemu-kvm: Clean up mpstate synchronization

Gleb Natapov
2010-02-02T12:24:03Z

On Tue, Feb 02, 2010 at 09:19:01AM +0100, Jan Kiszka wrote:
> Push mpstate reading/writing into kvm_arch_load/save_regs and, on x86,
> properly synchronize with halted in the accessor functions.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> hw/apic.c | 7 ----
> qemu-kvm-ia64.c | 4 ++-
> qemu-kvm-x86.c | 88 +++++++++++++++++++++++++++---------------------
> qemu-kvm.c | 30 -----------------
> qemu-kvm.h | 15 --------
> target-i386/machine.c | 6 ---
> target-ia64/machine.c | 3 ++
> 7 files changed, 55 insertions(+), 98 deletions(-)
>
> diff --git a/hw/apic.c b/hw/apic.c
> index 3e03e10..092c61e 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -507,13 +507,6 @@ void apic_init_reset(CPUState *env)
> s->wait_for_sipi = 1;
>
> env->halted = !(s->apicbase & MSR_IA32_APICBASE_BSP);
> -#ifdef KVM_CAP_MP_STATE
> - if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> - env->mp_state
> - = env->halted ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
> - kvm_load_mpstate(env);
> - }
> -#endif
> }
>
> static void apic_startup(APICState *s, int vector_num)
> diff --git a/qemu-kvm-ia64.c b/qemu-kvm-ia64.c
> index fc8110e..39bcbeb 100644
> --- a/qemu-kvm-ia64.c
> +++ b/qemu-kvm-ia64.c
> @@ -124,7 +124,9 @@ void kvm_arch_cpu_reset(CPUState *env)
> {
> if (kvm_irqchip_in_kernel(kvm_context)) {
> #ifdef KVM_CAP_MP_STATE
> - kvm_reset_mpstate(env->kvm_cpu_state.vcpu_ctx);
> + struct kvm_mp_state mp_state = {.mp_state = KVM_MP_STATE_UNINITIALIZED
> + };
> + kvm_set_mpstate(env, &mp_state);
> #endif
> } else {
> env->interrupt_request &= ~CPU_INTERRUPT_HARD;
> diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
> index 63cd095..6b5895f 100644
> --- a/qemu-kvm-x86.c
> +++ b/qemu-kvm-x86.c
> @@ -754,6 +754,48 @@ static int get_msr_entry(struct kvm_msr_entry *entry, CPUState *env)
> return 0;
> }
>
> +static void kvm_arch_save_mpstate(CPUState *env)
> +{
> +#ifdef KVM_CAP_MP_STATE
> + int r;
> + struct kvm_mp_state mp_state;
> +
> + r = kvm_get_mpstate(env, &mp_state);
> + if (r < 0) {
> + env->mp_state = -1;
> + } else {
> + env->mp_state = mp_state.mp_state;
> + if (kvm_irqchip_in_kernel()) {
> + env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
> + }
> + }
> +#else
> + env->mp_state = -1;
> +#endif
> +}
> +
> +static void kvm_arch_load_mpstate(CPUState *env)
> +{
> +#ifdef KVM_CAP_MP_STATE
> + struct kvm_mp_state mp_state;
> +
> + /*
> + * -1 indicates that the host did not support GET_MP_STATE ioctl,
> + * so don't touch it.
> + */
> + if (env->mp_state != -1) {
> + if (kvm_irqchip_in_kernel()) {
> + env->mp_state = env->halted ? KVM_MP_STATE_UNINITIALIZED :
> + KVM_MP_STATE_RUNNABLE;
When irqchip is in kernel env->halted doesn't contain any relevant
information, so this is incorrect. Actually env->halted is updated only
to show correct cpu state during "info cpus".

> + /* Avoid deadlock: no user space IRQ will ever clear it. */
And this comment explains why looking at env->halt when irqchip is in
kernel is wrong :)

> + env->halted = 0;
> + }
> + mp_state.mp_state = env->mp_state;
> + kvm_set_mpstate(env, &mp_state);
> + }
> +#endif
> +}
> +
> static void set_v8086_seg(struct kvm_segment *lhs, const SegmentCache *rhs)
> {
> lhs->selector = rhs->selector;
> @@ -926,6 +968,10 @@ void kvm_arch_load_regs(CPUState *env, int level)
> rc = kvm_set_msrs(env, msrs, n);
> if (rc == -1)
> perror("kvm_set_msrs FAILED");
> +
> + if (level >= KVM_PUT_RESET_STATE) {
> + kvm_arch_load_mpstate(env);
> + }
> }
>
> void kvm_load_tsc(CPUState *env)
> @@ -940,36 +986,6 @@ void kvm_load_tsc(CPUState *env)
> perror("kvm_set_tsc FAILED.\n");
> }
>
> -void kvm_arch_save_mpstate(CPUState *env)
> -{
> -#ifdef KVM_CAP_MP_STATE
> - int r;
> - struct kvm_mp_state mp_state;
> -
> - r = kvm_get_mpstate(env, &mp_state);
> - if (r < 0)
> - env->mp_state = -1;
> - else
> - env->mp_state = mp_state.mp_state;
> -#else
> - env->mp_state = -1;
> -#endif
> -}
> -
> -void kvm_arch_load_mpstate(CPUState *env)
> -{
> -#ifdef KVM_CAP_MP_STATE
> - struct kvm_mp_state mp_state = { .mp_state = env->mp_state };
> -
> - /*
> - * -1 indicates that the host did not support GET_MP_STATE ioctl,
> - * so don't touch it.
> - */
> - if (env->mp_state != -1)
> - kvm_set_mpstate(env, &mp_state);
> -#endif
> -}
> -
> void kvm_arch_save_regs(CPUState *env)
> {
> struct kvm_regs regs;
> @@ -1366,15 +1382,9 @@ void kvm_arch_cpu_reset(CPUState *env)
> {
> kvm_arch_reset_vcpu(env);
> kvm_put_vcpu_events(env);
> - if (!cpu_is_bsp(env)) {
> - if (kvm_irqchip_in_kernel()) {
> -#ifdef KVM_CAP_MP_STATE
> - kvm_reset_mpstate(env);
> -#endif
> - } else {
> - env->interrupt_request &= ~CPU_INTERRUPT_HARD;
> - env->halted = 1;
> - }
> + if (!cpu_is_bsp(env) && !kvm_irqchip_in_kernel()) {
> + env->interrupt_request &= ~CPU_INTERRUPT_HARD;
> + env->halted = 1;
> }
> }
>
> diff --git a/qemu-kvm.c b/qemu-kvm.c
> index 53030f1..efa6a29 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -1579,36 +1579,6 @@ void kvm_update_interrupt_request(CPUState *env)
> }
> }
>
> -static void kvm_do_load_mpstate(void *_env)
> -{
> - CPUState *env = _env;
> -
> - kvm_arch_load_mpstate(env);
> -}
> -
> -void kvm_load_mpstate(CPUState *env)
> -{
> - if (kvm_enabled() && qemu_system_ready && kvm_vcpu_inited(env))
> - on_vcpu(env, kvm_do_load_mpstate, env);
> -}
> -
> -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())
> - env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
> -#endif
> -}
> -
> -void kvm_save_mpstate(CPUState *env)
> -{
> - if (kvm_enabled())
> - on_vcpu(env, kvm_do_save_mpstate, env);
> -}
> -
> int kvm_cpu_exec(CPUState *env)
> {
> int r;
> diff --git a/qemu-kvm.h b/qemu-kvm.h
> index 6d785a0..aa7bcce 100644
> --- a/qemu-kvm.h
> +++ b/qemu-kvm.h
> @@ -299,16 +299,6 @@ int kvm_get_mpstate(CPUState *env, struct kvm_mp_state *mp_state);
> *
> */
> int kvm_set_mpstate(CPUState *env, struct kvm_mp_state *mp_state);
> -/*!
> - * * \brief Reset VCPU MP state
> - *
> - */
> -static inline int kvm_reset_mpstate(CPUState *env)
> -{
> - struct kvm_mp_state mp_state = {.mp_state = KVM_MP_STATE_UNINITIALIZED
> - };
> - return kvm_set_mpstate(env, &mp_state);
> -}
> #endif
>
> /*!
> @@ -874,8 +864,6 @@ static inline void kvm_inject_x86_mce(CPUState *cenv, int bank,
> int kvm_main_loop(void);
> int kvm_init_ap(void);
> int kvm_vcpu_inited(CPUState *env);
> -void kvm_load_mpstate(CPUState *env);
> -void kvm_save_mpstate(CPUState *env);
> void kvm_apic_init(CPUState *env);
> /* called from vcpu initialization */
> void qemu_kvm_load_lapic(CPUState *env);
> @@ -909,8 +897,6 @@ int kvm_arch_qemu_create_context(void);
>
> void kvm_arch_save_regs(CPUState *env);
> void kvm_arch_load_regs(CPUState *env, int level);
> -void kvm_arch_load_mpstate(CPUState *env);
> -void kvm_arch_save_mpstate(CPUState *env);
> int kvm_arch_has_work(CPUState *env);
> void kvm_arch_process_irqchip_events(CPUState *env);
> int kvm_arch_try_push_interrupts(void *opaque);
> @@ -979,7 +965,6 @@ void kvm_load_tsc(CPUState *env);
> #ifdef TARGET_I386
> #define qemu_kvm_has_pit_state2() (0)
> #endif
> -#define kvm_save_mpstate(env) do {} while(0)
> #define qemu_kvm_cpu_stop(env) do {} while(0)
> static inline void kvm_load_tsc(CPUState *env)
> {
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index bebde82..61e6a87 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -323,7 +323,6 @@ static void cpu_pre_save(void *opaque)
> int i;
>
> if (kvm_enabled()) {
> - kvm_save_mpstate(env);
> kvm_get_vcpu_events(env);
> }
>
> @@ -362,12 +361,7 @@ static int cpu_post_load(void *opaque, int version_id)
> tlb_flush(env, 1);
>
> if (kvm_enabled()) {
> - /* when in-kernel irqchip is used, env->halted causes deadlock
> - because no userspace IRQs will ever clear this flag */
> - env->halted = 0;
> -
> kvm_load_tsc(env);
> - kvm_load_mpstate(env);
> kvm_put_vcpu_events(env);
> }
>
> diff --git a/target-ia64/machine.c b/target-ia64/machine.c
> index fdbeeef..8cf5bdd 100644
> --- a/target-ia64/machine.c
> +++ b/target-ia64/machine.c
> @@ -4,6 +4,9 @@
> #include "exec-all.h"
> #include "qemu-kvm.h"
>
> +void kvm_arch_save_mpstate(CPUState *env);
> +void kvm_arch_load_mpstate(CPUState *env);
> +
> void cpu_save(QEMUFile *f, void *opaque)
> {
> CPUState *env = opaque;
> --
> 1.6.0.2
>
> --
> 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.