Re: [Qemu-devel] [PATCH 02/28] mips: MIPSCPU model subclasses

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

Hi Igor,

On 07/15/2017 06:48 PM, Philippe Mathieu-Daudé wrote:
> On 07/14/2017 10:51 AM, Igor Mammedov wrote:
>> Register separate QOM types for each mips cpu model,
>> so it would be possible to reuse generic CPU creation
>> routines.
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
>> ---
>> CC: Aurelien Jarno <aurelien@aurel32.net>
>> CC: Yongbok Kim <yongbok.kim@imgtec.com>
>> ---
>>   target/mips/cpu-qom.h        |  2 ++
>>   target/mips/cpu.h            | 57 
>> +++++++++++++++++++++++++++++++++++++++++++-
>>   target/mips/cpu.c            | 51 
>> +++++++++++++++++++++++++++++++++++++++
>>   target/mips/translate.c      | 13 +++++-----
>>   target/mips/translate_init.c | 57 
>> ++------------------------------------------
>>   5 files changed, 117 insertions(+), 63 deletions(-)
>>
>> diff --git a/target/mips/cpu-qom.h b/target/mips/cpu-qom.h
>> index 3f5bf23..4b32401 100644
>> --- a/target/mips/cpu-qom.h
>> +++ b/target/mips/cpu-qom.h
>> @@ -35,6 +35,7 @@
>>   #define MIPS_CPU_GET_CLASS(obj) \
>>       OBJECT_GET_CLASS(MIPSCPUClass, (obj), TYPE_MIPS_CPU)
>> +typedef struct mips_def_t mips_def_t;
>>   /**
>>    * MIPSCPUClass:
>>    * @parent_realize: The parent class' realize handler.
>> @@ -49,6 +50,7 @@ typedef struct MIPSCPUClass {
>>       DeviceRealize parent_realize;
>>       void (*parent_reset)(CPUState *cpu);
>> +    const mips_def_t *cpu_def;
>>   } MIPSCPUClass;
>>   typedef struct MIPSCPU MIPSCPU;
>> diff --git a/target/mips/cpu.h b/target/mips/cpu.h
>> index 9c32228..7c2e0bf 100644
>> --- a/target/mips/cpu.h
>> +++ b/target/mips/cpu.h
>> @@ -161,7 +161,62 @@ struct CPUMIPSMVPContext {
>>   #define CP0MVPC1_PCP1    0
>>   };
>> -typedef struct mips_def_t mips_def_t;
>> +/* MMU types, the first four entries have the same layout as the
>> +   CP0C0_MT field.  */
>> +enum mips_mmu_types {
>> +    MMU_TYPE_NONE,
>> +    MMU_TYPE_R4000,
>> +    MMU_TYPE_RESERVED,
>> +    MMU_TYPE_FMT,
>> +    MMU_TYPE_R3000,
>> +    MMU_TYPE_R6000,
>> +    MMU_TYPE_R8000
>> +};
>> +
>> +struct mips_def_t {
>> +    const char *name;
>> +    int32_t CP0_PRid;
>> +    int32_t CP0_Config0;
>> +    int32_t CP0_Config1;
>> +    int32_t CP0_Config2;
>> +    int32_t CP0_Config3;
>> +    int32_t CP0_Config4;
>> +    int32_t CP0_Config4_rw_bitmask;
>> +    int32_t CP0_Config5;
>> +    int32_t CP0_Config5_rw_bitmask;
>> +    int32_t CP0_Config6;
>> +    int32_t CP0_Config7;
>> +    target_ulong CP0_LLAddr_rw_bitmask;
>> +    int CP0_LLAddr_shift;
>> +    int32_t SYNCI_Step;
>> +    int32_t CCRes;
>> +    int32_t CP0_Status_rw_bitmask;
>> +    int32_t CP0_TCStatus_rw_bitmask;
>> +    int32_t CP0_SRSCtl;
>> +    int32_t CP1_fcr0;
>> +    int32_t CP1_fcr31_rw_bitmask;
>> +    int32_t CP1_fcr31;
>> +    int32_t MSAIR;
>> +    int32_t SEGBITS;
>> +    int32_t PABITS;
>> +    int32_t CP0_SRSConf0_rw_bitmask;
>> +    int32_t CP0_SRSConf0;
>> +    int32_t CP0_SRSConf1_rw_bitmask;
>> +    int32_t CP0_SRSConf1;
>> +    int32_t CP0_SRSConf2_rw_bitmask;
>> +    int32_t CP0_SRSConf2;
>> +    int32_t CP0_SRSConf3_rw_bitmask;
>> +    int32_t CP0_SRSConf3;
>> +    int32_t CP0_SRSConf4_rw_bitmask;
>> +    int32_t CP0_SRSConf4;
>> +    int32_t CP0_PageGrain_rw_bitmask;
>> +    int32_t CP0_PageGrain;
>> +    int insn_flags;
>> +    enum mips_mmu_types mmu_type;
>> +};
>> +
>> +extern const struct mips_def_t mips_defs[];
>> +extern const int mips_defs_number;
>>   #define MIPS_SHADOW_SET_MAX 16
>>   #define MIPS_TC_MAX 5
>> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
>> index 82afdaa..111b5ae 100644
>> --- a/target/mips/cpu.c
>> +++ b/target/mips/cpu.c
>> @@ -151,12 +151,37 @@ static void mips_cpu_initfn(Object *obj)
>>       CPUState *cs = CPU(obj);
>>       MIPSCPU *cpu = MIPS_CPU(obj);
>>       CPUMIPSState *env = &cpu->env;
>> +    MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(obj);
>>       cs->env_ptr = env;
>>       if (tcg_enabled()) {
>>           mips_tcg_init();
>>       }
>> +
>> +    if (mcc->cpu_def) {
>> +        env->cpu_model = mcc->cpu_def;
>> +    }
>> +}
>> +
>> +static char *mips_cpu_type_name(const char *cpu_model)
>> +{
>> +    return g_strdup_printf("%s-" TYPE_MIPS_CPU, cpu_model);
>> +}
>> +
>> +static ObjectClass *mips_cpu_class_by_name(const char *cpu_model)
>> +{
>> +    ObjectClass *oc;
>> +    char *typename;
>> +
>> +    if (cpu_model == NULL) {
>> +        return NULL;
>> +    }
>> +
>> +    typename = mips_cpu_type_name(cpu_model);
>> +    oc = object_class_by_name(typename);
>> +    g_free(typename);
>> +    return oc;
>>   }
>>   static void mips_cpu_class_init(ObjectClass *c, void *data)
>> @@ -171,6 +196,7 @@ static void mips_cpu_class_init(ObjectClass *c, 
>> void *data)
>>       mcc->parent_reset = cc->reset;
>>       cc->reset = mips_cpu_reset;
>> +    cc->class_by_name = mips_cpu_class_by_name;

