Re: [Qemu-devel] [PATCH qom-next 5/6] target-i386: make initialize CPU in QOM way

[Andreas Färber]

Am 23.05.2012 18:39, schrieb Igor Mammedov:
> Make CPU creation/initialization consistent with QOM object
> behavior in this, by moving tcg and apic initialization from board
> level into CPU's initfn/realize calls and cpu_model property setter.
> 
> Which makes CPU object self-sufficient in respect of creation/initialization
> and matches a typical object creation sequence, i.e.:
>   - create CPU instance
>   - set properties
>   - realize object - (x86_cpu_realize will be converted into realize
>       property setter, when it is implemented)
> 
> v2:
>   - fix moving of tcg_* initialization into cpu.c from helper.c
>       spotted-by: <Jan Kiszka address@hidden>
>   - make it compile/work on i386-linux-user target
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
>  hw/pc.c              |   45 ++++------------------------
>  target-i386/cpu.c    |   81 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  target-i386/helper.c |   39 ------------------------
>  3 files changed, 85 insertions(+), 80 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 1ccfc6e..d7845ea 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c

> @@ -911,30 +891,17 @@ static void pc_cpu_reset(void *opaque)
>      cpu_reset(CPU(cpu));
>  }
>  
> -static X86CPU *pc_new_cpu(const char *cpu_model)
> -{
> -    X86CPU *cpu;
> -    CPUX86State *env;
> -
> -    cpu = cpu_x86_init(cpu_model);
> -    if (cpu == NULL) {
> -        exit(1);
> -    }
> -    env = &cpu->env;
> -    if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
> -        env->apic_state = apic_init(env, env->cpuid_apic_id);
> -    }
> -    qemu_register_reset(pc_cpu_reset, cpu);
> -    pc_cpu_reset(cpu);
> -    return cpu;
> -}
> -
>  void pc_cpus_init(const char *cpu_model)
>  {
> +    X86CPU *cpu;
>      int i;
>  
>      for(i = 0; i < smp_cpus; i++) {

If we do changes here, please add the missing space. :)

> -        pc_new_cpu(cpu_model);
> +        cpu = cpu_x86_init(cpu_model);
> +        if (cpu == NULL) {
> +            exit(1);
> +        }
> +        qemu_register_reset(pc_cpu_reset, cpu);
>      }
>  }
>  
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index e655129..99ef891 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c

> @@ -1749,24 +1753,89 @@ static void x86_set_cpu_model(Object *obj, const char 
> *value, Error **errp)
>      if (cpu_x86_register(cpu, env->cpu_model_str) < 0) {
>          fprintf(stderr, "Unable to find x86 CPU definition\n");
>          error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> +        return;
> +    }
> +
> +#ifndef CONFIG_USER_ONLY
> +    if (((env->cpuid_features & CPUID_APIC) || smp_cpus > 1)) {
> +        if (kvm_irqchip_in_kernel()) {
> +            env->apic_state = qdev_create(NULL, "kvm-apic");
> +        } else if (xen_enabled()) {
> +            env->apic_state = qdev_create(NULL, "xen-apic");
> +        } else {
> +            env->apic_state = qdev_create(NULL, "apic");
> +        }
> +        object_property_add_child(OBJECT(cpu), "apic",
> +            OBJECT(env->apic_state), NULL);
> +
> +        qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id);
> +        qdev_prop_set_ptr(env->apic_state, "cpu_env", env);

I'd like to avoid re-adding this set_ptr(). We can cherry-pick my
link<X86CPU> property from QOM CPUState part 4 series.

