Patchwork [2/7] target-alpha: Turn CPU definitions into subclasses

login
register
mail settings
Submitter Andreas Färber
Date Oct. 31, 2012, 3:03 a.m.
Message ID <1351652644-18687-3-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/195725/
State New
Headers show

Comments

Andreas Färber - Oct. 31, 2012, 3:03 a.m.
Make TYPE_ALPHA_CPU abstract and default to creating type "ev67".

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-alpha/cpu.c       |  157 +++++++++++++++++++++++++++++++++++++++++++++-
 target-alpha/translate.c |   49 +++------------
 2 Dateien geändert, 163 Zeilen hinzugefügt(+), 43 Zeilen entfernt(-)
Richard Henderson - Oct. 31, 2012, 4:57 a.m.
On 2012-10-31 14:03, Andreas Färber wrote:
> +static const AlphaCPUInfo alpha_cpus[] = {
> +    { .name = "ev4",     .initfn = ev4_cpu_initfn },
> +    { .name = "ev5",     .initfn = ev5_cpu_initfn },
> +    { .name = "ev56",    .initfn = ev56_cpu_initfn },
> +    { .name = "pca56",   .initfn = pca56_cpu_initfn },
> +    { .name = "ev6",     .initfn = ev6_cpu_initfn },
> +    { .name = "ev67",    .initfn = ev67_cpu_initfn },
> +    { .name = "ev68",    .initfn = ev68_cpu_initfn },
> +    { .name = "21064",   .initfn = alpha_21064_cpu_initfn },
> +    { .name = "21164",   .initfn = alpha_21164_cpu_initfn },
> +    { .name = "21164a",  .initfn = alpha_21164a_cpu_initfn },
> +    { .name = "21164pc", .initfn = alpha_21164pc_cpu_initfn },
> +    { .name = "21264",   .initfn = alpha_21264_cpu_initfn },
> +    { .name = "21264a",  .initfn = alpha_21264a_cpu_initfn },
> +};

The "2*" names are aliases of the "ev*" names.  There's no need for so
much duplication.  And for that matter, "ev68" is no different from "ev67"
at the level for which we emulate.  In hw, it was more cache and a faster
multiply implementation.


r~
Andreas Färber - Dec. 6, 2012, 10:11 a.m.
Am 31.10.2012 05:57, schrieb Richard Henderson:
> On 2012-10-31 14:03, Andreas Färber wrote:
>> +static const AlphaCPUInfo alpha_cpus[] = {
>> +    { .name = "ev4",     .initfn = ev4_cpu_initfn },
>> +    { .name = "ev5",     .initfn = ev5_cpu_initfn },
>> +    { .name = "ev56",    .initfn = ev56_cpu_initfn },
>> +    { .name = "pca56",   .initfn = pca56_cpu_initfn },
>> +    { .name = "ev6",     .initfn = ev6_cpu_initfn },
>> +    { .name = "ev67",    .initfn = ev67_cpu_initfn },
>> +    { .name = "ev68",    .initfn = ev68_cpu_initfn },
>> +    { .name = "21064",   .initfn = alpha_21064_cpu_initfn },
>> +    { .name = "21164",   .initfn = alpha_21164_cpu_initfn },
>> +    { .name = "21164a",  .initfn = alpha_21164a_cpu_initfn },
>> +    { .name = "21164pc", .initfn = alpha_21164pc_cpu_initfn },
>> +    { .name = "21264",   .initfn = alpha_21264_cpu_initfn },
>> +    { .name = "21264a",  .initfn = alpha_21264a_cpu_initfn },
>> +};
> 
> The "2*" names are aliases of the "ev*" names.  There's no need for so
> much duplication.  And for that matter, "ev68" is no different from "ev67"
> at the level for which we emulate.  In hw, it was more cache and a faster
> multiply implementation.

Clearly I know little to nothing about Alpha CPU models. :)
Regarding ev68, we'll need to carry it for backwards compatibility; can
we assume that the Alpha ISA is dead? Then I could drop this shrinking
array and make, e.g., ev68 a trivial subclass of ev67.

