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

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

[Alistair Francis]

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

V4:
 - Rebase
 - Remove spaces
V3:
 - Make the varialbes static
V2:
 - Rebase
 - Reorder patches
 - Add a Raspberry Pi 2 CPU fix
V1:
 - Small fixes to prepare a series instead of RFC
 - Add commit messages for the commits
 - Expand the machine support to ARM machines
RFC v2:
 - Rebase on Igor's work
 - Use more QEMUisms inside the code
 - List the supported machines in a NULL terminated array


Alistair Francis (5):
  netduino2: Specify the valid CPUs
  bcm2836: Use the Cortex-A7 instead of Cortex-A15
  raspi: Specify the valid CPUs
  xlnx-zcu102: Specify the valid CPUs
  xilinx_zynq: Specify the valid CPUs

 hw/arm/bcm2836.c     |  2 +-
 hw/arm/netduino2.c   | 10 +++++++++-
 hw/arm/raspi.c       |  7 +++++++
 hw/arm/xilinx_zynq.c |  6 ++++++
 hw/arm/xlnx-zcu102.c | 17 +++++++++++++++++
 5 files changed, 40 insertions(+), 2 deletions(-)

-- 
2.14.1

[Qemu-devel] [PATCH v4 1/5] netduino2: Specify the valid CPUs

[Alistair Francis]

List all possible valid CPU options.

Although the board only ever has a Cortex-M3 we mark the Cortex-M4 as
supported because the Netduino2 Plus supports the Cortex-M4 and the
Netduino2 Plus is similar to the Netduino2.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

V3:
 - Add static property
V2:
 - Fixup allignment
RFC v2:
 - Use a NULL terminated list
 - Add the Cortex-M4 for testing


 hw/arm/netduino2.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
index f936017d4a..111a1d0aba 100644
--- a/hw/arm/netduino2.c
+++ b/hw/arm/netduino2.c
@@ -34,18 +34,26 @@ static void netduino2_init(MachineState *machine)
     DeviceState *dev;
 
     dev = qdev_create(NULL, TYPE_STM32F205_SOC);
-    qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3"));
+    qdev_prop_set_string(dev, "cpu-type", machine->cpu_type);
     object_property_set_bool(OBJECT(dev), true, "realized", &error_fatal);
 
     armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
                        FLASH_SIZE);
 }
 
+static const char *netduino_valid_cpus[] = {
+                                            ARM_CPU_TYPE_NAME("cortex-m3"),
+                                            ARM_CPU_TYPE_NAME("cortex-m4"),
+                                            NULL
+                                           };
+
 static void netduino2_machine_init(MachineClass *mc)
 {
     mc->desc = "Netduino 2 Machine";
     mc->init = netduino2_init;
     mc->ignore_memory_transaction_failures = true;
+    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m3");
+    mc->valid_cpu_types = netduino_valid_cpus;
 }
 
 DEFINE_MACHINE("netduino2", netduino2_machine_init)
-- 
2.14.1

[Qemu-devel] [PATCH v4 2/5] bcm2836: Use the Cortex-A7 instead of Cortex-A15

[Alistair Francis]

The BCM2836 uses a Cortex-A7 not a Cortex-A15. Update the device to use
the correct CPU.
https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/QA7_rev3.4.pdf

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
V3:
 - Use ARM_CPU_TYPE_NAME() macro
V2:
 - Fix the BCM2836 CPU

 hw/arm/bcm2836.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index 8c43291112..c477772484 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -30,7 +30,7 @@ static void bcm2836_init(Object *obj)
 
     for (n = 0; n < BCM2836_NCPUS; n++) {
         object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
-                          "cortex-a15-" TYPE_ARM_CPU);
+                          ARM_CPU_TYPE_NAME("cortex-a7"));
         object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]),
                                   &error_abort);
     }
-- 
2.14.1

[Qemu-devel] [PATCH v4 3/5] raspi: Specify the valid CPUs

[Alistair Francis]

List all possible valid CPU options.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

V4:
 - Remove spaces
V3:
 - Add static property
V2:
 - Fix the indentation

 hw/arm/raspi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index cd5fa8c3dc..bd175e8c16 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -158,6 +158,11 @@ static void raspi2_init(MachineState *machine)
     setup_boot(machine, 2, machine->ram_size - vcram_size);
 }
 
+static const char *raspi2_valid_cpus[] = {
+    ARM_CPU_TYPE_NAME("cortex-a7"),
+    NULL
+};
+
 static void raspi2_machine_init(MachineClass *mc)
 {
     mc->desc = "Raspberry Pi 2";
@@ -171,5 +176,7 @@ static void raspi2_machine_init(MachineClass *mc)
     mc->default_cpus = BCM2836_NCPUS;
     mc->default_ram_size = 1024 * 1024 * 1024;
     mc->ignore_memory_transaction_failures = true;
+    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a7");
+    mc->valid_cpu_types = raspi2_valid_cpus;
 };
 DEFINE_MACHINE("raspi2", raspi2_machine_init)
-- 
2.14.1

[Qemu-devel] [PATCH v4 4/5] xlnx-zcu102: Specify the valid CPUs

[Alistair Francis]

List all possible valid CPU options.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

An implementation for single CPU machines is still being discussed. A
solution proposed by Eduardo is this:

1) Change the default on TYPE_MACHINE to:
     mc->valid_cpu_types = { TYPE_CPU, NULL };

   This will keep the existing behavior for all boards.

2) mc->valid_cpu_types=NULL be interpreted as "no CPU model
   except the default is accepted" or "-cpu is not accepted" in
   machine_run_board_init() (I prefer the former, but both
   options would be correct)

3) Boards like xlnx_zynqmp could then just do this:

   static void xxx_class_init(...) {
       mc->default_cpu_type = MY_CPU_TYPE;
       /* Reason: XXX_init() is hardcoded to MY_CPU_TYPE */
       mc->valid_cpu_types = NULL;
   }
V4:
 - Remove spaces
V3:
 - Make variable static
V2:
 - Don't use the users -cpu
 - Fixup allignment

 hw/arm/xlnx-zcu102.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
index b126cf148b..08b5c28a3a 100644
--- a/hw/arm/xlnx-zcu102.c
+++ b/hw/arm/xlnx-zcu102.c
@@ -184,6 +184,11 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
     arm_load_kernel(s->soc.boot_cpu_ptr, &xlnx_zcu102_binfo);
 }
 
+static const char *xlnx_zynqmp_valid_cpus[] = {
+    ARM_CPU_TYPE_NAME("cortex-a53"),
+    NULL
+};
+
 static void xlnx_ep108_init(MachineState *machine)
 {
     XlnxZCU102 *s = EP108_MACHINE(machine);
@@ -216,6 +221,12 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data)
     mc->ignore_memory_transaction_failures = true;
     mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS;
     mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS;
+    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
+    /* The ZynqMP SoC is always a Cortex-A53. We add this here to give
+     * users a sane error if they specify a different CPU, but we never
+     * use their CPU choice.
+     */
+    mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
 }
 
 static const TypeInfo xlnx_ep108_machine_init_typeinfo = {
@@ -274,6 +285,12 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data)
     mc->ignore_memory_transaction_failures = true;
     mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS;
     mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS;
+    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
+    /* The ZynqMP SoC is always a Cortex-A53. We add this here to give
+     * users a sane error if they specify a different CPU, but we never
+     * use their CPU choice.
+     */
+    mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
 }
 
 static const TypeInfo xlnx_zcu102_machine_init_typeinfo = {
-- 
2.14.1

[Qemu-devel] [PATCH v4 5/5] xilinx_zynq: Specify the valid CPUs

[Alistair Francis]

List all possible valid CPU options.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

V4:
 - Remove spaces
V3:
 - Make variable static
V2:
 - Fixup alignment

 hw/arm/xilinx_zynq.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 1836a4ed45..4005230847 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -313,6 +313,11 @@ static void zynq_init(MachineState *machine)
     arm_load_kernel(ARM_CPU(first_cpu), &zynq_binfo);
 }
 
+static const char *xlnx_zynq_7000_valid_cpus[] = {
+    ARM_CPU_TYPE_NAME("cortex-a9"),
+    NULL
+};
+
 static void zynq_machine_init(MachineClass *mc)
 {
     mc->desc = "Xilinx Zynq Platform Baseboard for Cortex-A9";
@@ -321,6 +326,7 @@ static void zynq_machine_init(MachineClass *mc)
     mc->no_sdcard = 1;
     mc->ignore_memory_transaction_failures = true;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a9");
+    mc->valid_cpu_types = xlnx_zynq_7000_valid_cpus;
 }
 
 DEFINE_MACHINE("xilinx-zynq-a9", zynq_machine_init)
-- 
2.14.1

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

[Peter Maydell]

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.

thanks
-- PMM

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

[Alistair Francis]

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.

Alistair

>
> thanks
> -- PMM

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

[Alistair Francis]

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:

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++) {

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

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

[Eduardo Habkost]

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.

-- 
Eduardo

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

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

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

[Eduardo Habkost]

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.

-- 
Eduardo

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

[Igor Mammedov]

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

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

[Eduardo Habkost]

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.

-- 
Eduardo

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

[Alistair Francis]

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
>

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

[Eduardo Habkost]

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()?

-- 
Eduardo

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

[Igor Mammedov]

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.


 

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

[Eduardo Habkost]

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

-- 
Eduardo

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