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

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

On 2012-05-22 07:35, Igor Mammedov wrote:
> 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)
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/pc.c              |   32 +++++---------------------
>  target-i386/cpu.c    |   60 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  target-i386/helper.c |   39 --------------------------------
>  3 files changed, 65 insertions(+), 66 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 0eb0b73..677f9e0 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -894,37 +894,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) {
> -        if (kvm_irqchip_in_kernel()) {
> -            env->apic_state = qdev_create(NULL, "kvm-apic");
> -        } else {
> -            env->apic_state = qdev_create(NULL, "apic");
> -        }
> -        qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id);
> -        qdev_prop_set_ptr(env->apic_state, "cpu_env", env);
> -        qdev_init_nofail(env->apic_state);
> -    }
> -    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++) {
> -        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 538892d..0e804ea 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -31,6 +31,9 @@
>  
>  #include "hyperv.h"
>  
> +#include "hw/qdev.h"
> +#include "sysemu.h"
> +
>  /* feature flags taken from "Intel Processor Identification and the CPUID
>   * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
>   * between feature naming conventions, aliases may be added.
> @@ -1747,21 +1750,76 @@ 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;
> +    }
> +
> +    if (((env->cpuid_features & CPUID_APIC) || smp_cpus > 1)) {
> +        if (kvm_irqchip_in_kernel()) {
> +            env->apic_state = qdev_create(NULL, "kvm-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);
> +    }
> +}
> +
> +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);
>      }
>  }
>  
>  void x86_cpu_realize(Object *obj, Error **errp)
>  {
>      X86CPU *cpu = X86_CPU(obj);
> +    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;
> +        }
> +    }
>  
>      mce_init(cpu);
> -    qemu_init_vcpu(&cpu->env);
> +    qemu_init_vcpu(env);
> +    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);
>  
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 6fc67a9..443092e 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -947,34 +947,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);
> -}
> -
>  typedef struct MCEInjectionParams {
>      Monitor *mon;
>      CPUX86State *env;
> @@ -1161,20 +1133,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
> -    }
> -

Where does this hunk go to?

Did you test the result against TCG? :)

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux