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

[Igor Mammedov <imammedo@redhat.com>]

On 05/23/2012 11:27 PM, Andreas Färber wrote:
> 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 jan.kiszka@siemens.com>
>>    - make it compile/work on i386-linux-user target
>>
>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>> ---
>>   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. :)
I'll fix it.

>
>> -        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.
sure, I'll add you link<X86CPU> patches and rebase on top of it

>
>> +    }
>> +#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.
I'll fix it.
>
> 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?

mce_init() ordering here is not important and it is less #ifndef-s this way.
yep, with Paolo's realize patch whole #ifndef block should be thrown away.
I can split it into a separate patch, that could be easily discarded when
Paolo's realize is committed.


>> +    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.

Ok, I'll split it into separate patch.
On the second thought there is no compelling reason to move this hunk in cpu.c
It could be left at board level, just moved in the beginning of pc_cpus_init()
and called only once there.

>
>>   }
>>
>>   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?
Any preference in what header file it should be?


-- 
-----
Thanks
   Igor