From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58033) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dtiD3-0001iY-Vq for qemu-devel@nongnu.org; Sun, 17 Sep 2017 18:38:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dtiCy-0004pO-Tn for qemu-devel@nongnu.org; Sun, 17 Sep 2017 18:38:41 -0400 Received: from mail-qk0-x243.google.com ([2607:f8b0:400d:c09::243]:38841) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dtiCy-0004n5-Ny for qemu-devel@nongnu.org; Sun, 17 Sep 2017 18:38:36 -0400 Received: by mail-qk0-x243.google.com with SMTP id c69so4528142qke.5 for ; Sun, 17 Sep 2017 15:38:35 -0700 (PDT) Sender: =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= References: <20170830225225.27925-1-f4bug@amsat.org> <20170830225225.27925-6-f4bug@amsat.org> <20170916001535.GB21016@localhost.localdomain> From: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= Message-ID: <9decfe11-632b-fa17-e60c-7cdf969a43a8@amsat.org> Date: Sun, 17 Sep 2017 19:38:29 -0300 MIME-Version: 1.0 In-Reply-To: <20170916001535.GB21016@localhost.localdomain> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 5/7] mips: MIPSCPU model subclasses List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Igor Mammedov , =?UTF-8?Q?Herv=c3=a9_Poussineau?= , Aurelien Jarno , Marcel Apfelbaum , James Hogan , Yongbok Kim , Thomas Huth , qemu-devel@nongnu.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 >> >> 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 >> Signed-off-by: Philippe Mathieu-Daudé >> [PMD: use internal.h, use void* to hold cpu_def in MIPSCPUClass, >> mark MIPSCPU abstract] >> Tested-by: James Hogan >> --- >> 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 > > (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.