diff mbox

[v4,0/5] Add a valid_cpu_types property

Message ID CAKmqyKMKL_BGSbaetubaPv8gkmF9XFFdDwkXAPsUZdV1qkOyqA@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Alistair Francis Dec. 20, 2017, 1:03 a.m. UTC
On Tue, Dec 19, 2017 at 4:55 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 20 December 2017 at 00:27, Alistair Francis
>> <alistair.francis@xilinx.com> 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.
>
> 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.

Yeah, so I tested this diff on top of this series:


and I get this:

$ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel
./u-boot.elf -nographic -cpu "cortex-m4" -S
qemu-system-aarch64: Invalid CPU type: cortex-m4
The valid types are: cortex-m3, cortex-m4

So we loose the object_class_dynamic_cast() ability.

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?

Alistair

>
> Alistair
>
>>
>> thanks
>> -- PMM

Comments

Eduardo Habkost Dec. 20, 2017, 10:06 p.m. UTC | #1
On Tue, Dec 19, 2017 at 05:03:59PM -0800, Alistair Francis wrote:
> On Tue, Dec 19, 2017 at 4:55 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> >> On 20 December 2017 at 00:27, Alistair Francis
> >> <alistair.francis@xilinx.com> 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.
Alistair Francis Dec. 22, 2017, 6:45 p.m. UTC | #2
On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> 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
>> <alistair.francis@xilinx.com> wrote:
>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> >> On 20 December 2017 at 00:27, Alistair Francis
>> >> <alistair.francis@xilinx.com> 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
>
Alistair Francis Dec. 22, 2017, 7:47 p.m. UTC | #3
On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> 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
>>> <alistair.francis@xilinx.com> wrote:
>>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> >> On 20 December 2017 at 00:27, Alistair Francis
>>> >> <alistair.francis@xilinx.com> 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
>>
Eduardo Habkost Dec. 22, 2017, 9:26 p.m. UTC | #4
On Fri, Dec 22, 2017 at 11:47:00AM -0800, Alistair Francis wrote:
> On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
> > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> 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
> >>> <alistair.francis@xilinx.com> wrote:
> >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>> >> On 20 December 2017 at 00:27, Alistair Francis
> >>> >> <alistair.francis@xilinx.com> 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?

Yes, I think this would be simpler, considering that the current
users don't need the object_class_dynamic_cast() stuff.

Then whoever decide to make PC simply list TYPE_X86_CPU later
(possibly me) will need to clean up the CPU model
enumeration/lookup code and implement a
cpu_model_from_type_name() helper.
Igor Mammedov Dec. 28, 2017, 1:39 p.m. UTC | #5
On Fri, 22 Dec 2017 11:47:00 -0800
Alistair Francis <alistair.francis@xilinx.com> wrote:

> On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
> > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> 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
> >>> <alistair.francis@xilinx.com> wrote:
> >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>> >> On 20 December 2017 at 00:27, Alistair Francis
> >>> >> <alistair.francis@xilinx.com> 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)


BTW:
how hard is it, to add  cpu_type2cpu_name function?

> 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
> >>
Eduardo Habkost Dec. 28, 2017, 2:59 p.m. UTC | #6
On Thu, Dec 28, 2017 at 02:39:31PM +0100, Igor Mammedov wrote:
> On Fri, 22 Dec 2017 11:47:00 -0800
> Alistair Francis <alistair.francis@xilinx.com> wrote:
> 
> > On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis
> > <alistair.francis@xilinx.com> wrote:
> > > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> 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
> > >>> <alistair.francis@xilinx.com> wrote:
> > >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> > >>> >> On 20 December 2017 at 00:27, Alistair Francis
> > >>> >> <alistair.francis@xilinx.com> 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)
> 
> 
> BTW:
> how hard is it, to add  cpu_type2cpu_name function?

It shouldn't be hard, but I would like to avoid adding yet
another arch-specific hook just for that.  Probably we would need
to clean up the existing CPU model enumeration/lookup code if we
want to avoid increase duplication of code on arch-specific
hooks.
Alistair Francis Jan. 10, 2018, 9:30 p.m. UTC | #7
On Thu, Dec 28, 2017 at 6:59 AM, Eduardo Habkost <ehabkost@redhat.com> 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 <alistair.francis@xilinx.com> wrote:
>>
>> > On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis
>> > <alistair.francis@xilinx.com> wrote:
>> > > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> 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
>> > >>> <alistair.francis@xilinx.com> wrote:
>> > >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> > >>> >> On 20 December 2017 at 00:27, Alistair Francis
>> > >>> >> <alistair.francis@xilinx.com> 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.

Alistair

