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

[Igor Mammedov <imammedo@redhat.com>]

On 05/22/2012 12:56 PM, Jan Kiszka wrote:
> 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? :)

Yep it works :)
  It's my bad, that due patches reshuffling I've moved this hunk into:
    [PATCH qom-next 2/5] target-i386: add cpu-model property to x86_cpu
  Will move it to this patch.

Thanks,
  Igor