From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60711) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ecXZx-0002E2-L3 for qemu-devel@nongnu.org; Fri, 19 Jan 2018 09:23:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ecXZu-0003HL-FQ for qemu-devel@nongnu.org; Fri, 19 Jan 2018 09:23:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35436) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ecXZu-0003Gd-5f for qemu-devel@nongnu.org; Fri, 19 Jan 2018 09:23:34 -0500 Date: Fri, 19 Jan 2018 12:23:28 -0200 From: Eduardo Habkost Message-ID: <20180119142328.GT5292@localhost.localdomain> References: <1516203816-19374-1-git-send-email-imammedo@redhat.com> <1516203816-19374-21-git-send-email-imammedo@redhat.com> <20180118014846.GM627@localhost.localdomain> <20180118111035.3ab0a533@redhat.com> <20180118191809.GB5292@localhost.localdomain> <20180119111439.0feb4f5a@redhat.com> <20180119131430.GR5292@localhost.localdomain> <20180119143949.7457412f@igors-macbook-pro.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180119143949.7457412f@igors-macbook-pro.local> Subject: Re: [Qemu-devel] [PATCH 20/24] machine: drop MachineState::cpu_model List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: qemu-devel@nongnu.org, Laurent Vivier , Marcel Apfelbaum , Paolo Bonzini On Fri, Jan 19, 2018 at 02:39:49PM +0100, Igor Mammedov wrote: > On Fri, 19 Jan 2018 11:14:30 -0200 > Eduardo Habkost wrote: > > > On Fri, Jan 19, 2018 at 11:14:39AM +0100, Igor Mammedov wrote: > > > On Thu, 18 Jan 2018 17:18:09 -0200 > > > Eduardo Habkost wrote: > > > > > > > On Thu, Jan 18, 2018 at 11:10:35AM +0100, Igor Mammedov wrote: > > > > > On Wed, 17 Jan 2018 23:48:46 -0200 > > > > > Eduardo Habkost wrote: > > > > > > > > > > > On Wed, Jan 17, 2018 at 04:43:32PM +0100, Igor Mammedov wrote: > > > > > > > The last user of it was machine type 'none', which used field > > > > > > > to create CPU id user requested it on CLI with -cpu option. > > > [...] > > > > > > > It looks like default_cpu_type is being overloaded for two > > > > different roles: 1) specifying the default CPU type; 2) finding > > > > the arch-specific class to be used to parse -cpu. > > > > > > > > In the case of null-machine, these two roles conflict with each > > > > other. I believe we can find other solutions instead of this > > > > hack that involves lying on MachineClass::default_cpu_type (and > > > > then having to work around the lie on machine_none_init()). > > > > > > > > I see multiple options: adding a new MachineClass field for that > > > > (e.g. resolving_cpu_type, which defaults to default_cpu_type if > > > > NULL); moving the CPU parsing code to arch_init.c (so it could > > > > use CPU_RESOLVING_TYPE or something similar); adding a optional > > > > MachineClass::parse_cpu_model hook. We could even try to get rid > > > > of CPUClass::parse_features completely > > > Adding hooks just for the sake on null-machine seems to be overkill, > > > I'd go for arch_init.c but it won't work for linux-user, how about > > > exec.c as following: > > > > > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > > > index 93bd546..0185589 100644 > > > --- a/include/qom/cpu.h > > > +++ b/include/qom/cpu.h > > > @@ -661,8 +661,7 @@ ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model); > > > [...] > > > > > > diff --git a/exec.c b/exec.c > > > index d28fc0c..4543f06 100644 > > > --- a/exec.c > > > +++ b/exec.c > > > @@ -817,6 +817,29 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp) > > > #endif > > > } > > > > > > +const char *parse_cpu_model(const char *cpu_model) > > > +{ > > > + ObjectClass *oc; > > > + CPUClass *cc; > > > + gchar **model_pieces; > > > + const char *cpu_type; > > > + > > > + model_pieces = g_strsplit(cpu_model, ",", 2); > > > + > > > + oc = cpu_class_by_name(CPU_RESOLVING_TYPE, model_pieces[0]); > > > + if (oc == NULL) { > > > + error_report("unable to find CPU model '%s'", model_pieces[0]); > > > + g_strfreev(model_pieces); > > > + exit(EXIT_FAILURE); > > > + } > > > + > > > + cpu_type = object_class_get_name(oc); > > > + cc = CPU_CLASS(oc); > > > + cc->parse_features(cpu_type, model_pieces[1], &error_fatal); > > > + g_strfreev(model_pieces); > > > + return cpu_type; > > > +} > > > > Sounds good to me. Only two comments: > > > > This looks like duplication of cpu_parse_cpu_model(). Should > > this function body be replaced with: > > cpu_parse_cpu_model(CPU_RESOLVING_TYPE, cpu_model) > it's cpu_parse_cpu_model() which is moved to exec.c > and first typename argument is replaced by inline CPU_RESOLVING_TYPE > > > > I would move this to arch_init.c, because that's where existing > > target-dependent initialization code lives. > arch_init.c doesn't fit linux-user, it has only sys emulation code > while exec.c is used by both and still target depended so CPU_RESOLVING_TYPE > could be used there I don't really like adding more code to the mess that is exec.c, but this looks like the simplest solution. -- Eduardo