>>
>>
>> BTW:
>> how hard is it, to add  cpu_type2cpu_name function?
>
> It shouldn't be hard, but I would like to avoid adding yet
> another arch-specific hook just for that.  Probably we would need
> to clean up the existing CPU model enumeration/lookup code if we
> want to avoid increase duplication of code on arch-specific
> hooks.
>
> --
> Eduardo
>
Eduardo Habkost Jan. 10, 2018, 9:48 p.m. UTC | #8
On Wed, Jan 10, 2018 at 01:30:29PM -0800, Alistair Francis wrote:
> On Thu, Dec 28, 2017 at 6:59 AM, Eduardo Habkost <ehabkost@redhat.com> 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 <alistair.francis@xilinx.com> wrote:
> >>
> >> > On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis
> >> > <alistair.francis@xilinx.com> wrote:
> >> > > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> 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
> >> > >>> <alistair.francis@xilinx.com> wrote:
> >> > >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> >> > >>> >> On 20 December 2017 at 00:27, Alistair Francis
> >> > >>> >> <alistair.francis@xilinx.com> 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()?
Igor Mammedov Jan. 11, 2018, 10:25 a.m. UTC | #9
On Wed, 10 Jan 2018 19:48:00 -0200
Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com> 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 <alistair.francis@xilinx.com> wrote:
> > >>  
> > >> > On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis
> > >> > <alistair.francis@xilinx.com> wrote:  
> > >> > > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> 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
> > >> > >>> <alistair.francis@xilinx.com> wrote:  
> > >> > >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote:  
> > >> > >>> >> On 20 December 2017 at 00:27, Alistair Francis
> > >> > >>> >> <alistair.francis@xilinx.com> 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.
Eduardo Habkost Jan. 11, 2018, 12:58 p.m. UTC | #10
On Thu, Jan 11, 2018 at 11:25:08AM +0100, Igor Mammedov wrote:
> On Wed, 10 Jan 2018 19:48:00 -0200
> Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com> 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 <alistair.francis@xilinx.com> wrote:
> > > >>  
> > > >> > On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis
> > > >> > <alistair.francis@xilinx.com> wrote:  
> > > >> > > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> 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
> > > >> > >>> <alistair.francis@xilinx.com> wrote:  
> > > >> > >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote:  
> > > >> > >>> >> On 20 December 2017 at 00:27, Alistair Francis
> > > >> > >>> >> <alistair.francis@xilinx.com> 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).
Alistair Francis Feb. 1, 2018, 11:21 p.m. UTC | #11
On Thu, Jan 11, 2018 at 4:58 AM, Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com> 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 <ehabkost@redhat.com> 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 <alistair.francis@xilinx.com> wrote:
>> > > >>
>> > > >> > On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis
>> > > >> > <alistair.francis@xilinx.com> wrote:
>> > > >> > > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> 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
>> > > >> > >>> <alistair.francis@xilinx.com> wrote:
>> > > >> > >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> > > >> > >>> >> On 20 December 2017 at 00:27, Alistair Francis
>> > > >> > >>> >> <alistair.francis@xilinx.com> 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
>
diff mbox

Patch

diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
index 111a1d0aba..702e5e8fcf 100644
--- a/hw/arm/netduino2.c
+++ b/hw/arm/netduino2.c
@@ -42,8 +42,8 @@  static void netduino2_init(MachineState *machine)
 }

 static const char *netduino_valid_cpus[] = {
-                                            ARM_CPU_TYPE_NAME("cortex-m3"),
-                                            ARM_CPU_TYPE_NAME("cortex-m4"),
+                                            "cortex-m3",
+                                            "cortex-m4",
                                             NULL
                                            };

diff --git a/hw/core/machine.c b/hw/core/machine.c
index c857f3f934..4b817ccc58 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -761,8 +761,8 @@  void machine_run_board_init(MachineState *machine)
     /* If the machine supports the valid_cpu_types check and the user
      * specified a CPU with -cpu check here that the user CPU is supported.
      */
-    if (machine_class->valid_cpu_types && machine->cpu_type) {
-        ObjectClass *class = object_class_by_name(machine->cpu_type);
+    if (machine_class->valid_cpu_types && machine->cpu_model) {
+        ObjectClass *class = object_class_by_name(machine->cpu_model);
         int i;

         for (i = 0; machine_class->valid_cpu_types[i]; i++) {
@@ -777,7 +777,7 @@  void machine_run_board_init(MachineState *machine)

         if (!machine_class->valid_cpu_types[i]) {
             /* The user specified CPU is not valid */
-            error_report("Invalid CPU type: %s", machine->cpu_type);
+            error_report("Invalid CPU type: %s", machine->cpu_model);
             error_printf("The valid types are: %s",
                          machine_class->valid_cpu_types[0]);
             for (i = 1; machine_class->valid_cpu_types[i]; i++) {