Re: [Qemu-stable] [PATCH qom-cpu for-1.4 04/14] target-arm: Detect attempt to instantiate non-CPU type in cpu_init()

[Andreas Färber]

Am 23.01.2013 14:03, schrieb Peter Maydell:
> On 23 January 2013 12:07, Andreas Färber <address@hidden> wrote:
>> Consolidate model checking into a new arm_cpu_class_by_name().
>>
>> If the name matches an existing type, also check whether that type is
>> actually (a sub-type of) TYPE_ARM_CPU.
>>
>> This fixes, e.g., -cpu tmp105 asserting.
>>
>> Cc: qemu-stable <address@hidden>
>> Signed-off-by: Andreas Färber <address@hidden>
>> ---
>>  target-arm/cpu.c    |   17 +++++++++++++++++
>>  target-arm/helper.c |    6 ++++--
>>  2 Dateien geändert, 21 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-)
>>
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index 07588a1..d85f251 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -201,6 +201,21 @@ void arm_cpu_realize(ARMCPU *cpu)
>>
>>  /* CPU models */
>>
>> +static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
>> +{
>> +    ObjectClass *oc;
>> +
>> +    if (cpu_model == NULL) {
>> +        return NULL;
>> +    }
> 
> explicit "== NULL" is kind of ugly; established style in
> target-arm/ is "if (!cpu_model)..."

I consistently use !foo only if foo is bool. Any decent compiler will
optimize this appropriately. It not being that way in helper.c most
likely is a symptom of you replacing my patch with your initfn approach. ;)

> 
>> +
>> +    oc = object_class_by_name(cpu_model);
> 
> I note that the object_class_by_name() implementation returns
> NULL for NULL input, though the documentation doesn't guarantee
> it will...
> 
>> +    if (oc == NULL || object_class_dynamic_cast(oc, TYPE_ARM_CPU) == NULL) {
>> +        return NULL;
>> +    }
>> +    return oc;
>> +}
>> +
>>  static void arm926_initfn(Object *obj)
>>  {
>>      ARMCPU *cpu = ARM_CPU(obj);
>> @@ -766,6 +781,8 @@ static void arm_cpu_class_init(ObjectClass *oc, void 
>> *data)
>>
>>      acc->parent_reset = cc->reset;
>>      cc->reset = arm_cpu_reset;
>> +
>> +    cc->class_by_name = arm_cpu_class_by_name;
> 
> Is this a class method because the plan is that eventually
> the code that instantiates the CPU object will become
> generic rather than target specific?

Yes, the plan as indicated in the CPUState realizefn series is to
generalize cpu_init() so that it only needs to know which base type to
operate on. I'm not yet sure how to handle CPU properties in a generic
way, but said series got three or four targets into a generic QOM'ish
form already.

> 
>>  }
>>
>>  static void cpu_register(const ARMCPUInfo *info)
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 37c34a1..4c29117 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -1262,12 +1262,14 @@ ARMCPU *cpu_arm_init(const char *cpu_model)
>>  {
>>      ARMCPU *cpu;
>>      CPUARMState *env;
>> +    ObjectClass *oc;
>>      static int inited = 0;
>>
>> -    if (!object_class_by_name(cpu_model)) {
>> +    oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model);
>> +    if (oc == NULL) {
>>          return NULL;
>>      }
>> -    cpu = ARM_CPU(object_new(cpu_model));
>> +    cpu = ARM_CPU(object_new(object_class_get_name(oc)));
> 
> Do we really have to convert back to the char* type
> name in order to instantiate an object given the class?

Unless someone adds a new function, I fear so... internally TypeImpl is
used as alternative but that's not really exposed so far.
CC'ing Anthony.

Cheers,
Andreas

> 
>>      env = &cpu->env;
>>      env->cpu_model_str = cpu_model;
>>      arm_cpu_realize(cpu);
>> --
>> 1.7.10.4
> 
> thanks
> -- PMM
> 


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