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

[Jan Kiszka <jan.kiszka@siemens.com>]

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

OK, copied from apic_init_reset, see above. So that hunk was probably at
least useless, and now it's harmfull. Will drop this and only sync from
mp_state -> halted.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux