[Qemu-devel] [RFC v2 0/2] Add a valid_cpu_types property

[Qemu-devel] [RFC v2 0/2] 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.

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: cortex-a9-arm-cpu
The valid options are: cortex-m3-arm-cpu, cortex-m4-arm-cpu

RFC v2:
 - Rebase on Igor's work
 - Use more QEMUisms inside the code
 - List the supported machines in a NULL terminated array

Alistair Francis (2):
  machine: Add a valid_cpu_types property
  netduino2: Specify the valid CPUs

 hw/arm/netduino2.c  | 10 +++++++++-
 hw/core/machine.c   | 35 +++++++++++++++++++++++++++++++++++
 include/hw/boards.h |  1 +
 3 files changed, 45 insertions(+), 1 deletion(-)

-- 
2.11.0

[Qemu-devel] [RFC v2 1/2] machine: Add a valid_cpu_types property

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

RFC v2:
 - Rebase on Igor's cpu_type work
 - Use object_class_dynamic_cast()
 - Use a NULL terminated cahr** list
 - Do the check before the machine_class init() is called


 hw/core/machine.c   | 35 +++++++++++++++++++++++++++++++++++
 include/hw/boards.h |  1 +
 2 files changed, 36 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 80647edc2a..abebfabdb8 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -758,6 +758,41 @@ void machine_run_board_init(MachineState *machine)
     if (nb_numa_nodes) {
         machine_numa_finish_init(machine);
     }
+
+    if (machine_class->valid_cpu_types && machine->cpu_type) {
+        int i;
+
+        for (i = 0; machine_class->valid_cpu_types[i]; i++) {
+            ObjectClass *class = object_class_by_name(machine->cpu_type);
+
+            if (!class) {
+                break;
+            }
+
+            if (object_class_dynamic_cast(class,
+                                          machine_class->valid_cpu_types[i])) {
+                /* The user specificed CPU is in the valid field, we are
+                 * good to go.
+                 */
+                goto done;
+            }
+        }
+
+        /* The user specified CPU must not be a valid CPU, print a sane
+         * error
+         */
+        error_report("Invalid CPU: %s", machine->cpu_type);
+        error_printf("The valid options are: %s",
+                     machine_class->valid_cpu_types[0]);
+        for (i = 1; machine_class->valid_cpu_types[i]; i++) {
+            error_printf(", %s", machine_class->valid_cpu_types[i]);
+        }
+        error_printf("\n");
+
+        exit(1);
+    }
+
+done:
     machine_class->init(machine);
 }
 
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 156e0a5701..191a5b3cd8 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -191,6 +191,7 @@ struct MachineClass {
     bool has_hotpluggable_cpus;
     bool ignore_memory_transaction_failures;
     int numa_mem_align_shift;
+    const char **valid_cpu_types;
     void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
                                  int nb_nodes, ram_addr_t size);
 
-- 
2.11.0

[Qemu-devel] [RFC v2 2/2] netduino2: Specify the valid CPUs

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

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..391742ea11 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);
 }
 
+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.11.0

Re: [Qemu-devel] [RFC v2 0/2] Add a valid_cpu_types property

[Alistair Francis]

On Thu, Sep 21, 2017 at 4:41 PM, 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.
>
> 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: cortex-a9-arm-cpu
> The valid options are: cortex-m3-arm-cpu, cortex-m4-arm-cpu
>

Any comments on this?

Otherwise I'll prepare a patch series and start adding support for
more machines.

Thanks,
Alistair

> RFC v2:
>  - Rebase on Igor's work
>  - Use more QEMUisms inside the code
>  - List the supported machines in a NULL terminated array
>
> Alistair Francis (2):
>   machine: Add a valid_cpu_types property
>   netduino2: Specify the valid CPUs
>
>  hw/arm/netduino2.c  | 10 +++++++++-
>  hw/core/machine.c   | 35 +++++++++++++++++++++++++++++++++++
>  include/hw/boards.h |  1 +
>  3 files changed, 45 insertions(+), 1 deletion(-)
>
> --
> 2.11.0
>

Re: [Qemu-devel] [RFC v2 0/2] Add a valid_cpu_types property

[Philippe Mathieu-Daudé]

Hi Alistair,

On 10/02/2017 03:42 PM, Alistair Francis wrote:
> On Thu, Sep 21, 2017 at 4:41 PM, 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.
>>
>> 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: cortex-a9-arm-cpu
>> The valid options are: cortex-m3-arm-cpu, cortex-m4-arm-cpu
>>
> 
> Any comments on this?

