qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 01/14] target-arm: Add QOM subclasses for each A


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH 01/14] target-arm: Add QOM subclasses for each ARM cpu implementation
Date: Tue, 10 Apr 2012 14:33:46 +0100

On 10 April 2012 14:09, Andreas Färber <address@hidden> wrote:
> Am 30.03.2012 14:51, schrieb Peter Maydell:
>> Register subclasses for each ARM CPU implementation (with the
>> exception of "pxa270", which is an alias for "pxa270-a0").
>>
>> Let arm_cpu_list() enumerate CPU subclasses in alphabetical order,
>> except for special value "any".
>>
>> Replace cpu_arm_find_by_name()'s string -> CPUID lookup by storing the
>> CPUID (aka MIDR, Main ID Register) value in the class.
>>
>> Signed-off-by: Andreas Färber <address@hidden>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>>  target-arm/cpu-qom.h |    5 +
>>  target-arm/cpu.c     |  229 
>> +++++++++++++++++++++++++++++++++++++++++++++++++-
>>  target-arm/helper.c  |  114 +++++++++++--------------
>>  3 files changed, 283 insertions(+), 65 deletions(-)
>>
>> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
>> index 42d2a6b..1a3965f 100644
>> --- a/target-arm/cpu-qom.h
>> +++ b/target-arm/cpu-qom.h
>> @@ -58,6 +58,11 @@ typedef struct ARMCPU {
>>      /*< public >*/
>>
>>      CPUARMState env;
>> +
>> +    /* Configuration values (set by the instance init function);
>> +     * some of these might become properties eventually.
>> +     */
>> +    uint32_t midr;
>>  } ARMCPU;
>>
>>  static inline ARMCPU *arm_env_get_cpu(CPUARMState *env)
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index c3ed45b..a09e24e 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -34,6 +34,212 @@ static void arm_cpu_reset(CPUState *s)
>>      cpu_state_reset(&cpu->env);
>>  }
>>
>> +static void arm_cpu_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +
>> +    memset(&cpu->env, 0, sizeof(CPUARMState));
>
> This was actually papering over some bug in an earlier series of mine.
> We can drop this line, it is being memset() before calling the initfn.

OK.

>> +    cpu_exec_init(&cpu->env);
>> +
>> +    cpu->env.cpu_model_str = object_get_typename(obj);
>
> This rules out adding arguments to the cpu_model like you wanted. We
> should better leave that in cpu_arm_init().

OK.

>> +}
>> +
>> +/* CPU models */
>> +
>> +static void arm926_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_ARM926;
>> +}
>> +
>> +static void arm946_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_ARM946;
>> +}
>> +
>> +static void arm1026_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_ARM1026;
>> +}
>> +
>> +static void arm1136_r2_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_ARM1136_R2;
>> +}
>> +
>> +static void arm1136_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_ARM1136;
>> +}
>> +
>> +static void arm1176_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_ARM1176;
>> +}
>> +
>> +static void arm11mpcore_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_ARM11MPCORE;
>> +}
>> +
>> +static void cortex_m3_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_CORTEXM3;
>> +}
>> +
>> +static void cortex_a8_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_CORTEXA8;
>> +}
>> +
>> +static void cortex_a9_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_CORTEXA9;
>> +}
>> +
>> +static void cortex_a15_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_CORTEXA15;
>> +}
>> +
>> +static void ti925t_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_TI925T;
>> +}
>> +
>> +static void sa1100_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_SA1100;
>> +}
>> +
>> +static void sa1110_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_SA1110;
>> +}
>> +
>> +static void pxa250_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_PXA250;
>> +}
>> +
>> +static void pxa255_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_PXA255;
>> +}
>> +
>> +static void pxa260_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_PXA260;
>> +}
>> +
>> +static void pxa261_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_PXA261;
>> +}
>> +
>> +static void pxa262_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_PXA262;
>> +}
>> +
>> +static void pxa270a0_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_PXA270_A0;
>> +}
>> +
>> +static void pxa270a1_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_PXA270_A1;
>> +}
>> +
>> +static void pxa270b0_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_PXA270_B0;
>> +}
>> +
>> +static void pxa270b1_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_PXA270_B1;
>> +}
>> +
>> +static void pxa270c0_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_PXA270_C0;
>> +}
>> +
>> +static void pxa270c5_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_PXA270_C5;
>> +}
>> +
>> +static void arm_any_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +    cpu->midr = ARM_CPUID_ANY;
>> +}
>
> Dislike, I would prefer to copy the values of those defines here and to
> drop as many as possible of those defines.

