Re: [Qemu-devel] [PATCH v2 5/7] mips: MIPSCPU model subclasses

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

Hi Eduardo,

On 09/15/2017 09:15 PM, Eduardo Habkost wrote:
> On Wed, Aug 30, 2017 at 07:52:23PM -0300, Philippe Mathieu-Daudé wrote:
>> From: Igor Mammedov <imammedo@redhat.com>
>>
>> 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>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> [PMD: use internal.h, use void* to hold cpu_def in MIPSCPUClass,
>>   mark MIPSCPU abstract]
>> Tested-by: James Hogan <james.hogan@imgtec.com>
>> ---
>>   target/mips/cpu-qom.h        |  1 +
>>   target/mips/internal.h       | 59 ++++++++++++++++++++++++++++++++++++++++++++
>>   target/mips/cpu.c            | 53 ++++++++++++++++++++++++++++++++++++++-
>>   target/mips/translate.c      | 13 +++++-----
>>   target/mips/translate_init.c | 58 ++-----------------------------------------
>>   5 files changed, 120 insertions(+), 64 deletions(-)
>>
>> diff --git a/target/mips/cpu-qom.h b/target/mips/cpu-qom.h
>> index 3f5bf23823..085711d8f9 100644
>> --- a/target/mips/cpu-qom.h
>> +++ b/target/mips/cpu-qom.h
>> @@ -49,6 +49,7 @@ typedef struct MIPSCPUClass {
>>   
>>       DeviceRealize parent_realize;
>>       void (*parent_reset)(CPUState *cpu);
>> +    const void *cpu_def;
> 
> Why void*?  The following should be valid even if you don't
> include internal.h:
> 
>      const struct mips_def_t *cpu_def;

will fix.

> 
> 
>>   } MIPSCPUClass;
>>   
>>   typedef struct MIPSCPU MIPSCPU;
>> diff --git a/target/mips/internal.h b/target/mips/internal.h
>> index cf4c9db427..45ded3484c 100644
>> --- a/target/mips/internal.h
>> +++ b/target/mips/internal.h
>> @@ -7,6 +7,65 @@
>>   #ifndef MIPS_INTERNAL_H
>>   #define MIPS_INTERNAL_H
>>   
>> +
>> +/* 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;
>> +    target_ulong CP0_EBaseWG_rw_bitmask;
>> +    int insn_flags;
>> +    enum mips_mmu_types mmu_type;
>> +};
>> +
>> +extern const struct mips_def_t mips_defs[];
>> +extern const int mips_defs_number;
>> +
>>   enum CPUMIPSMSADataFormat {
>>       DF_BYTE = 0,
>>       DF_HALF,
>> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
>> index e3ef835599..84b6f8bf68 100644
>> --- a/target/mips/cpu.c
>> +++ b/target/mips/cpu.c
>> @@ -146,12 +146,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) {
> 
> Why do we need this conditional?  It looks harmless but
> unnecessary.

ok, will fix.

> 
> The rest of the patch looks good to me, and if the MIPS
> maintainers think the current code is good enough, they have my:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> (Additional questions/comments below)
> 
>> +        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;
>> +    }
> 
> For later: isn't this check for cpu_model==NULL duplicated on
> every single class_by_name implementation?  What about moving it
> to cpu_class_by_name()?

ok, in another patch

> 
>> +
>> +    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)
>> @@ -166,6 +191,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;
>>       cc->has_work = mips_cpu_has_work;
>>       cc->do_interrupt = mips_cpu_do_interrupt;
>>       cc->cpu_exec_interrupt = mips_cpu_exec_interrupt;
>> @@ -193,14 +219,39 @@ static const TypeInfo mips_cpu_type_info = {
>>       .parent = TYPE_CPU,
>>       .instance_size = sizeof(MIPSCPU),
>>       .instance_init = mips_cpu_initfn,
>> -    .abstract = false,
>> +    .abstract = true,
>>       .class_size = sizeof(MIPSCPUClass),
>>       .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 94c38e8755..f7128bc91d 100644
>> --- a/target/mips/translate.c
>> +++ b/target/mips/translate.c
>> @@ -20525,16 +20525,15 @@ void cpu_mips_realize_env(CPUMIPSState *env)
>>   
>>   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 255d25bacd..8bbded46c4 100644
>> --- a/target/mips/translate_init.c
>> +++ b/target/mips/translate_init.c
>> @@ -51,64 +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;
>> -    target_ulong CP0_EBaseWG_rw_bitmask;
>> -    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",
>> @@ -808,6 +753,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)
> 
> What's needed to get rid of cpu_mips_find_by_name() completely?

Next part of this series uses cpu_has_feature() for CPU_ISA and 
CPU_CPS_SMP and this function is removed. This was out of Igor's first 
series scope.

Regards,

Phil.