The name scheme we are heading towards now looks like <name>-alpha-cpu.
Did I understand you correctly that we would want, e.g., ev4-alpha-cpu
as type and have "21064" map to it? Or the other way around?

Andreas
Eduardo Habkost - Dec. 6, 2012, 3:29 p.m.
On Wed, Oct 31, 2012 at 04:03:59AM +0100, Andreas Färber wrote:
[...]
> +static void alpha_cpu_register(const AlphaCPUInfo *info)
> +{
> +    TypeInfo type_info = {
> +        .name = info->name,
> +        .parent = TYPE_ALPHA_CPU,
> +        .instance_init = info->initfn,
> +    };
> +
> +    type_register_static(&type_info);

You should use type_register() instead of type_register_static(), here.
Andreas Färber - Dec. 6, 2012, 3:51 p.m.
Am 06.12.2012 16:29, schrieb Eduardo Habkost:
> On Wed, Oct 31, 2012 at 04:03:59AM +0100, Andreas Färber wrote:
> [...]
>> +static void alpha_cpu_register(const AlphaCPUInfo *info)
>> +{
>> +    TypeInfo type_info = {
>> +        .name = info->name,
>> +        .parent = TYPE_ALPHA_CPU,
>> +        .instance_init = info->initfn,
>> +    };
>> +
>> +    type_register_static(&type_info);
> 
> You should use type_register() instead of type_register_static(), here.

I still don't understand why. (CC'ing Anthony, Paolo, Peter)

The TypeInfo argument is in no way retained inside
qom/object.c:type_register_internal().
Therefore the lifetime of TypeInfo should be completely irrelevant for
the static/non-static decision and the documentation should be fixed IMO.
Is there a reason to do it differently? What would we want to do with
TypeInfo after transfer of its field values to TypeImpl?

FWIW if, as suggested earlier, we don't loop over Alpha CPU models in
favor of a handful of trivial static TypeInfos, it becomes irrelevant
for this patch but I'd still like to understand for the remaining
architectures.

Regards,
Andreas
Eduardo Habkost - Dec. 6, 2012, 4:09 p.m.
On Thu, Dec 06, 2012 at 04:51:31PM +0100, Andreas Färber wrote:
> Am 06.12.2012 16:29, schrieb Eduardo Habkost:
> > On Wed, Oct 31, 2012 at 04:03:59AM +0100, Andreas Färber wrote:
> > [...]
> >> +static void alpha_cpu_register(const AlphaCPUInfo *info)
> >> +{
> >> +    TypeInfo type_info = {
> >> +        .name = info->name,
> >> +        .parent = TYPE_ALPHA_CPU,
> >> +        .instance_init = info->initfn,
> >> +    };
> >> +
> >> +    type_register_static(&type_info);
> > 
> > You should use type_register() instead of type_register_static(), here.
> 
> I still don't understand why. (CC'ing Anthony, Paolo, Peter)
> 
> The TypeInfo argument is in no way retained inside
> qom/object.c:type_register_internal().
> Therefore the lifetime of TypeInfo should be completely irrelevant for
> the static/non-static decision and the documentation should be fixed IMO.
> Is there a reason to do it differently? What would we want to do with
> TypeInfo after transfer of its field values to TypeImpl?

The current implementation doesn't matter. It can change at any minute. The
interface, on the other hand, is documented as:

  type_register_static:
  @info: The #TypeInfo of the new type.

  @info and all of the strings it points to should exist for the life time
  that the type is registered.
Andreas Färber - Dec. 6, 2012, 4:21 p.m.
Am 06.12.2012 17:09, schrieb Eduardo Habkost:
> On Thu, Dec 06, 2012 at 04:51:31PM +0100, Andreas Färber wrote:
>> Am 06.12.2012 16:29, schrieb Eduardo Habkost:
>>> On Wed, Oct 31, 2012 at 04:03:59AM +0100, Andreas Färber wrote:
>>> [...]
>>>> +static void alpha_cpu_register(const AlphaCPUInfo *info)
>>>> +{
>>>> +    TypeInfo type_info = {
>>>> +        .name = info->name,
>>>> +        .parent = TYPE_ALPHA_CPU,
>>>> +        .instance_init = info->initfn,
>>>> +    };
>>>> +
>>>> +    type_register_static(&type_info);
>>>
>>> You should use type_register() instead of type_register_static(), here.
>>
>> I still don't understand why. (CC'ing Anthony, Paolo, Peter)
>>
>> The TypeInfo argument is in no way retained inside
>> qom/object.c:type_register_internal().
>> Therefore the lifetime of TypeInfo should be completely irrelevant for
>> the static/non-static decision and the documentation should be fixed IMO.
>> Is there a reason to do it differently? What would we want to do with
>> TypeInfo after transfer of its field values to TypeImpl?
> 
> The current implementation doesn't matter. It can change at any minute. The
> interface, on the other hand, is documented as:
> 
>   type_register_static:
>   @info: The #TypeInfo of the new type.
> 
>   @info and all of the strings it points to should exist for the life time
>   that the type is registered.

Both implementation and documentation can be changed. My question is,
why does the documentation say this and where does Anthony (or Paolo)
want to go with the current implementation that makes this necessary.

Like, if we switched to C++, we would drop both registration functions
completely.

Andreas
Richard Henderson - Dec. 7, 2012, 1:58 p.m.
On 2012-12-06 04:11, Andreas Färber wrote:
>> The "2*" names are aliases of the "ev*" names.  There's no need for so
>> much duplication.  And for that matter, "ev68" is no different from "ev67"
>> at the level for which we emulate.  In hw, it was more cache and a faster
>> multiply implementation.
> 
> Clearly I know little to nothing about Alpha CPU models. :)
> Regarding ev68, we'll need to carry it for backwards compatibility; can
> we assume that the Alpha ISA is dead? Then I could drop this shrinking
> array and make, e.g., ev68 a trivial subclass of ev67.