I kept the defines because at the moment we still have code which
uses them elsewhere. I'd rather not either have (a) some CPUs with
defines and some without or (b) the same magic number in two places.
I agree that in the medium term we should get rid of the defines,
so this is just an argument about ordering I think.

>> +
>> +typedef struct ARMCPUInfo {
>> +    const char *name;
>> +    void (*initfn)(Object *obj);
>> +} ARMCPUInfo;
>> +
>> +static const ARMCPUInfo arm_cpus[] = {
>> +    { .name = "arm926",      .initfn = arm926_initfn },
>> +    { .name = "arm946",      .initfn = arm946_initfn },
>> +    { .name = "arm1026",     .initfn = arm1026_initfn },
>> +    /* What QEMU calls "arm1136-r2" is actually the 1136 r0p2, i.e. an
>> +     * older core than plain "arm1136". In particular this does not
>> +     * have the v6K features.
>> +     */
>> +    { .name = "arm1136-r2",  .initfn = arm1136_r2_initfn },
>
> Please name the function according to what it actually is, no need to
> propagate the error further. :)

I think that would be more confusing, not less confusing.
We're stuck with this naming, so we ought to at least be
consistent internally I think.

>> +    { .name = "arm1136",     .initfn = arm1136_initfn },
>> +    { .name = "arm1176",     .initfn = arm1176_initfn },
>> +    { .name = "arm11mpcore", .initfn = arm11mpcore_initfn },
>> +    { .name = "cortex-m3",   .initfn = cortex_m3_initfn },
>> +    { .name = "cortex-a8",   .initfn = cortex_a8_initfn },
>> +    { .name = "cortex-a9",   .initfn = cortex_a9_initfn },
>> +    { .name = "cortex-a15",  .initfn = cortex_a15_initfn },
>> +    { .name = "ti925t",      .initfn = ti925t_initfn },
>> +    { .name = "sa1100",      .initfn = sa1100_initfn },
>> +    { .name = "sa1110",      .initfn = sa1110_initfn },
>> +    { .name = "pxa250",      .initfn = pxa250_initfn },
>> +    { .name = "pxa255",      .initfn = pxa255_initfn },
>> +    { .name = "pxa260",      .initfn = pxa260_initfn },
>> +    { .name = "pxa261",      .initfn = pxa261_initfn },
>> +    { .name = "pxa262",      .initfn = pxa262_initfn },
>> +    { .name = "pxa270-a0",   .initfn = pxa270a0_initfn },
>> +    { .name = "pxa270-a1",   .initfn = pxa270a1_initfn },
>> +    { .name = "pxa270-b0",   .initfn = pxa270b0_initfn },
>> +    { .name = "pxa270-b1",   .initfn = pxa270b1_initfn },
>> +    { .name = "pxa270-c0",   .initfn = pxa270c0_initfn },
>> +    { .name = "pxa270-c5",   .initfn = pxa270c5_initfn },
>> +    { .name = "any",         .initfn = arm_any_initfn },
>> +};
>
> Like the initfn concept.
>
>> +
>>  static void arm_cpu_class_init(ObjectClass *oc, void *data)
>>  {
>>      ARMCPUClass *acc = ARM_CPU_CLASS(oc);
>> @@ -43,18 +249,37 @@ static void arm_cpu_class_init(ObjectClass *oc, void 
>> *data)
>>      cc->reset = arm_cpu_reset;
>>  }
>>
>> +static void cpu_register(const ARMCPUInfo *info)
>> +{
>> +    TypeInfo type = {
>> +        .name = info->name,
>> +        .parent = TYPE_ARM_CPU,
>> +        .instance_size = sizeof(ARMCPU),
>> +        .instance_init = info->initfn,
>> +        .class_size = sizeof(ARMCPUClass),
>> +        .class_init = arm_cpu_class_init,
>> +    };
>> +
>> +    type_register_static(&type);
>
> Note: I moved to naming it type_info for clarity elsewhere.

Changed.

>> +}
>> +
>>  static const TypeInfo arm_cpu_type_info = {
>>      .name = TYPE_ARM_CPU,
>>      .parent = TYPE_CPU,
>>      .instance_size = sizeof(ARMCPU),
>> -    .abstract = false,
>> +    .instance_init = arm_cpu_initfn,
>> +    .abstract = true,
>>      .class_size = sizeof(ARMCPUClass),
>> -    .class_init = arm_cpu_class_init,
>
> Why are you moving it to the other TypeInfo? If there's no compelling
> reason I'd suggest to leave it here.

Hmm, not sure why that's there; I agree it ought to be this class'
class_init, not the subclass one. Fixed.

>>  };
>>
>>  static void arm_cpu_register_types(void)
>>  {
>> +    int i;
>> +
>>      type_register_static(&arm_cpu_type_info);
>> +    for (i = 0; i < ARRAY_SIZE(arm_cpus); i++) {
>> +        cpu_register(&arm_cpus[i]);
>> +    }
>>  }
>>
>>  type_init(arm_cpu_register_types)
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index d974b57..4748f80 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -6,6 +6,7 @@
>>  #include "hw/loader.h"
>>  #endif
>>  #include "sysemu.h"
>> +#include "cpu-qom.h"
>
> Still needed?

