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

login
register
mail settings
Submitter Andreas Färber
Date Jan. 23, 2013, 12:07 p.m.
Message ID <1358942867-8612-5-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/214927/
State New
Headers show

Comments

Andreas Färber - Jan. 23, 2013, 12:07 p.m.
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 <qemu-stable@nongnu.org>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-arm/cpu.c    |   17 +++++++++++++++++
 target-arm/helper.c |    6 ++++--
 2 Dateien geändert, 21 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-)
Peter Maydell - Jan. 23, 2013, 1:03 p.m.
On 23 January 2013 12:07, Andreas Färber <afaerber@suse.de> 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 <qemu-stable@nongnu.org>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  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)..."

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

>  }
>
>  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?

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

thanks
-- PMM
Andreas Färber - Jan. 23, 2013, 1:38 p.m.
Am 23.01.2013 14:03, schrieb Peter Maydell:
> On 23 January 2013 12:07, Andreas Färber <afaerber@suse.de> 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 <qemu-stable@nongnu.org>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  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
>
Peter Maydell - Jan. 23, 2013, 1:41 p.m.
On 23 January 2013 13:38, Andreas Färber <afaerber@suse.de> wrote:
> Am 23.01.2013 14:03, schrieb Peter Maydell:
>> On 23 January 2013 12:07, Andreas Färber <afaerber@suse.de> wrote:
>>> +    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.

Yes -- I prefer (!ptr) not because I think the code will be different
but because I think it is better style (and consistent with the
current code -- there are no instances of "== NULL" in target-arm
today).

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

Cool.

-- PMM
Andreas Färber - Jan. 23, 2013, 3:25 p.m.
Am 23.01.2013 14:41, schrieb Peter Maydell:
> On 23 January 2013 13:38, Andreas Färber <afaerber@suse.de> wrote:
>> Am 23.01.2013 14:03, schrieb Peter Maydell:
>>> On 23 January 2013 12:07, Andreas Färber <afaerber@suse.de> wrote:
>>>> +    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.
> 
> Yes -- I prefer (!ptr) not because I think the code will be different
> but because I think it is better style (and consistent with the
> current code -- there are no instances of "== NULL" in target-arm
> today).

Please see style-changed version here:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu-types
https://github.com/afaerber/qemu-cpu/commit/726554290fa69425d0e94e2e4fd2fdfeeb54e00c

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

https://lists.nongnu.org/archive/html/qemu-devel/2013-01/msg03606.html

> Cool.

Preview of rebased CPU realizefn here:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu-realize

Andreas
Peter Maydell - Jan. 23, 2013, 3:43 p.m.
On 23 January 2013 15:25, Andreas Färber <afaerber@suse.de> wrote:
> Am 23.01.2013 14:41, schrieb Peter Maydell:
>> On 23 January 2013 13:38, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 23.01.2013 14:03, schrieb Peter Maydell:
>>>> On 23 January 2013 12:07, Andreas Färber <afaerber@suse.de> wrote:
>>>>> +    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.
>>
>> Yes -- I prefer (!ptr) not because I think the code will be different
>> but because I think it is better style (and consistent with the
>> current code -- there are no instances of "== NULL" in target-arm
>> today).
>
> Please see style-changed version here:
> https://github.com/afaerber/qemu-cpu/commits/qom-cpu-types
> https://github.com/afaerber/qemu-cpu/commit/726554290fa69425d0e94e2e4fd2fdfeeb54e00c

Thanks.
Acked-by: Peter Maydell <peter.maydell@linaro.org>
for the arm related patches in this series.

-- PMM

Patch

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;
+    }
+
+    oc = object_class_by_name(cpu_model);
+    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;
 }
 
 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)));
     env = &cpu->env;
     env->cpu_model_str = cpu_model;
     arm_cpu_realize(cpu);