Now than I'm reading again...

>>       cc->has_work = mips_cpu_has_work;
>>       cc->do_interrupt = mips_cpu_do_interrupt;
>>       cc->cpu_exec_interrupt = mips_cpu_exec_interrupt;
>> @@ -203,9 +229,34 @@ static const TypeInfo mips_cpu_type_info = {

Shouldn't this class now be abstract?

>>       .class_init = mips_cpu_class_init,
>>   };
>> +static void mips_cpu_cpudef_class_init(ObjectClass *oc, void *data)
>> +{
>> +    MIPSCPUClass *mcc = MIPS_CPU_CLASS(oc);
>> +    mcc->cpu_def = data;
>> +}
>> +
>> +static void mips_register_cpudef_type(const struct mips_def_t *def)
>> +{
>> +    char *typename = mips_cpu_type_name(def->name);
>> +    TypeInfo ti = {
>> +        .name = typename,
>> +        .parent = TYPE_MIPS_CPU,
>> +        .class_init = mips_cpu_cpudef_class_init,
>> +        .class_data = (void *)def,
>> +    };
>> +
>> +    type_register(&ti);
>> +    g_free(typename);
>> +}
>> +
>>   static void mips_cpu_register_types(void)
>>   {
>> +    int i;
>> +
>>       type_register_static(&mips_cpu_type_info);
>> +    for (i = 0; i < mips_defs_number; i++) {
>> +        mips_register_cpudef_type(&mips_defs[i]);
>> +    }
>>   }
>>   type_init(mips_cpu_register_types)
>> diff --git a/target/mips/translate.c b/target/mips/translate.c
>> index 7b3ae81..ae7ca80 100644
>> --- a/target/mips/translate.c
>> +++ b/target/mips/translate.c
>> @@ -20193,16 +20193,15 @@ void mips_tcg_init(void)
>>   MIPSCPU *cpu_mips_init(const char *cpu_model)
>>   {
>> +    ObjectClass *oc;
>>       MIPSCPU *cpu;
>> -    CPUMIPSState *env;
>> -    const mips_def_t *def;
>> -    def = cpu_mips_find_by_name(cpu_model);
>> -    if (!def)
>> +    oc = cpu_class_by_name(TYPE_MIPS_CPU, cpu_model);
>> +    if (oc == NULL) {
>>           return NULL;
>> -    cpu = MIPS_CPU(object_new(TYPE_MIPS_CPU));
>> -    env = &cpu->env;
>> -    env->cpu_model = def;
>> +    }
>> +
>> +    cpu = MIPS_CPU(object_new(object_class_get_name(oc)));
>>       object_property_set_bool(OBJECT(cpu), true, "realized", NULL);
>> diff --git a/target/mips/translate_init.c b/target/mips/translate_init.c
>> index c771ff1..16c214b 100644
>> --- a/target/mips/translate_init.c
>> +++ b/target/mips/translate_init.c
>> @@ -51,63 +51,9 @@
>>   #define MIPS_CONFIG5                                              \
>>   ((0 << CP0C5_M))
>> -/* MMU types, the first four entries have the same layout as the
>> -   CP0C0_MT field.  */
>> -enum mips_mmu_types {
>> -    MMU_TYPE_NONE,
>> -    MMU_TYPE_R4000,
>> -    MMU_TYPE_RESERVED,
>> -    MMU_TYPE_FMT,
>> -    MMU_TYPE_R3000,
>> -    MMU_TYPE_R6000,
>> -    MMU_TYPE_R8000
>> -};
>> -
>> -struct mips_def_t {
>> -    const char *name;
>> -    int32_t CP0_PRid;
>> -    int32_t CP0_Config0;
>> -    int32_t CP0_Config1;
>> -    int32_t CP0_Config2;
>> -    int32_t CP0_Config3;
>> -    int32_t CP0_Config4;
>> -    int32_t CP0_Config4_rw_bitmask;
>> -    int32_t CP0_Config5;
>> -    int32_t CP0_Config5_rw_bitmask;
>> -    int32_t CP0_Config6;
>> -    int32_t CP0_Config7;
>> -    target_ulong CP0_LLAddr_rw_bitmask;
>> -    int CP0_LLAddr_shift;
>> -    int32_t SYNCI_Step;
>> -    int32_t CCRes;
>> -    int32_t CP0_Status_rw_bitmask;
>> -    int32_t CP0_TCStatus_rw_bitmask;
>> -    int32_t CP0_SRSCtl;
>> -    int32_t CP1_fcr0;
>> -    int32_t CP1_fcr31_rw_bitmask;
>> -    int32_t CP1_fcr31;
>> -    int32_t MSAIR;
>> -    int32_t SEGBITS;
>> -    int32_t PABITS;
>> -    int32_t CP0_SRSConf0_rw_bitmask;
>> -    int32_t CP0_SRSConf0;
>> -    int32_t CP0_SRSConf1_rw_bitmask;
>> -    int32_t CP0_SRSConf1;
>> -    int32_t CP0_SRSConf2_rw_bitmask;
>> -    int32_t CP0_SRSConf2;
>> -    int32_t CP0_SRSConf3_rw_bitmask;
>> -    int32_t CP0_SRSConf3;
>> -    int32_t CP0_SRSConf4_rw_bitmask;
>> -    int32_t CP0_SRSConf4;
>> -    int32_t CP0_PageGrain_rw_bitmask;
>> -    int32_t CP0_PageGrain;
>> -    int insn_flags;
>> -    enum mips_mmu_types mmu_type;
>> -};
>> -
>>   
>> /*****************************************************************************/ 
>>
>>   /* MIPS CPU definitions */
>> -static const mips_def_t mips_defs[] =
>> +const mips_def_t mips_defs[] =
>>   {
>>       {
>>           .name = "4Kc",
>> @@ -803,6 +749,7 @@ static const mips_def_t mips_defs[] =
>>   #endif
>>   };
>> +const int mips_defs_number = ARRAY_SIZE(mips_defs);
>>   static const mips_def_t *cpu_mips_find_by_name (const char *name)
>>   {
>>