Apparently not. Dropped.

>>  static uint32_t cortexa15_cp15_c0_c1[8] = {
>>      0x00001131, 0x00011011, 0x02010555, 0x00000000,
>> @@ -46,8 +47,6 @@ static uint32_t arm1176_cp15_c0_c1[8] =
>>  static uint32_t arm1176_cp15_c0_c2[8] =
>>  { 0x0140011, 0x12002111, 0x11231121, 0x01102131, 0x01141, 0, 0, 0 };
>>
>> -static uint32_t cpu_arm_find_by_name(const char *name);
>> -
>>  static inline void set_feature(CPUARMState *env, int feature)
>>  {
>>      env->features |= 1u << feature;
>> @@ -55,7 +54,6 @@ static inline void set_feature(CPUARMState *env, int 
>> feature)
>>
>>  static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
>>  {
>> -    env->cp15.c0_cpuid = id;
>>      switch (id) {
>>      case ARM_CPUID_ARM926:
>>          set_feature(env, ARM_FEATURE_V5);
>> @@ -201,7 +199,6 @@ static void cpu_reset_model_id(CPUARMState *env, 
>> uint32_t id)
>>      case ARM_CPUID_TI925T:
>>          set_feature(env, ARM_FEATURE_V4T);
>>          set_feature(env, ARM_FEATURE_OMAPCP);
>> -        env->cp15.c0_cpuid = ARM_CPUID_TI925T; /* Depends on wiring.  */
>>          env->cp15.c0_cachetype = 0x5109149;
>>          env->cp15.c1_sys = 0x00000070;
>>          env->cp15.c15_i_max = 0x000;
>> @@ -287,6 +284,7 @@ void cpu_state_reset(CPUARMState *env)
>>  {
>>      uint32_t id;
>>      uint32_t tmp = 0;
>> +    ARMCPU *cpu = arm_env_get_cpu(env);
>>
>>      if (qemu_loglevel_mask(CPU_LOG_RESET)) {
>>          qemu_log("CPU Reset (CPU %d)\n", env->cpu_index);
>> @@ -299,6 +297,7 @@ void cpu_state_reset(CPUARMState *env)
>>      if (id)
>>          cpu_reset_model_id(env, id);
>>      env->cp15.c15_config_base_address = tmp;
>> +    env->cp15.c0_cpuid = cpu->midr;
>>  #if defined (CONFIG_USER_ONLY)
>>      env->uncached_cpsr = ARM_CPU_MODE_USR;
>>      /* For user mode we must enable access to coprocessors */
>
> In the current stadium, cpu_state_reset() calls cpu_reset(), doesn't it?
> In that case can we move the new ARMCPU-dependent code into the reset
> function?

No, at this point in the series cpu_state_reset() doesn't call cpu_reset(),
it is the other way around (arm_cpu_reset() calls cpu_state_reset()).
The last but one patch in this series moves all the code out of cpu_state_reset
into the ARMCPU class reset function: at that point cpu_state_reset()
changes to call cpu_reset().

>> @@ -405,24 +404,28 @@ static int vfp_gdb_set_reg(CPUARMState *env, uint8_t 
>> *buf, int reg)
>>
>>  CPUARMState *cpu_arm_init(const char *cpu_model)
>>  {
>> +    ObjectClass *klass;
>>      ARMCPU *cpu;
>>      CPUARMState *env;
>> -    uint32_t id;
>>      static int inited = 0;
>>
>> -    id = cpu_arm_find_by_name(cpu_model);
>> -    if (id == 0)
>> +    /* One legacy alias to check */
>> +    if (strcmp(cpu_model, "pxa270") == 0) {
>> +        cpu_model = "pxa270-a0";
>> +    }
>
> (I am to blame for this, yes.) If we handle aliases this way, we won't
> see it in an automated -cpu ? output. If that were desired, we could
> instead have pxa270 be a subclass of the to aliases type.

Or we could just handle the alias by adding another line to the table:
    { .name = "pxa270-a0",   .initfn = pxa270a0_initfn },
    { .name = "pxa270",   .initfn = pxa270a0_initfn }, /* legacy alias */

since the only difference between CPUs is the initfn...

-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]