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

[Alistair Francis]

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.

Ok, so it sounds like I'll respin this series with an extra column in
the array for human readable names. Then in the future we can work on
removing that.

Alistair

>
> --
> Eduardo
>