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

[Andreas Färber]

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.

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.

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