No negative comment, so far so good!

> 
> Otherwise I'll prepare a patch series and start adding support for
> more machines.

Yes please, I'm waiting your work get merged to continue some SoC 
cleanups/improvements.

> 
> Thanks,
> Alistair
> 
>> RFC v2:
>>   - Rebase on Igor's work
>>   - Use more QEMUisms inside the code
>>   - List the supported machines in a NULL terminated array
>>
>> Alistair Francis (2):
>>    machine: Add a valid_cpu_types property
>>    netduino2: Specify the valid CPUs
>>
>>   hw/arm/netduino2.c  | 10 +++++++++-
>>   hw/core/machine.c   | 35 +++++++++++++++++++++++++++++++++++
>>   include/hw/boards.h |  1 +
>>   3 files changed, 45 insertions(+), 1 deletion(-)
>>
>> --
>> 2.11.0
>>
> 

Re: [Qemu-devel] [RFC v2 1/2] machine: Add a valid_cpu_types property

[Eduardo Habkost]

On Thu, Sep 21, 2017 at 04:41:50PM -0700, Alistair Francis wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> 
> RFC v2:
>  - Rebase on Igor's cpu_type work
>  - Use object_class_dynamic_cast()
>  - Use a NULL terminated cahr** list
>  - Do the check before the machine_class init() is called
> 
> 
>  hw/core/machine.c   | 35 +++++++++++++++++++++++++++++++++++
>  include/hw/boards.h |  1 +
>  2 files changed, 36 insertions(+)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 80647edc2a..abebfabdb8 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -758,6 +758,41 @@ void machine_run_board_init(MachineState *machine)
>      if (nb_numa_nodes) {
>          machine_numa_finish_init(machine);
>      }
> +
> +    if (machine_class->valid_cpu_types && machine->cpu_type) {
> +        int i;
> +
> +        for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> +            ObjectClass *class = object_class_by_name(machine->cpu_type);
> +
> +            if (!class) {
> +                break;
> +            }
> +
> +            if (object_class_dynamic_cast(class,
> +                                          machine_class->valid_cpu_types[i])) {
> +                /* The user specificed CPU is in the valid field, we are
> +                 * good to go.
> +                 */
> +                goto done;

I would move the object_class_by_name() call outside the for loop
and remove the "goto", like this:

    if (machine->cpu_type && machine_class->valid_cpu_types) {
        ObjectClass *class = object_class_by_name(machine->cpu_type);
        int i;

        /* machine->cpu_type is supposed to be always a valid QOM type */
        assert(class);

        for (i = 0; machine_class->valid_cpu_types[i]; i++) {
            if (object_class_dynamic_cast(class,
                                          machine_class->valid_cpu_types[i])) {
                /* Valid CPU type, we're good to go */
                break;
            }
        }
        if (!machine_class->valid_cpu_types[i]) {
            error_report(...);
            ...
        }
    }

    machine_class->init(machine);

> +            }
> +        }
> +
> +        /* The user specified CPU must not be a valid CPU, print a sane
> +         * error
> +         */
> +        error_report("Invalid CPU: %s", machine->cpu_type);
> +        error_printf("The valid options are: %s",
> +                     machine_class->valid_cpu_types[0]);
> +        for (i = 1; machine_class->valid_cpu_types[i]; i++) {
> +            error_printf(", %s", machine_class->valid_cpu_types[i]);
> +        }
> +        error_printf("\n");

I would still like to make this share code with
query-cpu-definitions one day, but I'm OK with this
implementation.

I would just rewrite the message as "valid types are:" instead of
"valid options are:" and "Invalid CPU type:" instead of "Invalid
CPU:", because the -cpu option doesn't need to match a string in
valid_cpu_types exactly, it just needs to resolve to a type that
implements a valid type.


> +
> +        exit(1);
> +    }
> +
> +done:
>      machine_class->init(machine);
>  }
>  
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 156e0a5701..191a5b3cd8 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -191,6 +191,7 @@ struct MachineClass {
>      bool has_hotpluggable_cpus;
>      bool ignore_memory_transaction_failures;
>      int numa_mem_align_shift;
> +    const char **valid_cpu_types;
>      void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
>                                   int nb_nodes, ram_addr_t size);
>  
> -- 
> 2.11.0
> 
> 

-- 
Eduardo