[{"id":1769519,"web_url":"http://patchwork.ozlabs.org/comment/1769519/","msgid":"<20170916001535.GB21016@localhost.localdomain>","list_archive_url":null,"date":"2017-09-16T00:15:35","subject":"Re: [Qemu-devel] [PATCH v2 5/7] mips: MIPSCPU model subclasses","submitter":{"id":195,"url":"http://patchwork.ozlabs.org/api/people/195/","name":"Eduardo Habkost","email":"ehabkost@redhat.com"},"content":"On Wed, Aug 30, 2017 at 07:52:23PM -0300, Philippe Mathieu-Daudé wrote:\n> From: Igor Mammedov \n> \n> Register separate QOM types for each mips cpu model,\n> so it would be possible to reuse generic CPU creation\n> routines.\n> \n> Signed-off-by: Igor Mammedov \n> Signed-off-by: Philippe Mathieu-Daudé \n> [PMD: use internal.h, use void* to hold cpu_def in MIPSCPUClass,\n> mark MIPSCPU abstract]\n> Tested-by: James Hogan \n> ---\n> target/mips/cpu-qom.h | 1 +\n> target/mips/internal.h | 59 ++++++++++++++++++++++++++++++++++++++++++++\n> target/mips/cpu.c | 53 ++++++++++++++++++++++++++++++++++++++-\n> target/mips/translate.c | 13 +++++-----\n> target/mips/translate_init.c | 58 ++-----------------------------------------\n> 5 files changed, 120 insertions(+), 64 deletions(-)\n> \n> diff --git a/target/mips/cpu-qom.h b/target/mips/cpu-qom.h\n> index 3f5bf23823..085711d8f9 100644\n> --- a/target/mips/cpu-qom.h\n> +++ b/target/mips/cpu-qom.h\n> @@ -49,6 +49,7 @@ typedef struct MIPSCPUClass {\n> \n> DeviceRealize parent_realize;\n> void (*parent_reset)(CPUState *cpu);\n> + const void *cpu_def;\n\nWhy void*? The following should be valid even if you don't\ninclude internal.h:\n\n const struct mips_def_t *cpu_def;\n\n\n> } MIPSCPUClass;\n> \n> typedef struct MIPSCPU MIPSCPU;\n> diff --git a/target/mips/internal.h b/target/mips/internal.h\n> index cf4c9db427..45ded3484c 100644\n> --- a/target/mips/internal.h\n> +++ b/target/mips/internal.h\n> @@ -7,6 +7,65 @@\n> #ifndef MIPS_INTERNAL_H\n> #define MIPS_INTERNAL_H\n> \n> +\n> +/* MMU types, the first four entries have the same layout as the\n> + CP0C0_MT field. */\n> +enum mips_mmu_types {\n> + MMU_TYPE_NONE,\n> + MMU_TYPE_R4000,\n> + MMU_TYPE_RESERVED,\n> + MMU_TYPE_FMT,\n> + MMU_TYPE_R3000,\n> + MMU_TYPE_R6000,\n> + MMU_TYPE_R8000\n> +};\n> +\n> +struct mips_def_t {\n> + const char *name;\n> + int32_t CP0_PRid;\n> + int32_t CP0_Config0;\n> + int32_t CP0_Config1;\n> + int32_t CP0_Config2;\n> + int32_t CP0_Config3;\n> + int32_t CP0_Config4;\n> + int32_t CP0_Config4_rw_bitmask;\n> + int32_t CP0_Config5;\n> + int32_t CP0_Config5_rw_bitmask;\n> + int32_t CP0_Config6;\n> + int32_t CP0_Config7;\n> + target_ulong CP0_LLAddr_rw_bitmask;\n> + int CP0_LLAddr_shift;\n> + int32_t SYNCI_Step;\n> + int32_t CCRes;\n> + int32_t CP0_Status_rw_bitmask;\n> + int32_t CP0_TCStatus_rw_bitmask;\n> + int32_t CP0_SRSCtl;\n> + int32_t CP1_fcr0;\n> + int32_t CP1_fcr31_rw_bitmask;\n> + int32_t CP1_fcr31;\n> + int32_t MSAIR;\n> + int32_t SEGBITS;\n> + int32_t PABITS;\n> + int32_t CP0_SRSConf0_rw_bitmask;\n> + int32_t CP0_SRSConf0;\n> + int32_t CP0_SRSConf1_rw_bitmask;\n> + int32_t CP0_SRSConf1;\n> + int32_t CP0_SRSConf2_rw_bitmask;\n> + int32_t CP0_SRSConf2;\n> + int32_t CP0_SRSConf3_rw_bitmask;\n> + int32_t CP0_SRSConf3;\n> + int32_t CP0_SRSConf4_rw_bitmask;\n> + int32_t CP0_SRSConf4;\n> + int32_t CP0_PageGrain_rw_bitmask;\n> + int32_t CP0_PageGrain;\n> + target_ulong CP0_EBaseWG_rw_bitmask;\n> + int insn_flags;\n> + enum mips_mmu_types mmu_type;\n> +};\n> +\n> +extern const struct mips_def_t mips_defs[];\n> +extern const int mips_defs_number;\n> +\n> enum CPUMIPSMSADataFormat {\n> DF_BYTE = 0,\n> DF_HALF,\n> diff --git a/target/mips/cpu.c b/target/mips/cpu.c\n> index e3ef835599..84b6f8bf68 100644\n> --- a/target/mips/cpu.c\n> +++ b/target/mips/cpu.c\n> @@ -146,12 +146,37 @@ static void mips_cpu_initfn(Object *obj)\n> CPUState *cs = CPU(obj);\n> MIPSCPU *cpu = MIPS_CPU(obj);\n> CPUMIPSState *env = &cpu->env;\n> + MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(obj);\n> \n> cs->env_ptr = env;\n> \n> if (tcg_enabled()) {\n> mips_tcg_init();\n> }\n> +\n> + if (mcc->cpu_def) {\n\nWhy do we need this conditional? It looks harmless but\nunnecessary.\n\nThe rest of the patch looks good to me, and if the MIPS\nmaintainers think the current code is good enough, they have my:\n\nReviewed-by: Eduardo Habkost \n\n(Additional questions/comments below)\n\n> + env->cpu_model = mcc->cpu_def;\n> + }\n> +}\n> +\n> +static char *mips_cpu_type_name(const char *cpu_model)\n> +{\n> + return g_strdup_printf(\"%s-\" TYPE_MIPS_CPU, cpu_model);\n> +}\n> +\n> +static ObjectClass *mips_cpu_class_by_name(const char *cpu_model)\n> +{\n> + ObjectClass *oc;\n> + char *typename;\n> +\n> + if (cpu_model == NULL) {\n> + return NULL;\n> + }\n\nFor later: isn't this check for cpu_model==NULL duplicated on\nevery single class_by_name implementation? What about moving it\nto cpu_class_by_name()?\n\n> +\n> + typename = mips_cpu_type_name(cpu_model);\n> + oc = object_class_by_name(typename);\n> + g_free(typename);\n> + return oc;\n> }\n> \n> static void mips_cpu_class_init(ObjectClass *c, void *data)\n> @@ -166,6 +191,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data)\n> mcc->parent_reset = cc->reset;\n> cc->reset = mips_cpu_reset;\n> \n> + cc->class_by_name = mips_cpu_class_by_name;\n> cc->has_work = mips_cpu_has_work;\n> cc->do_interrupt = mips_cpu_do_interrupt;\n> cc->cpu_exec_interrupt = mips_cpu_exec_interrupt;\n> @@ -193,14 +219,39 @@ static const TypeInfo mips_cpu_type_info = {\n> .parent = TYPE_CPU,\n> .instance_size = sizeof(MIPSCPU),\n> .instance_init = mips_cpu_initfn,\n> - .abstract = false,\n> + .abstract = true,\n> .class_size = sizeof(MIPSCPUClass),\n> .class_init = mips_cpu_class_init,\n> };\n> \n> +static void mips_cpu_cpudef_class_init(ObjectClass *oc, void *data)\n> +{\n> + MIPSCPUClass *mcc = MIPS_CPU_CLASS(oc);\n> + mcc->cpu_def = data;\n> +}\n> +\n> +static void mips_register_cpudef_type(const struct mips_def_t *def)\n> +{\n> + char *typename = mips_cpu_type_name(def->name);\n> + TypeInfo ti = {\n> + .name = typename,\n> + .parent = TYPE_MIPS_CPU,\n> + .class_init = mips_cpu_cpudef_class_init,\n> + .class_data = (void *)def,\n> + };\n> +\n> + type_register(&ti);\n> + g_free(typename);\n> +}\n> +\n> static void mips_cpu_register_types(void)\n> {\n> + int i;\n> +\n> type_register_static(&mips_cpu_type_info);\n> + for (i = 0; i < mips_defs_number; i++) {\n> + mips_register_cpudef_type(&mips_defs[i]);\n> + }\n> }\n> \n> type_init(mips_cpu_register_types)\n> diff --git a/target/mips/translate.c b/target/mips/translate.c\n> index 94c38e8755..f7128bc91d 100644\n> --- a/target/mips/translate.c\n> +++ b/target/mips/translate.c\n> @@ -20525,16 +20525,15 @@ void cpu_mips_realize_env(CPUMIPSState *env)\n> \n> MIPSCPU *cpu_mips_init(const char *cpu_model)\n> {\n> + ObjectClass *oc;\n> MIPSCPU *cpu;\n> - CPUMIPSState *env;\n> - const mips_def_t *def;\n> \n> - def = cpu_mips_find_by_name(cpu_model);\n> - if (!def)\n> + oc = cpu_class_by_name(TYPE_MIPS_CPU, cpu_model);\n> + if (oc == NULL) {\n> return NULL;\n> - cpu = MIPS_CPU(object_new(TYPE_MIPS_CPU));\n> - env = &cpu->env;\n> - env->cpu_model = def;\n> + }\n> +\n> + cpu = MIPS_CPU(object_new(object_class_get_name(oc)));\n> \n> object_property_set_bool(OBJECT(cpu), true, \"realized\", NULL);\n> \n> diff --git a/target/mips/translate_init.c b/target/mips/translate_init.c\n> index 255d25bacd..8bbded46c4 100644\n> --- a/target/mips/translate_init.c\n> +++ b/target/mips/translate_init.c\n> @@ -51,64 +51,9 @@\n> #define MIPS_CONFIG5 \\\n> ((0 << CP0C5_M))\n> \n> -/* MMU types, the first four entries have the same layout as the\n> - CP0C0_MT field. */\n> -enum mips_mmu_types {\n> - MMU_TYPE_NONE,\n> - MMU_TYPE_R4000,\n> - MMU_TYPE_RESERVED,\n> - MMU_TYPE_FMT,\n> - MMU_TYPE_R3000,\n> - MMU_TYPE_R6000,\n> - MMU_TYPE_R8000\n> -};\n> -\n> -struct mips_def_t {\n> - const char *name;\n> - int32_t CP0_PRid;\n> - int32_t CP0_Config0;\n> - int32_t CP0_Config1;\n> - int32_t CP0_Config2;\n> - int32_t CP0_Config3;\n> - int32_t CP0_Config4;\n> - int32_t CP0_Config4_rw_bitmask;\n> - int32_t CP0_Config5;\n> - int32_t CP0_Config5_rw_bitmask;\n> - int32_t CP0_Config6;\n> - int32_t CP0_Config7;\n> - target_ulong CP0_LLAddr_rw_bitmask;\n> - int CP0_LLAddr_shift;\n> - int32_t SYNCI_Step;\n> - int32_t CCRes;\n> - int32_t CP0_Status_rw_bitmask;\n> - int32_t CP0_TCStatus_rw_bitmask;\n> - int32_t CP0_SRSCtl;\n> - int32_t CP1_fcr0;\n> - int32_t CP1_fcr31_rw_bitmask;\n> - int32_t CP1_fcr31;\n> - int32_t MSAIR;\n> - int32_t SEGBITS;\n> - int32_t PABITS;\n> - int32_t CP0_SRSConf0_rw_bitmask;\n> - int32_t CP0_SRSConf0;\n> - int32_t CP0_SRSConf1_rw_bitmask;\n> - int32_t CP0_SRSConf1;\n> - int32_t CP0_SRSConf2_rw_bitmask;\n> - int32_t CP0_SRSConf2;\n> - int32_t CP0_SRSConf3_rw_bitmask;\n> - int32_t CP0_SRSConf3;\n> - int32_t CP0_SRSConf4_rw_bitmask;\n> - int32_t CP0_SRSConf4;\n> - int32_t CP0_PageGrain_rw_bitmask;\n> - int32_t CP0_PageGrain;\n> - target_ulong CP0_EBaseWG_rw_bitmask;\n> - int insn_flags;\n> - enum mips_mmu_types mmu_type;\n> -};\n> -\n> /*****************************************************************************/\n> /* MIPS CPU definitions */\n> -static const mips_def_t mips_defs[] =\n> +const mips_def_t mips_defs[] =\n> {\n> {\n> .name = \"4Kc\",\n> @@ -808,6 +753,7 @@ static const mips_def_t mips_defs[] =\n> \n> #endif\n> };\n> +const int mips_defs_number = ARRAY_SIZE(mips_defs);\n> \n> static const mips_def_t *cpu_mips_find_by_name (const char *name)\n\nWhat's needed to get rid of cpu_mips_find_by_name() completely?","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=ehabkost@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xvCVP5BQsz9sPr\n\tfor ;\n\tSat, 16 Sep 2017 10:16:10 +1000 (AEST)","from localhost ([::1]:55453 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dt0mF-0002wi-56\n\tfor incoming@patchwork.ozlabs.org; Fri, 15 Sep 2017 20:16:07 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:38150)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dt0lv-0002wC-6N\n\tfor qemu-devel@nongnu.org; Fri, 15 Sep 2017 20:15:48 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dt0lq-00056i-U5\n\tfor qemu-devel@nongnu.org; Fri, 15 Sep 2017 20:15:47 -0400","from mx1.redhat.com ([209.132.183.28]:44978)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dt0lp-00055y-Up\n\tfor qemu-devel@nongnu.org; Fri, 15 Sep 2017 20:15:42 -0400","from smtp.corp.redhat.com\n\t(int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 6ABC9C047B86;\n\tSat, 16 Sep 2017 00:15:40 +0000 (UTC)","from localhost (ovpn-116-24.gru2.redhat.com [10.97.116.24])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id B1F5839BD;\n\tSat, 16 Sep 2017 00:15:37 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 6ABC9C047B86","Date":"Fri, 15 Sep 2017 21:15:35 -0300","From":"Eduardo Habkost ","To":"Philippe =?iso-8859-1?q?Mathieu-Daud=E9?= ","Message-ID":"<20170916001535.GB21016@localhost.localdomain>","References":"<20170830225225.27925-1-f4bug@amsat.org>\n\t<20170830225225.27925-6-f4bug@amsat.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","In-Reply-To":"<20170830225225.27925-6-f4bug@amsat.org>","X-Fnord":"you can see the fnord","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.15","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.31]);\n\tSat, 16 Sep 2017 00:15:40 +0000 (UTC)","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH v2 5/7] mips: MIPSCPU model subclasses","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Thomas Huth , James Hogan ,\n\tqemu-devel@nongnu.org, Yongbok Kim , \n\t=?iso-8859-1?q?Herv=E9?= Poussineau ,\n\tMarcel Apfelbaum ,\n\tIgor Mammedov , Aurelien Jarno\n\t","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1769813,"web_url":"http://patchwork.ozlabs.org/comment/1769813/","msgid":"<9decfe11-632b-fa17-e60c-7cdf969a43a8@amsat.org>","list_archive_url":null,"date":"2017-09-17T22:38:29","subject":"Re: [Qemu-devel] [PATCH v2 5/7] mips: MIPSCPU model subclasses","submitter":{"id":70924,"url":"http://patchwork.ozlabs.org/api/people/70924/","name":"Philippe Mathieu-Daudé","email":"f4bug@amsat.org"},"content":"Hi Eduardo,\n\nOn 09/15/2017 09:15 PM, Eduardo Habkost wrote:\n> On Wed, Aug 30, 2017 at 07:52:23PM -0300, Philippe Mathieu-Daudé wrote:\n>> From: Igor Mammedov \n>>\n>> Register separate QOM types for each mips cpu model,\n>> so it would be possible to reuse generic CPU creation\n>> routines.\n>>\n>> Signed-off-by: Igor Mammedov \n>> Signed-off-by: Philippe Mathieu-Daudé \n>> [PMD: use internal.h, use void* to hold cpu_def in MIPSCPUClass,\n>> mark MIPSCPU abstract]\n>> Tested-by: James Hogan \n>> ---\n>> target/mips/cpu-qom.h | 1 +\n>> target/mips/internal.h | 59 ++++++++++++++++++++++++++++++++++++++++++++\n>> target/mips/cpu.c | 53 ++++++++++++++++++++++++++++++++++++++-\n>> target/mips/translate.c | 13 +++++-----\n>> target/mips/translate_init.c | 58 ++-----------------------------------------\n>> 5 files changed, 120 insertions(+), 64 deletions(-)\n>>\n>> diff --git a/target/mips/cpu-qom.h b/target/mips/cpu-qom.h\n>> index 3f5bf23823..085711d8f9 100644\n>> --- a/target/mips/cpu-qom.h\n>> +++ b/target/mips/cpu-qom.h\n>> @@ -49,6 +49,7 @@ typedef struct MIPSCPUClass {\n>> \n>> DeviceRealize parent_realize;\n>> void (*parent_reset)(CPUState *cpu);\n>> + const void *cpu_def;\n> \n> Why void*? The following should be valid even if you don't\n> include internal.h:\n> \n> const struct mips_def_t *cpu_def;\n\nwill fix.\n\n> \n> \n>> } MIPSCPUClass;\n>> \n>> typedef struct MIPSCPU MIPSCPU;\n>> diff --git a/target/mips/internal.h b/target/mips/internal.h\n>> index cf4c9db427..45ded3484c 100644\n>> --- a/target/mips/internal.h\n>> +++ b/target/mips/internal.h\n>> @@ -7,6 +7,65 @@\n>> #ifndef MIPS_INTERNAL_H\n>> #define MIPS_INTERNAL_H\n>> \n>> +\n>> +/* MMU types, the first four entries have the same layout as the\n>> + CP0C0_MT field. */\n>> +enum mips_mmu_types {\n>> + MMU_TYPE_NONE,\n>> + MMU_TYPE_R4000,\n>> + MMU_TYPE_RESERVED,\n>> + MMU_TYPE_FMT,\n>> + MMU_TYPE_R3000,\n>> + MMU_TYPE_R6000,\n>> + MMU_TYPE_R8000\n>> +};\n>> +\n>> +struct mips_def_t {\n>> + const char *name;\n>> + int32_t CP0_PRid;\n>> + int32_t CP0_Config0;\n>> + int32_t CP0_Config1;\n>> + int32_t CP0_Config2;\n>> + int32_t CP0_Config3;\n>> + int32_t CP0_Config4;\n>> + int32_t CP0_Config4_rw_bitmask;\n>> + int32_t CP0_Config5;\n>> + int32_t CP0_Config5_rw_bitmask;\n>> + int32_t CP0_Config6;\n>> + int32_t CP0_Config7;\n>> + target_ulong CP0_LLAddr_rw_bitmask;\n>> + int CP0_LLAddr_shift;\n>> + int32_t SYNCI_Step;\n>> + int32_t CCRes;\n>> + int32_t CP0_Status_rw_bitmask;\n>> + int32_t CP0_TCStatus_rw_bitmask;\n>> + int32_t CP0_SRSCtl;\n>> + int32_t CP1_fcr0;\n>> + int32_t CP1_fcr31_rw_bitmask;\n>> + int32_t CP1_fcr31;\n>> + int32_t MSAIR;\n>> + int32_t SEGBITS;\n>> + int32_t PABITS;\n>> + int32_t CP0_SRSConf0_rw_bitmask;\n>> + int32_t CP0_SRSConf0;\n>> + int32_t CP0_SRSConf1_rw_bitmask;\n>> + int32_t CP0_SRSConf1;\n>> + int32_t CP0_SRSConf2_rw_bitmask;\n>> + int32_t CP0_SRSConf2;\n>> + int32_t CP0_SRSConf3_rw_bitmask;\n>> + int32_t CP0_SRSConf3;\n>> + int32_t CP0_SRSConf4_rw_bitmask;\n>> + int32_t CP0_SRSConf4;\n>> + int32_t CP0_PageGrain_rw_bitmask;\n>> + int32_t CP0_PageGrain;\n>> + target_ulong CP0_EBaseWG_rw_bitmask;\n>> + int insn_flags;\n>> + enum mips_mmu_types mmu_type;\n>> +};\n>> +\n>> +extern const struct mips_def_t mips_defs[];\n>> +extern const int mips_defs_number;\n>> +\n>> enum CPUMIPSMSADataFormat {\n>> DF_BYTE = 0,\n>> DF_HALF,\n>> diff --git a/target/mips/cpu.c b/target/mips/cpu.c\n>> index e3ef835599..84b6f8bf68 100644\n>> --- a/target/mips/cpu.c\n>> +++ b/target/mips/cpu.c\n>> @@ -146,12 +146,37 @@ static void mips_cpu_initfn(Object *obj)\n>> CPUState *cs = CPU(obj);\n>> MIPSCPU *cpu = MIPS_CPU(obj);\n>> CPUMIPSState *env = &cpu->env;\n>> + MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(obj);\n>> \n>> cs->env_ptr = env;\n>> \n>> if (tcg_enabled()) {\n>> mips_tcg_init();\n>> }\n>> +\n>> + if (mcc->cpu_def) {\n> \n> Why do we need this conditional? It looks harmless but\n> unnecessary.\n\nok, will fix.\n\n> \n> The rest of the patch looks good to me, and if the MIPS\n> maintainers think the current code is good enough, they have my:\n> \n> Reviewed-by: Eduardo Habkost \n> \n> (Additional questions/comments below)\n> \n>> + env->cpu_model = mcc->cpu_def;\n>> + }\n>> +}\n>> +\n>> +static char *mips_cpu_type_name(const char *cpu_model)\n>> +{\n>> + return g_strdup_printf(\"%s-\" TYPE_MIPS_CPU, cpu_model);\n>> +}\n>> +\n>> +static ObjectClass *mips_cpu_class_by_name(const char *cpu_model)\n>> +{\n>> + ObjectClass *oc;\n>> + char *typename;\n>> +\n>> + if (cpu_model == NULL) {\n>> + return NULL;\n>> + }\n> \n> For later: isn't this check for cpu_model==NULL duplicated on\n> every single class_by_name implementation? What about moving it\n> to cpu_class_by_name()?\n\nok, in another patch\n\n> \n>> +\n>> + typename = mips_cpu_type_name(cpu_model);\n>> + oc = object_class_by_name(typename);\n>> + g_free(typename);\n>> + return oc;\n>> }\n>> \n>> static void mips_cpu_class_init(ObjectClass *c, void *data)\n>> @@ -166,6 +191,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data)\n>> mcc->parent_reset = cc->reset;\n>> cc->reset = mips_cpu_reset;\n>> \n>> + cc->class_by_name = mips_cpu_class_by_name;\n>> cc->has_work = mips_cpu_has_work;\n>> cc->do_interrupt = mips_cpu_do_interrupt;\n>> cc->cpu_exec_interrupt = mips_cpu_exec_interrupt;\n>> @@ -193,14 +219,39 @@ static const TypeInfo mips_cpu_type_info = {\n>> .parent = TYPE_CPU,\n>> .instance_size = sizeof(MIPSCPU),\n>> .instance_init = mips_cpu_initfn,\n>> - .abstract = false,\n>> + .abstract = true,\n>> .class_size = sizeof(MIPSCPUClass),\n>> .class_init = mips_cpu_class_init,\n>> };\n>> \n>> +static void mips_cpu_cpudef_class_init(ObjectClass *oc, void *data)\n>> +{\n>> + MIPSCPUClass *mcc = MIPS_CPU_CLASS(oc);\n>> + mcc->cpu_def = data;\n>> +}\n>> +\n>> +static void mips_register_cpudef_type(const struct mips_def_t *def)\n>> +{\n>> + char *typename = mips_cpu_type_name(def->name);\n>> + TypeInfo ti = {\n>> + .name = typename,\n>> + .parent = TYPE_MIPS_CPU,\n>> + .class_init = mips_cpu_cpudef_class_init,\n>> + .class_data = (void *)def,\n>> + };\n>> +\n>> + type_register(&ti);\n>> + g_free(typename);\n>> +}\n>> +\n>> static void mips_cpu_register_types(void)\n>> {\n>> + int i;\n>> +\n>> type_register_static(&mips_cpu_type_info);\n>> + for (i = 0; i < mips_defs_number; i++) {\n>> + mips_register_cpudef_type(&mips_defs[i]);\n>> + }\n>> }\n>> \n>> type_init(mips_cpu_register_types)\n>> diff --git a/target/mips/translate.c b/target/mips/translate.c\n>> index 94c38e8755..f7128bc91d 100644\n>> --- a/target/mips/translate.c\n>> +++ b/target/mips/translate.c\n>> @@ -20525,16 +20525,15 @@ void cpu_mips_realize_env(CPUMIPSState *env)\n>> \n>> MIPSCPU *cpu_mips_init(const char *cpu_model)\n>> {\n>> + ObjectClass *oc;\n>> MIPSCPU *cpu;\n>> - CPUMIPSState *env;\n>> - const mips_def_t *def;\n>> \n>> - def = cpu_mips_find_by_name(cpu_model);\n>> - if (!def)\n>> + oc = cpu_class_by_name(TYPE_MIPS_CPU, cpu_model);\n>> + if (oc == NULL) {\n>> return NULL;\n>> - cpu = MIPS_CPU(object_new(TYPE_MIPS_CPU));\n>> - env = &cpu->env;\n>> - env->cpu_model = def;\n>> + }\n>> +\n>> + cpu = MIPS_CPU(object_new(object_class_get_name(oc)));\n>> \n>> object_property_set_bool(OBJECT(cpu), true, \"realized\", NULL);\n>> \n>> diff --git a/target/mips/translate_init.c b/target/mips/translate_init.c\n>> index 255d25bacd..8bbded46c4 100644\n>> --- a/target/mips/translate_init.c\n>> +++ b/target/mips/translate_init.c\n>> @@ -51,64 +51,9 @@\n>> #define MIPS_CONFIG5 \\\n>> ((0 << CP0C5_M))\n>> \n>> -/* MMU types, the first four entries have the same layout as the\n>> - CP0C0_MT field. */\n>> -enum mips_mmu_types {\n>> - MMU_TYPE_NONE,\n>> - MMU_TYPE_R4000,\n>> - MMU_TYPE_RESERVED,\n>> - MMU_TYPE_FMT,\n>> - MMU_TYPE_R3000,\n>> - MMU_TYPE_R6000,\n>> - MMU_TYPE_R8000\n>> -};\n>> -\n>> -struct mips_def_t {\n>> - const char *name;\n>> - int32_t CP0_PRid;\n>> - int32_t CP0_Config0;\n>> - int32_t CP0_Config1;\n>> - int32_t CP0_Config2;\n>> - int32_t CP0_Config3;\n>> - int32_t CP0_Config4;\n>> - int32_t CP0_Config4_rw_bitmask;\n>> - int32_t CP0_Config5;\n>> - int32_t CP0_Config5_rw_bitmask;\n>> - int32_t CP0_Config6;\n>> - int32_t CP0_Config7;\n>> - target_ulong CP0_LLAddr_rw_bitmask;\n>> - int CP0_LLAddr_shift;\n>> - int32_t SYNCI_Step;\n>> - int32_t CCRes;\n>> - int32_t CP0_Status_rw_bitmask;\n>> - int32_t CP0_TCStatus_rw_bitmask;\n>> - int32_t CP0_SRSCtl;\n>> - int32_t CP1_fcr0;\n>> - int32_t CP1_fcr31_rw_bitmask;\n>> - int32_t CP1_fcr31;\n>> - int32_t MSAIR;\n>> - int32_t SEGBITS;\n>> - int32_t PABITS;\n>> - int32_t CP0_SRSConf0_rw_bitmask;\n>> - int32_t CP0_SRSConf0;\n>> - int32_t CP0_SRSConf1_rw_bitmask;\n>> - int32_t CP0_SRSConf1;\n>> - int32_t CP0_SRSConf2_rw_bitmask;\n>> - int32_t CP0_SRSConf2;\n>> - int32_t CP0_SRSConf3_rw_bitmask;\n>> - int32_t CP0_SRSConf3;\n>> - int32_t CP0_SRSConf4_rw_bitmask;\n>> - int32_t CP0_SRSConf4;\n>> - int32_t CP0_PageGrain_rw_bitmask;\n>> - int32_t CP0_PageGrain;\n>> - target_ulong CP0_EBaseWG_rw_bitmask;\n>> - int insn_flags;\n>> - enum mips_mmu_types mmu_type;\n>> -};\n>> -\n>> /*****************************************************************************/\n>> /* MIPS CPU definitions */\n>> -static const mips_def_t mips_defs[] =\n>> +const mips_def_t mips_defs[] =\n>> {\n>> {\n>> .name = \"4Kc\",\n>> @@ -808,6 +753,7 @@ static const mips_def_t mips_defs[] =\n>> \n>> #endif\n>> };\n>> +const int mips_defs_number = ARRAY_SIZE(mips_defs);\n>> \n>> static const mips_def_t *cpu_mips_find_by_name (const char *name)\n> \n> What's needed to get rid of cpu_mips_find_by_name() completely?\n\nNext part of this series uses cpu_has_feature() for CPU_ISA and \nCPU_CPS_SMP and this function is removed. This was out of Igor's first \nseries scope.\n\nRegards,\n\nPhil.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"J5F9UnkY\"; dkim-atps=neutral"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xwPFY3vnXz9s72\n\tfor ;\n\tMon, 18 Sep 2017 08:39:12 +1000 (AEST)","from localhost ([::1]:33869 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dtiDV-0001iz-Qs\n\tfor incoming@patchwork.ozlabs.org; Sun, 17 Sep 2017 18:39:09 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:58033)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from )\n\tid 1dtiD3-0001iY-Vq\n\tfor qemu-devel@nongnu.org; Sun, 17 Sep 2017 18:38:47 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from )\n\tid 1dtiCy-0004pO-Tn\n\tfor qemu-devel@nongnu.org; Sun, 17 Sep 2017 18:38:41 -0400","from mail-qk0-x243.google.com ([2607:f8b0:400d:c09::243]:38841)\n\tby eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16)\n\t(Exim 4.71) (envelope-from )\n\tid 1dtiCy-0004n5-Ny\n\tfor qemu-devel@nongnu.org; Sun, 17 Sep 2017 18:38:36 -0400","by mail-qk0-x243.google.com with SMTP id c69so4528142qke.5\n\tfor ; Sun, 17 Sep 2017 15:38:35 -0700 (PDT)","from [192.168.1.240] ([181.93.89.178])\n\tby smtp.gmail.com with ESMTPSA id\n\td22sm4198881qkj.88.2017.09.17.15.38.30\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tSun, 17 Sep 2017 15:38:33 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=sender:subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=3Ihfdnl82Ig7DoGzduDs0DT2AAAkmfS2TIthCAxwIuo=;\n\tb=J5F9UnkY9J41Kzeww04tRXUvh8j5xyj5Wnf96REYqGmPawaXNrmYYNEME4nmQw+gWV\n\tpG7xy69d+CsNfNOADBDahL0FSxp8M+W6d877Oj2xYsUwwAxVX9bbdkt2KfWIrCqxxDu2\n\t5x/mQCfZm/mXh1UXB4Eg7BXojmeWq67/xswhF8Zy48ww9te2hLISxgsm/2UzoOMZ2pGf\n\tqhUuiPS2TG7lPsh85/5acG/t9vGJWmAz/gNdMuUHY38qNs4dMauk7hpjzQXallcdOpsJ\n\t3itXzhZEvqwF/Hqh7NQgFZllqDymucbg9NcWzYvzKCfEERvVqjLCBuOxAMbebEQ+oJbN\n\tE1uA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:sender:subject:to:cc:references:from:message-id\n\t:date:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=3Ihfdnl82Ig7DoGzduDs0DT2AAAkmfS2TIthCAxwIuo=;\n\tb=C1qqRs1zgihb6Z5/sris45sEw5tI21AIdM+btjOvScwcN8ycU2EsezCYUT17YzlKID\n\t+TVuMK3S5kwKb6zR3zhJt/sibnbh73HygFUc4fCPGsTEp7Y0/cb6yEsqfRv13SOIvUZ+\n\tR7Emy51qoL+IFsx14sUYf+s0PW0wt+AzOxqMfcPhv8RE0bhPErEcGY6xfy6boGDCyQI8\n\tX3uPYmqXBrolxT9mzOqaTbmfhnbZpBqvbhDtF4uYVWmFpd2xmy0ufpuxYEsOcobMbR/P\n\t9tPFh6H+vCMNiWsJdXyRt2p2Go+7YEJz005QaT4GuUFY9x1UjY+b8omL8FEAqfe1pzYX\n\tg5PQ==","X-Gm-Message-State":"AHPjjUhXCG9nIOJpdYAmeZfu6S4iCbv5QD3um6U5SO34unhBVGafLDlA\n\tE5f31jcQ0UBmkZvlqPk=","X-Google-Smtp-Source":"AOwi7QDs4brbDB9cR3EwqVLo2UllfU1lOshFF1Y31DYaO8fJrbWUlq76VAjix+VHiXHCktqQlwcqkA==","X-Received":"by 10.55.169.81 with SMTP id s78mr18457964qke.34.1505687914594; \n\tSun, 17 Sep 2017 15:38:34 -0700 (PDT)","To":"Eduardo Habkost ","References":"<20170830225225.27925-1-f4bug@amsat.org>\n\t<20170830225225.27925-6-f4bug@amsat.org>\n\t<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","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","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","X-detected-operating-system":"by eggs.gnu.org: Genre and OS details not\n\trecognized.","X-Received-From":"2607:f8b0:400d:c09::243","Subject":"Re: [Qemu-devel] [PATCH v2 5/7] mips: MIPSCPU model subclasses","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Thomas Huth , James Hogan ,\n\tqemu-devel@nongnu.org, Yongbok Kim , \n\t=?utf-8?q?Herv=C3=A9_Poussineau?= ,\n\tMarcel Apfelbaum ,\n\tIgor Mammedov , Aurelien Jarno\n\t","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]