> +    }
> +#endif
> +}
> +
> +#ifndef CONFIG_USER_ONLY
> +static CPUDebugExcpHandler *prev_debug_excp_handler;
> +
> +static void breakpoint_handler(CPUX86State *env)
> +{
> +    CPUBreakpoint *bp;
> +
> +    if (env->watchpoint_hit) {
> +        if (env->watchpoint_hit->flags & BP_CPU) {
> +            env->watchpoint_hit = NULL;
> +            if (check_hw_breakpoints(env, 0)) {
> +                raise_exception_env(EXCP01_DB, env);
> +            } else {
> +                cpu_resume_from_signal(env, NULL);
> +            }
> +        }
> +    } else {
> +        QTAILQ_FOREACH(bp, &env->breakpoints, entry)
> +            if (bp->pc == env->eip) {
> +                if (bp->flags & BP_CPU) {
> +                    check_hw_breakpoints(env, 1);
> +                    raise_exception_env(EXCP01_DB, env);
> +                }
> +                break;
> +            }
> +    }
> +    if (prev_debug_excp_handler) {
> +        prev_debug_excp_handler(env);
>      }
>  }
> +#endif
>  
>  void x86_cpu_realize(Object *obj, Error **errp)
>  {
>      X86CPU *cpu = X86_CPU(obj);
> +#ifndef CONFIG_USER_ONLY
> +    CPUX86State *env = &cpu->env;
> +
> +    if (env->apic_state) {
> +        if (qdev_init(env->apic_state) < 0) {
> +            error_set(errp, QERR_DEVICE_INIT_FAILED,
> +                object_get_typename(OBJECT(env->apic_state)));
> +            return;
> +        }
> +    }
> +#endif
>  
>      mce_init(cpu);
> -    qemu_init_vcpu(&cpu->env);
> +    qemu_init_vcpu(env);

This only works because currently qemu_init_vcpu() is a no-op macro that
doesn't use the parameter. Please don't change it back, I guess it's a
mismerge.

We can avoid the env variable if I finish merging Paolo's series - by
realizing the CPU the APIC as its child would get realized, too. Is the
ordering before mce_init() mandatory here or is it just to reduce the
#ifndef'ery?

> +    cpu_reset(CPU(cpu));
>  }
>  
>  static void x86_cpu_initfn(Object *obj)
>  {
>      X86CPU *cpu = X86_CPU(obj);
>      CPUX86State *env = &cpu->env;
> +    static int inited;
>  
>      cpu_exec_init(env);
>  
> +    env->cpuid_apic_id = env->cpu_index;
> +
>      object_property_add(obj, "family", "int",
>                          x86_cpuid_version_get_family,
>                          x86_cpuid_version_set_family, NULL, NULL, NULL);
> @@ -1795,7 +1864,15 @@ static void x86_cpu_initfn(Object *obj)
>      object_property_add_str(obj, "cpu-model",
>          x86_get_cpu_model, x86_set_cpu_model, NULL);
>  
> -    env->cpuid_apic_id = env->cpu_index;
> +    /* init various static tables used in TCG mode */
> +    if (tcg_enabled() && !inited) {
> +        inited = 1;
> +        optimize_flags_init();
> +#ifndef CONFIG_USER_ONLY
> +        prev_debug_excp_handler =
> +            cpu_set_debug_excp_handler(breakpoint_handler);
> +#endif
> +    }

Did you forget to put that into its own patch or did that not work?
My idea was to have it first in the series so that other changes here
and elsewhere can be rebased onto it.

Also I wonder whether it would better be placed into the class_init? I'd
tend towards initfn because that will not be invoked during type
enumeration.

>  }
>  
>  static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index fbaeeea..38ac25d 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -941,34 +941,6 @@ int check_hw_breakpoints(CPUX86State *env, int 
> force_dr6_update)
>      return hit_enabled;
>  }
>  
> -static CPUDebugExcpHandler *prev_debug_excp_handler;
> -
> -static void breakpoint_handler(CPUX86State *env)
> -{
> -    CPUBreakpoint *bp;
> -
> -    if (env->watchpoint_hit) {
> -        if (env->watchpoint_hit->flags & BP_CPU) {
> -            env->watchpoint_hit = NULL;
> -            if (check_hw_breakpoints(env, 0))
> -                raise_exception_env(EXCP01_DB, env);
> -            else
> -                cpu_resume_from_signal(env, NULL);
> -        }
> -    } else {
> -        QTAILQ_FOREACH(bp, &env->breakpoints, entry)
> -            if (bp->pc == env->eip) {
> -                if (bp->flags & BP_CPU) {
> -                    check_hw_breakpoints(env, 1);
> -                    raise_exception_env(EXCP01_DB, env);
> -                }
> -                break;
> -            }
> -    }
> -    if (prev_debug_excp_handler)
> -        prev_debug_excp_handler(env);
> -}
> -

I wonder if that could rather stay here as non-static?

>  typedef struct MCEInjectionParams {
>      Monitor *mon;
>      CPUX86State *env;
> @@ -1155,20 +1127,9 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>  {
>      X86CPU *cpu;
>      Error *errp = NULL;
> -    static int inited;
>  
>      cpu = X86_CPU(object_new(TYPE_X86_CPU));
>  
> -    /* init various static tables used in TCG mode */
> -    if (tcg_enabled() && !inited) {
> -        inited = 1;
> -        optimize_flags_init();
> -#ifndef CONFIG_USER_ONLY
> -        prev_debug_excp_handler =
> -            cpu_set_debug_excp_handler(breakpoint_handler);
> -#endif
> -    }
> -
>      if (cpu_model) {
>          object_property_set_str(OBJECT(cpu), cpu_model, "cpu-model", &errp);
>      } else {

/-F

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg