Re: [Qemu-devel] [PATCH 02/12] target-i386: split APIC creation from initialization in x86_cpu_realizefn()

[Igor Mammedov]

On Thu, 04 Apr 2013 10:59:55 +0200
Andreas Färber <address@hidden> wrote:

> Am 21.03.2013 15:28, schrieb Igor Mammedov:
> > When APIC is hotplugged during CPU hotplug, device_set_realized()
> > calls device_reset() on it. And if QEMU runs in KVM mode, following
> > call chain will fail:
> >     apic_reset_common()
> >         -> kvm_apic_vapic_base_update()
> >             -> kvm_vcpu_ioctl(cpu->kvm_fd,...)
> > due to cpu->kvm_fd not being initialized yet.
> > 
> > cpu->kvm_fd is initialized during qemu_init_vcpu() call but 
> > x86_cpu_apic_init()
> > can't be moved after it because kvm_init_vcpu() -> kvm_arch_reset_vcpu()
> > relies on APIC to determine if CPU is BSP for setting initial env->mp_state.
> > 
> > So split APIC device creation from its initialization and realize APIC
> > after CPU is created, when it's safe to call APIC's reset method.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >  target-i386/cpu.c |   24 +++++++++++++++++++++---
> >  1 files changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index e905bcf..affbb76 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2051,9 +2051,8 @@ static void mce_init(X86CPU *cpu)
> >  #define MSI_ADDR_BASE 0xfee00000
> >  
> >  #ifndef CONFIG_USER_ONLY
> > -static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
> > +static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
> >  {
> > -    static int apic_mapped;
> >      CPUX86State *env = &cpu->env;
> >      APICCommonState *apic;
> >      const char *apic_type = "apic";
> > @@ -2076,6 +2075,16 @@ static void x86_cpu_apic_init(X86CPU *cpu, Error 
> > **errp)
> >      /* TODO: convert to link<> */
> >      apic = APIC_COMMON(env->apic_state);
> >      apic->cpu = cpu;
> > +}
> > +
> > +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
> > +{
> > +    CPUX86State *env = &cpu->env;
> > +    static int apic_mapped;
> > +
> > +    if (env->apic_state == NULL) {
> > +        return;
> > +    }
> >  
> >      if (qdev_init(env->apic_state)) {
> >          error_setg(errp, "APIC device '%s' could not be initialized",
> > @@ -2093,6 +2102,10 @@ static void x86_cpu_apic_init(X86CPU *cpu, Error 
> > **errp)
> >          apic_mapped = 1;
> >      }
> >  }
> > +#else
> > +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
> > +{
> > +}
> >  #endif
> >  
> >  static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> > @@ -2143,7 +2156,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
> > **errp)
> >      qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
> >  
> >      if (cpu->env.cpuid_features & CPUID_APIC || smp_cpus > 1) {
> > -        x86_cpu_apic_init(cpu, &local_err);
> > +        x86_cpu_apic_create(cpu, &local_err);
> >          if (local_err != NULL) {
> >              goto out;
> >          }
> > @@ -2152,6 +2165,11 @@ static void x86_cpu_realizefn(DeviceState *dev, 
> > Error **errp)
> >  
> >      mce_init(cpu);
> >      qemu_init_vcpu(&cpu->env);
> > +
> > +    x86_cpu_apic_init(cpu, &local_err);
> 
> If we use the function like this, my request on IRC was to rename it to
> _realize please instead of _init.
Sure, I'll do it.
> 
> What's the plan for APIC link<>s above? I'm not opposed to the function
> split, but I wondered if - since we know there are only three possible
> APIC types - a union would allow us to make this an _initialize
> function, more closely following the QOM workflow? Could be done as
> follow-up.
I plan to post follow on for APIC links<> after this is in, based on your
original patch.
But for links to work we need CPU to have parent before it's realize is
called. I'll add qdev patch to this series that in device_set_realized() sets
parent before child's realize() is called. That would make
device_set_realized() consistent with device_add() where parent set before
realize() and will allow to use links<> in realize() of children of DEVICE.

About union, I've tried that it some time ago but gave up due to another
header deps hell it unleashes when embedding APIC in CPU.
It was doable though, perhaps after CPU unplug there will be time for it.

> 
> Andreas
> 
> > +    if (local_err != NULL) {
> > +        goto out;
> > +    }
> >      cpu_reset(CPU(cpu));
> >  
> >      xcc->parent_realize(dev, &local_err);
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> 


-- 
Regards,
  Igor