Re: [PATCH] execute kvm_init_vcpu in the end of pc_new_cpu

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

Glauber Costa wrote:
>>> diff --git a/qemu-kvm.c b/qemu-kvm.c
>>> index 8c0d463..8fd80c1 100644
>>> --- a/qemu-kvm.c
>>> +++ b/qemu-kvm.c
>>> @@ -435,6 +435,9 @@ static void *ap_main_loop(void *_env)
>>>      kvm_create_vcpu(kvm_context, env->cpu_index);
>>>      kvm_qemu_init_env(env);
>>>  
>>> +    /* APIC state creation takes place before we get here. So despite the fact that
>>> +     * apic_reset() (called by apic_init) will also load the apic state, we have to redo it here
>>> +     */
>>>  #ifdef USE_KVM_DEVICE_ASSIGNMENT
>>>      /* do ioperm for io ports of assigned devices */
>>>      LIST_FOREACH(data, &ioperm_head, entries)
>>> @@ -446,6 +449,8 @@ static void *ap_main_loop(void *_env)
>>>      current_env->kvm_cpu_state.created = 1;
>>>      pthread_cond_signal(&qemu_vcpu_cond);
>>>  
>>> +    qemu_kvm_load_lapic(env);
>>> +
>> This feels strange after a first glance, I need to look closer... Ah
>> wait, found one reason for this feeling: APIC is x86 stuff, but you are
>> patching generic code.
> Yeah, I don't disagree. I could wrap it inside an ifdef, but I don't see this
> as a strong enough reason to create yet another hook. Maybe we could put this
> inside kvm_qemu_init_env()? Although it is not exactly creating any env,
> at least it is arch specific...
> 
>>>      /* and wait for machine initialization */
>>>      while (!qemu_system_ready)
>>>  	qemu_cond_wait(&qemu_system_cond);
>>> @@ -463,6 +468,11 @@ void kvm_init_vcpu(CPUState *env)
>>>  	qemu_cond_wait(&qemu_vcpu_cond);
>>>  }
>>>  
>>> +int kvm_vcpu_inited(CPUState *env)
>>> +{
>>> +    return env->kvm_cpu_state.created;
>>> +}
>>> +
>>>  int kvm_init_ap(void)
>>>  {
>>>  #ifdef TARGET_I386
>>> diff --git a/qemu-kvm.h b/qemu-kvm.h
>>> index c0549df..6fa9d5a 100644
>>> --- a/qemu-kvm.h
>>> +++ b/qemu-kvm.h
>>> @@ -16,6 +16,7 @@ int kvm_main_loop(void);
>>>  int kvm_qemu_init(void);
>>>  int kvm_qemu_create_context(void);
>>>  int kvm_init_ap(void);
>>> +int kvm_vcpu_inited(CPUState *env);
>>>  void kvm_qemu_destroy(void);
>>>  void kvm_load_registers(CPUState *env);
>>>  void kvm_save_registers(CPUState *env);
>>> @@ -31,6 +32,9 @@ int kvm_update_guest_debug(CPUState *env, unsigned long reinject_trap);
>>>  int kvm_qemu_init_env(CPUState *env);
>>>  int kvm_qemu_check_extension(int ext);
>>>  void kvm_apic_init(CPUState *env);
>>> +/* called from vcpu initialization */
>>> +void qemu_kvm_load_lapic(CPUState *env);
>>> +
>>>  int kvm_set_irq(int irq, int level, int *status);
>>>  
>>>  int kvm_physical_memory_set_dirty_tracking(int enable);
>>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>>> index 719e31e..511b48c 100644
>>> --- a/target-i386/helper.c
>>> +++ b/target-i386/helper.c
>>> @@ -1696,7 +1696,5 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
>>>      kqemu_init(env);
>>>  #endif
>>>  
>>> -    qemu_init_vcpu(env);
>>> -
>>>      return env;
>>>  }
>> The reordering of qemu_init_vcpu could also simplify reset management (I
>> have a patch pending that adds a kvm hook to apic reset for solving it
>> within the existing scheme). But I would suggest to get an ack from
>> upstream first, or better even merge this pattern there and then adjust
>> qemu-kvm. The other way around is calling for troubles if qemu sticks
>> with a different approach.
> 
> I've just sent a couple of patches do upstream qemu that moves everything inside
> cpu_x86_init, and only calls kvm_vcpu_init when everything else is already
> initialized. This includes reset management.

Missed that, having a look now.

> 
> The reason I sent this patch separatedly, is that we would have to deal with
> the fact that the first call to SET_LAPIC would fail anyway, this is qemu-kvm specific.
> 
> And upstream qemu does not have pc_new_cpu, so the clash would not be that big.

Yes, but it has qemu_vcpu_init. The only meta-difference is that
upstream has no in-kernel LAPIC yet. It also has to update the kvm state
when changing APIC stuff (its base specifically).

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux