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

[Alistair Francis]

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?

Alistair

>
> 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
>>