From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32991) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eZa2z-00056R-BN for qemu-devel@nongnu.org; Thu, 11 Jan 2018 05:25:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eZa2v-0007S7-9e for qemu-devel@nongnu.org; Thu, 11 Jan 2018 05:25:21 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43544) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eZa2u-0007Qn-P2 for qemu-devel@nongnu.org; Thu, 11 Jan 2018 05:25:16 -0500 Date: Thu, 11 Jan 2018 11:25:08 +0100 From: Igor Mammedov Message-ID: <20180111112508.46410b61@redhat.com> In-Reply-To: <20180110214800.GU6646@localhost.localdomain> References: <20171220220608.GH24025@localhost.localdomain> <20171228143931.516b9e60@igors-macbook-pro.local> <20171228145904.GJ24856@localhost.localdomain> <20180110214800.GU6646@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 0/5] Add a valid_cpu_types property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Alistair Francis , Marcel Apfelbaum , Peter Maydell , Philippe =?UTF-8?B?TWF0aGlldS1EYXVkw6k=?= , QEMU Developers On Wed, 10 Jan 2018 19:48:00 -0200 Eduardo Habkost wrote: > On Wed, Jan 10, 2018 at 01:30:29PM -0800, Alistair Francis wrote: > > On Thu, Dec 28, 2017 at 6:59 AM, Eduardo Habkost wrote: > > > On Thu, Dec 28, 2017 at 02:39:31PM +0100, Igor Mammedov wrote: > > >> On Fri, 22 Dec 2017 11:47:00 -0800 > > >> Alistair Francis wrote: > > >> > > >> > On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis > > >> > wrote: > > >> > > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost wrote: > > >> > >> On Tue, Dec 19, 2017 at 05:03:59PM -0800, Alistair Francis wrote: > > >> > >>> On Tue, Dec 19, 2017 at 4:55 PM, Alistair Francis > > >> > >>> wrote: > > >> > >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell wrote: > > >> > >>> >> On 20 December 2017 at 00:27, Alistair Francis > > >> > >>> >> wrote: > > >> > >>> >>> There are numorous QEMU machines that only have a single or a handful of > > >> > >>> >>> valid CPU options. To simplyfy the management of specificying which CPU > > >> > >>> >>> is/isn't valid let's create a property that can be set in the machine > > >> > >>> >>> init. We can then check to see if the user supplied CPU is in that list > > >> > >>> >>> or not. > > >> > >>> >>> > > >> > >>> >>> I have added the valid_cpu_types for some ARM machines only at the > > >> > >>> >>> moment. > > >> > >>> >>> > > >> > >>> >>> Here is what specifying the CPUs looks like now: > > >> > >>> >>> > > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m3" -S > > >> > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information > > >> > >>> >>> (qemu) info cpus > > >> > >>> >>> * CPU #0: thread_id=24175 > > >> > >>> >>> (qemu) q > > >> > >>> >>> > > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m4" -S > > >> > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information > > >> > >>> >>> (qemu) q > > >> > >>> >>> > > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m5" -S > > >> > >>> >>> qemu-system-aarch64: unable to find CPU model 'cortex-m5' > > >> > >>> >>> > > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-a9" -S > > >> > >>> >>> qemu-system-aarch64: Invalid CPU type: cortex-a9-arm-cpu > > >> > >>> >>> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu > > >> > >>> >> > > >> > >>> >> Thanks for this; we really should be more strict about > > >> > >>> >> forbidding "won't work" combinations than we have > > >> > >>> >> been in the past. > > >> > >>> >> > > >> > >>> >> In the last of these cases, I think that when we > > >> > >>> >> list the invalid CPU type and the valid types > > >> > >>> >> we should use the same names we want the user to > > >> > >>> >> use on the command line, without the "-arm-cpu" > > >> > >>> >> suffixes. > > >> > >>> > > > >> > >>> > Hmm... That is a good point, it is confusing that they don't line up. > > >> > >> > > >> > >> Agreed. > > >> > >> > > >> > >>> > > > >> > >>> > The problem is that we are just doing a simple > > >> > >>> > object_class_dynamic_cast() in hw/core/machine.c which I think > > >> > >>> > (untested) requires us to have the full name in the valid cpu array. > > >> > >> [...] > > >> > >>> > > >> > >>> I think an earlier version of my previous series adding the support to > > >> > >>> machine.c did string comparison, but it was decided to utilise objects > > >> > >>> instead. One option is to make the array 2 wide and have the second > > >> > >>> string be user friendly? > > >> > >> > > >> > >> Making the array 2-column will duplicate information that we can > > >> > >> already find out using other methods, and it won't solve the > > >> > >> problem if an entry has a parent class with multiple subclasses > > >> > >> (the original reason I suggested object_class_dynamic_cast()). > > >> > >> > > >> > >> The main obstacle to fix this easily is that we do have a common > > >> > >> ObjectClass *cpu_class_by_name(const char *cpu_model) > > >> > >> function, but not a common method to get the model name from a > > >> > >> CPUClass. Implementing this is possible, but probably better to > > >> > >> do it after moving the existing arch-specific CPU model > > >> > >> enumeration hooks to common code (currently we duplicate lots of > > >> > >> CPU enumeration/lookup boilerplate code that we shouldn't have > > >> > >> to). > > >> > >> > > >> > >> Listing only the human-friendly names in the array like in the > > >> > >> original patch could be a reasonable temporary solution. It > > >> > >> won't allow us to use a single entry for all subclasses of a > > >> > >> given type by now (e.g. listing only TYPE_X86_CPU on PC), but at > > >> > >> least we can address this issue without waiting for a refactor of > > >> > >> the CPU model enumeration code. > > >> > > > >> > Ah, I just re-read this. Do you mean go back to the original RFC and > > >> > just use strcmp() to compare the human readable cpu_model? > > >> It's sort of going backwards but I won't object to this as far as you > > >> won't use machine->cpu_model (which is in process of being removed) > > > > Wait, machine->cpu_model is the human readable name. Without using > > that we can't use just human readable strings for the valid cpu types. > > Well, if we want to deprecate machine->cpu_model we need to offer > an alternative first, otherwise we can't prevent people from > using it. > > Igor, do you see an (existing) alternative to machine->cpu_model > that would allow us to avoid using it in > machine_run_board_init()? In recently merged refactoring machine->cpu_model is being replaced by machine->cpu_type. So currently we don't need machine->cpu_model anywhere except machine('none'), and once I refactor that it could be dropped completely and after some work on *-user targets we can practically get rid of cpu_model notion completely (excluding of -cpu option parser). My dislike of idea is that it's adding back cpumodel strings in boards code again (which I've just got rid of). I hate to say that but it looks like we need more refactoring for this series to print cpumodels back to user. We already have FOO_cpu_list()/FOO_query_cpu_definitions() which already do cpu type => cpumodel conversion (and even have some code duplication within a target), I'd suggest generalizing that across targets and then using generic helper for printing back to user converted cpu types from mc->valid_cpu_types which this series introduces.