Yes, the ISA is dead.  We're also nearing the end of "extended" hardware
support for Alpha systems from HP.

> The name scheme we are heading towards now looks like <name>-alpha-cpu.
> Did I understand you correctly that we would want, e.g., ev4-alpha-cpu
> as type and have "21064" map to it?

Correct.  That corresponds more closely to how the compiler tools are
configured (e.g. alphaev67-linux, not alpha21164-linux).


r~

Patch

diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
index 11a19eb..e1a5739 100644
--- a/target-alpha/cpu.c
+++ b/target-alpha/cpu.c
@@ -23,6 +23,145 @@ 
 #include "qemu-common.h"
 
 
+/* Models */
+
+static void ev4_cpu_initfn(Object *obj)
+{
+    AlphaCPU *cpu = ALPHA_CPU(obj);
+    CPUAlphaState *env = &cpu->env;
+
+    env->implver = IMPLVER_2106x;
+}
+
+static void ev5_cpu_initfn(Object *obj)
+{
+    AlphaCPU *cpu = ALPHA_CPU(obj);
+    CPUAlphaState *env = &cpu->env;
+
+    env->implver = IMPLVER_21164;
+}
+
+static void ev56_cpu_initfn(Object *obj)
+{
+    AlphaCPU *cpu = ALPHA_CPU(obj);
+    CPUAlphaState *env = &cpu->env;
+
+    env->implver = IMPLVER_21164;
+    env->amask = AMASK_BWX;
+}
+
+static void pca56_cpu_initfn(Object *obj)
+{
+    AlphaCPU *cpu = ALPHA_CPU(obj);
+    CPUAlphaState *env = &cpu->env;
+
+    env->implver = IMPLVER_21164;
+    env->amask = AMASK_BWX | AMASK_MVI;
+}
+
+static void ev6_cpu_initfn(Object *obj)
+{
+    AlphaCPU *cpu = ALPHA_CPU(obj);
+    CPUAlphaState *env = &cpu->env;
+
+    env->implver = IMPLVER_21264;
+    env->amask = AMASK_BWX | AMASK_FIX | AMASK_MVI | AMASK_TRAP;
+}
+
+static void ev67_cpu_initfn(Object *obj)
+{
+    AlphaCPU *cpu = ALPHA_CPU(obj);
+    CPUAlphaState *env = &cpu->env;
+
+    env->implver = IMPLVER_21264;
+    env->amask = AMASK_BWX | AMASK_FIX | AMASK_CIX | AMASK_MVI | AMASK_TRAP |
+                 AMASK_PREFETCH;
+}
+
+static void ev68_cpu_initfn(Object *obj)
+{
+    AlphaCPU *cpu = ALPHA_CPU(obj);
+    CPUAlphaState *env = &cpu->env;
+
+    env->implver = IMPLVER_21264;
+    env->amask = AMASK_BWX | AMASK_FIX | AMASK_CIX | AMASK_MVI | AMASK_TRAP |
+                 AMASK_PREFETCH;
+}
+
+static void alpha_21064_cpu_initfn(Object *obj)
+{
+    AlphaCPU *cpu = ALPHA_CPU(obj);
+    CPUAlphaState *env = &cpu->env;
+
+    env->implver = IMPLVER_2106x;
+}
+
+static void alpha_21164_cpu_initfn(Object *obj)
+{
+    AlphaCPU *cpu = ALPHA_CPU(obj);
+    CPUAlphaState *env = &cpu->env;
+
+    env->implver = IMPLVER_21164;
+}
+
+static void alpha_21164a_cpu_initfn(Object *obj)
+{
+    AlphaCPU *cpu = ALPHA_CPU(obj);
+    CPUAlphaState *env = &cpu->env;
+
+    env->implver = IMPLVER_21164;
+    env->amask = AMASK_BWX;
+}
+
+static void alpha_21164pc_cpu_initfn(Object *obj)
+{
+    AlphaCPU *cpu = ALPHA_CPU(obj);
+    CPUAlphaState *env = &cpu->env;
+
+    env->implver = IMPLVER_21164;
+    env->amask = AMASK_BWX | AMASK_MVI;
+}
+
+static void alpha_21264_cpu_initfn(Object *obj)
+{
+    AlphaCPU *cpu = ALPHA_CPU(obj);
+    CPUAlphaState *env = &cpu->env;
+
+    env->implver = IMPLVER_21264;
+    env->amask = AMASK_BWX | AMASK_FIX | AMASK_MVI | AMASK_TRAP;
+}
+
+static void alpha_21264a_cpu_initfn(Object *obj)
+{
+    AlphaCPU *cpu = ALPHA_CPU(obj);
+    CPUAlphaState *env = &cpu->env;
+
+    env->implver = IMPLVER_21264;
+    env->amask = AMASK_BWX | AMASK_FIX | AMASK_CIX | AMASK_MVI | AMASK_TRAP |
+                 AMASK_PREFETCH;
+}
+
+typedef struct AlphaCPUInfo {
+    const char *name;
+    void (*initfn)(Object *obj);
+} AlphaCPUInfo;
+
+static const AlphaCPUInfo alpha_cpus[] = {
+    { .name = "ev4",     .initfn = ev4_cpu_initfn },
+    { .name = "ev5",     .initfn = ev5_cpu_initfn },
+    { .name = "ev56",    .initfn = ev56_cpu_initfn },
+    { .name = "pca56",   .initfn = pca56_cpu_initfn },
+    { .name = "ev6",     .initfn = ev6_cpu_initfn },
+    { .name = "ev67",    .initfn = ev67_cpu_initfn },
+    { .name = "ev68",    .initfn = ev68_cpu_initfn },
+    { .name = "21064",   .initfn = alpha_21064_cpu_initfn },
+    { .name = "21164",   .initfn = alpha_21164_cpu_initfn },
+    { .name = "21164a",  .initfn = alpha_21164a_cpu_initfn },
+    { .name = "21164pc", .initfn = alpha_21164pc_cpu_initfn },
+    { .name = "21264",   .initfn = alpha_21264_cpu_initfn },
+    { .name = "21264a",  .initfn = alpha_21264a_cpu_initfn },
+};
+
 static void alpha_cpu_initfn(Object *obj)
 {
     AlphaCPU *cpu = ALPHA_CPU(obj);
@@ -41,18 +180,34 @@  static void alpha_cpu_initfn(Object *obj)
     env->fen = 1;
 }
 
