Re: [Qemu-devel] [PATCH v4 0/5] Add a valid_cpu_types property

[Alistair Francis]

On Thu, Jan 11, 2018 at 4:58 AM, Eduardo Habkost <address@hidden> wrote:
> On Thu, Jan 11, 2018 at 11:25:08AM +0100, Igor Mammedov wrote:
>> On Wed, 10 Jan 2018 19:48:00 -0200
>> Eduardo Habkost <address@hidden> 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 <address@hidden> 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 <address@hidden> wrote:
>> > > >>
>> > > >> > On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis
>> > > >> > <address@hidden> wrote:
>> > > >> > > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <address@hidden> 
>> > > >> > > 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
>> > > >> > >>> <address@hidden> wrote:
>> > > >> > >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell 
>> > > >> > >>> > <address@hidden> wrote:
>> > > >> > >>> >> On 20 December 2017 at 00:27, Alistair Francis
>> > > >> > >>> >> <address@hidden> 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.
>
> I agree with the long term goal of making cpu type => cpu model
> conversion generic.  But until we refactor the arch-specific code
> and implement that, we have 2 options:
>
> 1) Keep printing a confusing error message until we implement cpu
>    type => cpu model conversion;
> 2) Keep the MachineState::cpu_model field until we implement cpu
>    type => cpu model conversion.
>
> I don't see any reason to pick (1) instead of (2).

Ok, I'm going over this again (sorry for the delay).

I'm going to respin using machine->cpu_model. It shouldn't be hard to
refactor in the future to machine->cpu_type, once that is or can be
translated to a user friendly string.

Alistair

>
> --
> Eduardo
>