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

[Glauber Costa <glommer@redhat.com>]

> > 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.

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.