+static void alpha_cpu_register(const AlphaCPUInfo *info)
+{
+    TypeInfo type_info = {
+        .name = info->name,
+        .parent = TYPE_ALPHA_CPU,
+        .instance_init = info->initfn,
+    };
+
+    type_register_static(&type_info);
+}
+
 static const TypeInfo alpha_cpu_type_info = {
     .name = TYPE_ALPHA_CPU,
     .parent = TYPE_CPU,
     .instance_size = sizeof(AlphaCPU),
     .instance_init = alpha_cpu_initfn,
-    .abstract = false,
+    .abstract = true,
     .class_size = sizeof(AlphaCPUClass),
 };
 
 static void alpha_cpu_register_types(void)
 {
+    int i;
+
     type_register_static(&alpha_cpu_type_info);
+    for (i = 0; i < ARRAY_SIZE(alpha_cpus); i++) {
+        alpha_cpu_register(&alpha_cpus[i]);
+    }
 }
 
 type_init(alpha_cpu_register_types)
diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index f707d8d..6ee031d 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -3493,56 +3493,21 @@  void gen_intermediate_code_pc (CPUAlphaState *env, struct TranslationBlock *tb)
     gen_intermediate_code_internal(env, tb, 1);
 }
 
-struct cpu_def_t {
-    const char *name;
-    int implver, amask;
-};
-
-static const struct cpu_def_t cpu_defs[] = {
-    { "ev4",   IMPLVER_2106x, 0 },
-    { "ev5",   IMPLVER_21164, 0 },
-    { "ev56",  IMPLVER_21164, AMASK_BWX },
-    { "pca56", IMPLVER_21164, AMASK_BWX | AMASK_MVI },
-    { "ev6",   IMPLVER_21264, AMASK_BWX | AMASK_FIX | AMASK_MVI | AMASK_TRAP },
-    { "ev67",  IMPLVER_21264, (AMASK_BWX | AMASK_FIX | AMASK_CIX
-			       | AMASK_MVI | AMASK_TRAP | AMASK_PREFETCH), },
-    { "ev68",  IMPLVER_21264, (AMASK_BWX | AMASK_FIX | AMASK_CIX
-			       | AMASK_MVI | AMASK_TRAP | AMASK_PREFETCH), },
-    { "21064", IMPLVER_2106x, 0 },
-    { "21164", IMPLVER_21164, 0 },
-    { "21164a", IMPLVER_21164, AMASK_BWX },
-    { "21164pc", IMPLVER_21164, AMASK_BWX | AMASK_MVI },
-    { "21264", IMPLVER_21264, AMASK_BWX | AMASK_FIX | AMASK_MVI | AMASK_TRAP },
-    { "21264a", IMPLVER_21264, (AMASK_BWX | AMASK_FIX | AMASK_CIX
-				| AMASK_MVI | AMASK_TRAP | AMASK_PREFETCH), }
-};
-
-CPUAlphaState * cpu_alpha_init (const char *cpu_model)
+CPUAlphaState *cpu_alpha_init(const char *cpu_model)
 {
     AlphaCPU *cpu;
     CPUAlphaState *env;
-    int implver, amask, i, max;
+    const char *typename = cpu_model;
 
-    cpu = ALPHA_CPU(object_new(TYPE_ALPHA_CPU));
+    if (object_class_by_name(typename) == NULL) {
+        /* Default to ev67; no reason not to emulate insns by default.  */
+        typename = "ev67";
+    }
+    cpu = ALPHA_CPU(object_new(typename));
     env = &cpu->env;
 
     alpha_translate_init();
 
-    /* Default to ev67; no reason not to emulate insns by default.  */
-    implver = IMPLVER_21264;
-    amask = (AMASK_BWX | AMASK_FIX | AMASK_CIX | AMASK_MVI
-	     | AMASK_TRAP | AMASK_PREFETCH);
-
-    max = ARRAY_SIZE(cpu_defs);
-    for (i = 0; i < max; i++) {
-        if (strcmp (cpu_model, cpu_defs[i].name) == 0) {
-            implver = cpu_defs[i].implver;
-            amask = cpu_defs[i].amask;
-            break;
-        }
-    }
-    env->implver = implver;
-    env->amask = amask;
     env->cpu_model_str = cpu_model;
 
     qemu_init_vcpu(env);