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

[Qemu-devel] [PATCH v1 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

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):
  machine: Add a valid_cpu_types property
  netduino2: Specify the valid CPUs
  xlnx-zcu102: Specify the valid CPUs
  xilinx_zynq: : Specify the valid CPUs
  raspi: : Specify the valid CPUs

 hw/arm/netduino2.c           |  9 ++++++++-
 hw/arm/raspi.c               |  6 ++++++
 hw/arm/xilinx_zynq.c         |  5 +++++
 hw/arm/xlnx-zcu102.c         | 10 ++++++++++
 hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
 hw/core/machine.c            | 35 +++++++++++++++++++++++++++++++++++
 include/hw/arm/xlnx-zynqmp.h |  1 +
 include/hw/boards.h          |  1 +
 8 files changed, 75 insertions(+), 8 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v1 1/5] machine: Add a valid_cpu_types property

[Alistair Francis]

This patch add a MachineClass element that can be set in the machine C
code to specify a list of supported CPU types. If the supported CPU
types are specified the user enter CPU (by -cpu at runtime) is checked
against the supported types and QEMU exits if they aren't supported.

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

V1:
 - Use 'type' in the error message
 - Move a uneeded operation out of the for loop
 - Remove the goto
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..3afc6a7b5b 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 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);
+        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])) {
+                /* The user specificed CPU is in the valid field, we are
+                 * good to go.
+                 */
+                break;
+            }
+        }
+
+        if (!machine_class->valid_cpu_types[i]) {
+            /* The user specified CPU must not be a valid CPU */
+            error_report("Invalid CPU type: %s", machine->cpu_type);
+            error_printf("The valid types 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);
+        }
+    }
+
     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] [PATCH v1 2/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>
---

RFC v2:
 - Use a NULL terminated list
 - Add the Cortex-M4 for testing


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

diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
index f936017d4a..b68ecf2c08 100644
--- a/hw/arm/netduino2.c
+++ b/hw/arm/netduino2.c
@@ -34,18 +34,25 @@ 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

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

[Alistair Francis]

List all possible valid CPU options.

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

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

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 1836a4ed45..de1e0bbce1 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -313,6 +313,10 @@ static void zynq_init(MachineState *machine)
     arm_load_kernel(ARM_CPU(first_cpu), &zynq_binfo);
 }
 
+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 +325,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.11.0

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

[Alistair Francis]

List all possible valid CPU options.

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

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

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 5941c9f751..555db0f258 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -158,6 +158,10 @@ static void raspi2_init(MachineState *machine)
     setup_boot(machine, 2, machine->ram_size - vcram_size);
 }
 
+const char *raspi2_valid_cpus[] = { ARM_CPU_TYPE_NAME("cortex-a7"),
+                                    NULL
+                                  };
+
 static void raspi2_machine_init(MachineClass *mc)
 {
     mc->desc = "Raspberry Pi 2";
@@ -169,5 +173,7 @@ static void raspi2_machine_init(MachineClass *mc)
     mc->max_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.11.0

Re: [Qemu-devel] [PATCH v1 1/5] machine: Add a valid_cpu_types property

[Eduardo Habkost]

On Tue, Oct 03, 2017 at 01:05:09PM -0700, Alistair Francis wrote:
> This patch add a MachineClass element that can be set in the machine C
> code to specify a list of supported CPU types. If the supported CPU
> types are specified the user enter CPU (by -cpu at runtime) is checked
> against the supported types and QEMU exits if they aren't supported.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---

Thanks!

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

However, I will squash the following changes before queueing,
because:

* object_class_dynamic_cast() is safe even if class is NULL,
  so there's no need to validate cpu_type here.
* "must not be valid" sounds like the CPU is not allowed to be a
  valid CPU, so I rewrote the comment.


diff --git a/hw/core/machine.c b/hw/core/machine.c
index 3afc6a7b5b..36c2fb069c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -766,9 +766,6 @@ void machine_run_board_init(MachineState *machine)
         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])) {
@@ -780,7 +777,7 @@ void machine_run_board_init(MachineState *machine)
         }
 
         if (!machine_class->valid_cpu_types[i]) {
-            /* The user specified CPU must not be a valid CPU */
+            /* The user specified CPU is not valid */
             error_report("Invalid CPU type: %s", machine->cpu_type);
             error_printf("The valid types are: %s",
                          machine_class->valid_cpu_types[0]);
> 
> V1:
>  - Use 'type' in the error message
>  - Move a uneeded operation out of the for loop
>  - Remove the goto
> 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..3afc6a7b5b 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 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);
> +        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])) {
> +                /* The user specificed CPU is in the valid field, we are
> +                 * good to go.
> +                 */
> +                break;
> +            }
> +        }
> +
> +        if (!machine_class->valid_cpu_types[i]) {
> +            /* The user specified CPU must not be a valid CPU */
> +            error_report("Invalid CPU type: %s", machine->cpu_type);
> +            error_printf("The valid types 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);
> +        }
> +    }
> +
>      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

Re: [Qemu-devel] [PATCH v1 1/5] machine: Add a valid_cpu_types property

[Alistair Francis]

On Tue, Oct 3, 2017 at 1:23 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Oct 03, 2017 at 01:05:09PM -0700, Alistair Francis wrote:
>> This patch add a MachineClass element that can be set in the machine C
>> code to specify a list of supported CPU types. If the supported CPU
>> types are specified the user enter CPU (by -cpu at runtime) is checked
>> against the supported types and QEMU exits if they aren't supported.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>
> Thanks!
>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>
> However, I will squash the following changes before queueing,
> because:
>
> * object_class_dynamic_cast() is safe even if class is NULL,
>   so there's no need to validate cpu_type here.
> * "must not be valid" sounds like the CPU is not allowed to be a
>   valid CPU, so I rewrote the comment.
>
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 3afc6a7b5b..36c2fb069c 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -766,9 +766,6 @@ void machine_run_board_init(MachineState *machine)
>          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])) {
> @@ -780,7 +777,7 @@ void machine_run_board_init(MachineState *machine)
>          }
>
>          if (!machine_class->valid_cpu_types[i]) {
> -            /* The user specified CPU must not be a valid CPU */
> +            /* The user specified CPU is not valid */
>              error_report("Invalid CPU type: %s", machine->cpu_type);
>              error_printf("The valid types are: %s",
>                           machine_class->valid_cpu_types[0]);

Looks good to me.

Does that mean you are taking the whole series now?

Thanks,
Alistair

>>
>> V1:
>>  - Use 'type' in the error message
>>  - Move a uneeded operation out of the for loop
>>  - Remove the goto
>> 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..3afc6a7b5b 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 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);
>> +        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])) {
>> +                /* The user specificed CPU is in the valid field, we are
>> +                 * good to go.
>> +                 */
>> +                break;
>> +            }
>> +        }
>> +
>> +        if (!machine_class->valid_cpu_types[i]) {
>> +            /* The user specified CPU must not be a valid CPU */
>> +            error_report("Invalid CPU type: %s", machine->cpu_type);
>> +            error_printf("The valid types 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);
>> +        }
>> +    }
>> +
>>      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

Re: [Qemu-devel] [PATCH v1 2/5] netduino2: Specify the valid CPUs

[Eduardo Habkost]

On Tue, Oct 03, 2017 at 01:05:11PM -0700, Alistair Francis wrote:
> 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>

-- 
Eduardo

Re: [Qemu-devel] [PATCH v1 1/5] machine: Add a valid_cpu_types property

[Eduardo Habkost]

On Tue, Oct 03, 2017 at 01:26:53PM -0700, Alistair Francis wrote:
> On Tue, Oct 3, 2017 at 1:23 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Tue, Oct 03, 2017 at 01:05:09PM -0700, Alistair Francis wrote:
> >> This patch add a MachineClass element that can be set in the machine C
> >> code to specify a list of supported CPU types. If the supported CPU
> >> types are specified the user enter CPU (by -cpu at runtime) is checked
> >> against the supported types and QEMU exits if they aren't supported.
> >>
> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >> ---
> >
> > Thanks!
> >
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> >
> > However, I will squash the following changes before queueing,
> > because:
> >
> > * object_class_dynamic_cast() is safe even if class is NULL,
> >   so there's no need to validate cpu_type here.
> > * "must not be valid" sounds like the CPU is not allowed to be a
> >   valid CPU, so I rewrote the comment.
> >
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 3afc6a7b5b..36c2fb069c 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -766,9 +766,6 @@ void machine_run_board_init(MachineState *machine)
> >          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])) {
> > @@ -780,7 +777,7 @@ void machine_run_board_init(MachineState *machine)
> >          }
> >
> >          if (!machine_class->valid_cpu_types[i]) {
> > -            /* The user specified CPU must not be a valid CPU */
> > +            /* The user specified CPU is not valid */
> >              error_report("Invalid CPU type: %s", machine->cpu_type);
> >              error_printf("The valid types are: %s",
> >                           machine_class->valid_cpu_types[0]);
> 
> Looks good to me.
> 
> Does that mean you are taking the whole series now?

I was planning to tacke only patch 1/5, but I can take the whole
series if I get an Acked-by from the corresponding maintainers.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zcu102: Specify the valid CPUs

[Eduardo Habkost]

On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:
> List all possible valid CPU options.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> 
>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
>  include/hw/arm/xlnx-zynqmp.h |  1 +
>  3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> index 519a16ed98..039649e522 100644
> --- a/hw/arm/xlnx-zcu102.c
> +++ b/hw/arm/xlnx-zcu102.c
> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
>                                &error_abort);
>  
> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> +                            &error_fatal);

Do you have plans to support other CPU types to xlnx_zynqmp in
the future?  If not, I wouldn't bother adding the cpu-type
property and the extra boilerplate code if it's always going to
be set to cortex-a53.


>      object_property_set_link(OBJECT(&s->soc), OBJECT(&s->ddr_ram),
>                           "ddr-ram", &error_abort);
>      object_property_set_bool(OBJECT(&s->soc), s->secure, "secure",
> @@ -160,6 +162,10 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
>      arm_load_kernel(s->soc.boot_cpu_ptr, &xlnx_zcu102_binfo);
>  }
>  
> +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);
> @@ -185,6 +191,8 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data)
>      mc->block_default_type = IF_IDE;
>      mc->units_per_default_bus = 1;
>      mc->ignore_memory_transaction_failures = true;
> +    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> +    mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
>  }
>  
>  static const TypeInfo xlnx_ep108_machine_init_typeinfo = {
> @@ -240,6 +248,8 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data)
>      mc->block_default_type = IF_IDE;
>      mc->units_per_default_bus = 1;
>      mc->ignore_memory_transaction_failures = true;
> +    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> +    mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
>  }
>  
>  static const TypeInfo xlnx_zcu102_machine_init_typeinfo = {
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 2b27daf51d..1bff099ec1 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -133,13 +133,6 @@ static void xlnx_zynqmp_init(Object *obj)
>      XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
>      int i;
>  
> -    for (i = 0; i < XLNX_ZYNQMP_NUM_APU_CPUS; i++) {
> -        object_initialize(&s->apu_cpu[i], sizeof(s->apu_cpu[i]),
> -                          "cortex-a53-" TYPE_ARM_CPU);
> -        object_property_add_child(obj, "apu-cpu[*]", OBJECT(&s->apu_cpu[i]),
> -                                  &error_abort);
> -    }
> -
>      object_initialize(&s->gic, sizeof(s->gic), gic_class_name());
>      qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
>  
> @@ -187,6 +180,14 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>      qemu_irq gic_spi[GIC_NUM_SPI_INTR];
>      Error *err = NULL;
>  
> +    /* We need to do this here to ensure the cpu_type property is set. */
> +    for (i = 0; i < XLNX_ZYNQMP_NUM_APU_CPUS; i++) {
> +        object_initialize(&s->apu_cpu[i], sizeof(s->apu_cpu[i]),
> +                          s->cpu_type);
> +        object_property_add_child(OBJECT(dev), "apu-cpu[*]", OBJECT(&s->apu_cpu[i]),
> +                                  &error_abort);
> +    }
> +
>      ram_size = memory_region_size(s->ddr_ram);
>  
>      /* Create the DDR Memory Regions. User friendly checks should happen at
> @@ -425,6 +426,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>  }
>  
>  static Property xlnx_zynqmp_props[] = {
> +    DEFINE_PROP_STRING("cpu-type", XlnxZynqMPState, cpu_type),
>      DEFINE_PROP_STRING("boot-cpu", XlnxZynqMPState, boot_cpu),
>      DEFINE_PROP_BOOL("secure", XlnxZynqMPState, secure, false),
>      DEFINE_PROP_BOOL("virtualization", XlnxZynqMPState, virt, false),
> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> index 6eff81a995..5afb8de11e 100644
> --- a/include/hw/arm/xlnx-zynqmp.h
> +++ b/include/hw/arm/xlnx-zynqmp.h
> @@ -86,6 +86,7 @@ typedef struct XlnxZynqMPState {
>      XlnxDPState dp;
>      XlnxDPDMAState dpdma;
>  
> +    char *cpu_type;
>      char *boot_cpu;
>      ARMCPU *boot_cpu_ptr;
>  
> -- 
> 2.11.0
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v1 5/5] raspi: : Specify the valid CPUs

[Eduardo Habkost]

On Tue, Oct 03, 2017 at 01:05:18PM -0700, Alistair Francis wrote:
> List all possible valid CPU options.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> 
>  hw/arm/raspi.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 5941c9f751..555db0f258 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -158,6 +158,10 @@ static void raspi2_init(MachineState *machine)
>      setup_boot(machine, 2, machine->ram_size - vcram_size);
>  }
>  
> +const char *raspi2_valid_cpus[] = { ARM_CPU_TYPE_NAME("cortex-a7"),
> +                                    NULL
> +                                  };
> +
>  static void raspi2_machine_init(MachineClass *mc)
>  {
>      mc->desc = "Raspberry Pi 2";
> @@ -169,5 +173,7 @@ static void raspi2_machine_init(MachineClass *mc)
>      mc->max_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;

I'm confused: bcm2836_init() is hardcoded to cortex-a15, not
cortex-a7.

>  };
>  DEFINE_MACHINE("raspi2", raspi2_machine_init)
> -- 
> 2.11.0
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v1 5/5] raspi: : Specify the valid CPUs

[Alistair Francis]

On Tue, Oct 3, 2017 at 1:39 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Oct 03, 2017 at 01:05:18PM -0700, Alistair Francis wrote:
>> List all possible valid CPU options.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>>  hw/arm/raspi.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
>> index 5941c9f751..555db0f258 100644
>> --- a/hw/arm/raspi.c
>> +++ b/hw/arm/raspi.c
>> @@ -158,6 +158,10 @@ static void raspi2_init(MachineState *machine)
>>      setup_boot(machine, 2, machine->ram_size - vcram_size);
>>  }
>>
>> +const char *raspi2_valid_cpus[] = { ARM_CPU_TYPE_NAME("cortex-a7"),
>> +                                    NULL
>> +                                  };
>> +
>>  static void raspi2_machine_init(MachineClass *mc)
>>  {
>>      mc->desc = "Raspberry Pi 2";
>> @@ -169,5 +173,7 @@ static void raspi2_machine_init(MachineClass *mc)
>>      mc->max_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;
>
> I'm confused: bcm2836_init() is hardcoded to cortex-a15, not
> cortex-a7.

Odd. I just looked up the Raspberry Pi 2 and it says a Cortex-A7:
https://www.raspberrypi.org/products/raspberry-pi-2-model-b/

Thanks,
Alistair

>
>>  };
>>  DEFINE_MACHINE("raspi2", raspi2_machine_init)
>> --
>> 2.11.0
>>
>
> --
> Eduardo

Re: [Qemu-devel] [PATCH v1 1/5] machine: Add a valid_cpu_types property

[Alistair Francis]

On Tue, Oct 3, 2017 at 1:33 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Oct 03, 2017 at 01:26:53PM -0700, Alistair Francis wrote:
>> On Tue, Oct 3, 2017 at 1:23 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > On Tue, Oct 03, 2017 at 01:05:09PM -0700, Alistair Francis wrote:
>> >> This patch add a MachineClass element that can be set in the machine C
>> >> code to specify a list of supported CPU types. If the supported CPU
>> >> types are specified the user enter CPU (by -cpu at runtime) is checked
>> >> against the supported types and QEMU exits if they aren't supported.
>> >>
>> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> >> ---
>> >
>> > Thanks!
>> >
>> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> >
>> > However, I will squash the following changes before queueing,
>> > because:
>> >
>> > * object_class_dynamic_cast() is safe even if class is NULL,
>> >   so there's no need to validate cpu_type here.
>> > * "must not be valid" sounds like the CPU is not allowed to be a
>> >   valid CPU, so I rewrote the comment.
>> >
>> >
>> > diff --git a/hw/core/machine.c b/hw/core/machine.c
>> > index 3afc6a7b5b..36c2fb069c 100644
>> > --- a/hw/core/machine.c
>> > +++ b/hw/core/machine.c
>> > @@ -766,9 +766,6 @@ void machine_run_board_init(MachineState *machine)
>> >          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])) {
>> > @@ -780,7 +777,7 @@ void machine_run_board_init(MachineState *machine)
>> >          }
>> >
>> >          if (!machine_class->valid_cpu_types[i]) {
>> > -            /* The user specified CPU must not be a valid CPU */
>> > +            /* The user specified CPU is not valid */
>> >              error_report("Invalid CPU type: %s", machine->cpu_type);
>> >              error_printf("The valid types are: %s",
>> >                           machine_class->valid_cpu_types[0]);
>>
>> Looks good to me.
>>
>> Does that mean you are taking the whole series now?
>
> I was planning to tacke only patch 1/5, but I can take the whole
> series if I get an Acked-by from the corresponding maintainers.

Most of them are maintained by me. Just getting patch 1 in is the most
important part though (then others can use it). So if you want to just
take it by itself that's fine with me.

Thanks,
Alistair

>
> --
> Eduardo
>

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zcu102: Specify the valid CPUs

[Alistair Francis]

On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:
>> List all possible valid CPU options.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
>>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
>>  include/hw/arm/xlnx-zynqmp.h |  1 +
>>  3 files changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
>> index 519a16ed98..039649e522 100644
>> --- a/hw/arm/xlnx-zcu102.c
>> +++ b/hw/arm/xlnx-zcu102.c
>> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
>>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
>>                                &error_abort);
>>
>> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
>> +                            &error_fatal);
>
> Do you have plans to support other CPU types to xlnx_zynqmp in
> the future?  If not, I wouldn't bother adding the cpu-type
> property and the extra boilerplate code if it's always going to
> be set to cortex-a53.

No, it'll always be A53.

I did think of that, but I also wanted to use the new option! I also
think there is an advantage in sanely handling users '-cpu' option,
before now we just ignored it, so I think it still does give a
benefit. That'll be especially important on the Xilinx tree (sometimes
people use our machines with a different CPU to 'benchmark' or test
other CPUs with our CoSimulation setup). So I think it does make sense
to keep in.

Thanks,
Alistair

>
>
>>      object_property_set_link(OBJECT(&s->soc), OBJECT(&s->ddr_ram),
>>                           "ddr-ram", &error_abort);
>>      object_property_set_bool(OBJECT(&s->soc), s->secure, "secure",
>> @@ -160,6 +162,10 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
>>      arm_load_kernel(s->soc.boot_cpu_ptr, &xlnx_zcu102_binfo);
>>  }
>>
>> +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);
>> @@ -185,6 +191,8 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data)
>>      mc->block_default_type = IF_IDE;
>>      mc->units_per_default_bus = 1;
>>      mc->ignore_memory_transaction_failures = true;
>> +    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
>> +    mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
>>  }
>>
>>  static const TypeInfo xlnx_ep108_machine_init_typeinfo = {
>> @@ -240,6 +248,8 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data)
>>      mc->block_default_type = IF_IDE;
>>      mc->units_per_default_bus = 1;
>>      mc->ignore_memory_transaction_failures = true;
>> +    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
>> +    mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
>>  }
>>
>>  static const TypeInfo xlnx_zcu102_machine_init_typeinfo = {
>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
>> index 2b27daf51d..1bff099ec1 100644
>> --- a/hw/arm/xlnx-zynqmp.c
>> +++ b/hw/arm/xlnx-zynqmp.c
>> @@ -133,13 +133,6 @@ static void xlnx_zynqmp_init(Object *obj)
>>      XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
>>      int i;
>>
>> -    for (i = 0; i < XLNX_ZYNQMP_NUM_APU_CPUS; i++) {
>> -        object_initialize(&s->apu_cpu[i], sizeof(s->apu_cpu[i]),
>> -                          "cortex-a53-" TYPE_ARM_CPU);
>> -        object_property_add_child(obj, "apu-cpu[*]", OBJECT(&s->apu_cpu[i]),
>> -                                  &error_abort);
>> -    }
>> -
>>      object_initialize(&s->gic, sizeof(s->gic), gic_class_name());
>>      qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
>>
>> @@ -187,6 +180,14 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>>      qemu_irq gic_spi[GIC_NUM_SPI_INTR];
>>      Error *err = NULL;
>>
>> +    /* We need to do this here to ensure the cpu_type property is set. */
>> +    for (i = 0; i < XLNX_ZYNQMP_NUM_APU_CPUS; i++) {
>> +        object_initialize(&s->apu_cpu[i], sizeof(s->apu_cpu[i]),
>> +                          s->cpu_type);
>> +        object_property_add_child(OBJECT(dev), "apu-cpu[*]", OBJECT(&s->apu_cpu[i]),
>> +                                  &error_abort);
>> +    }
>> +
>>      ram_size = memory_region_size(s->ddr_ram);
>>
>>      /* Create the DDR Memory Regions. User friendly checks should happen at
>> @@ -425,6 +426,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>>  }
>>
>>  static Property xlnx_zynqmp_props[] = {
>> +    DEFINE_PROP_STRING("cpu-type", XlnxZynqMPState, cpu_type),
>>      DEFINE_PROP_STRING("boot-cpu", XlnxZynqMPState, boot_cpu),
>>      DEFINE_PROP_BOOL("secure", XlnxZynqMPState, secure, false),
>>      DEFINE_PROP_BOOL("virtualization", XlnxZynqMPState, virt, false),
>> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
>> index 6eff81a995..5afb8de11e 100644
>> --- a/include/hw/arm/xlnx-zynqmp.h
>> +++ b/include/hw/arm/xlnx-zynqmp.h
>> @@ -86,6 +86,7 @@ typedef struct XlnxZynqMPState {
>>      XlnxDPState dp;
>>      XlnxDPDMAState dpdma;
>>
>> +    char *cpu_type;
>>      char *boot_cpu;
>>      ARMCPU *boot_cpu_ptr;
>>
>> --
>> 2.11.0
>>
>
> --
> Eduardo

Re: [Qemu-devel] [PATCH v1 2/5] netduino2: Specify the valid CPUs

[Philippe Mathieu-Daudé]

On 10/03/2017 05:05 PM, Alistair Francis wrote:
> 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: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
> 
> RFC v2:
>  - Use a NULL terminated list
>  - Add the Cortex-M4 for testing
> 
> 
>  hw/arm/netduino2.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> index f936017d4a..b68ecf2c08 100644
> --- a/hw/arm/netduino2.c
> +++ b/hw/arm/netduino2.c
> @@ -34,18 +34,25 @@ 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)
> 

Re: [Qemu-devel] [PATCH v1 4/5] xilinx_zynq: : Specify the valid CPUs

[Philippe Mathieu-Daudé]

On 10/03/2017 05:05 PM, Alistair Francis wrote:
> List all possible valid CPU options.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
> 
>  hw/arm/xilinx_zynq.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 1836a4ed45..de1e0bbce1 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -313,6 +313,10 @@ static void zynq_init(MachineState *machine)
>      arm_load_kernel(ARM_CPU(first_cpu), &zynq_binfo);
>  }
>  
> +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 +325,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)
> 

Re: [Qemu-devel] [PATCH v1 5/5] raspi: : Specify the valid CPUs

[Philippe Mathieu-Daudé]

On 10/03/2017 06:36 PM, Alistair Francis wrote:
> On Tue, Oct 3, 2017 at 1:39 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> On Tue, Oct 03, 2017 at 01:05:18PM -0700, Alistair Francis wrote:
>>> List all possible valid CPU options.
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> ---
>>>
>>>  hw/arm/raspi.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
>>> index 5941c9f751..555db0f258 100644
>>> --- a/hw/arm/raspi.c
>>> +++ b/hw/arm/raspi.c
>>> @@ -158,6 +158,10 @@ static void raspi2_init(MachineState *machine)
>>>      setup_boot(machine, 2, machine->ram_size - vcram_size);
>>>  }
>>>
>>> +const char *raspi2_valid_cpus[] = { ARM_CPU_TYPE_NAME("cortex-a7"),
>>> +                                    NULL
>>> +                                  };
>>> +
>>>  static void raspi2_machine_init(MachineClass *mc)
>>>  {
>>>      mc->desc = "Raspberry Pi 2";
>>> @@ -169,5 +173,7 @@ static void raspi2_machine_init(MachineClass *mc)
>>>      mc->max_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;
>>
>> I'm confused: bcm2836_init() is hardcoded to cortex-a15, not
>> cortex-a7.
> 
> Odd. I just looked up the Raspberry Pi 2 and it says a Cortex-A7:
> https://www.raspberrypi.org/products/raspberry-pi-2-model-b/

The BCM2836 SoC definitively is Cortex-A7.

git history says the A7 was added after (dcf578ed8cec) the raspi2 board
(bad5623690b1).

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> Thanks,
> Alistair
> 
>>
>>>  };
>>>  DEFINE_MACHINE("raspi2", raspi2_machine_init)
>>> --
>>> 2.11.0
>>>
>>
>> --
>> Eduardo

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zcu102: Specify the valid CPUs

[Eduardo Habkost]

On Tue, Oct 03, 2017 at 02:41:17PM -0700, Alistair Francis wrote:
> On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:
> >> List all possible valid CPU options.
> >>
> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >> ---
> >>
> >>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
> >>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
> >>  include/hw/arm/xlnx-zynqmp.h |  1 +
> >>  3 files changed, 20 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> >> index 519a16ed98..039649e522 100644
> >> --- a/hw/arm/xlnx-zcu102.c
> >> +++ b/hw/arm/xlnx-zcu102.c
> >> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
> >>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> >>                                &error_abort);
> >>
> >> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> >> +                            &error_fatal);
> >
> > Do you have plans to support other CPU types to xlnx_zynqmp in
> > the future?  If not, I wouldn't bother adding the cpu-type
> > property and the extra boilerplate code if it's always going to
> > be set to cortex-a53.
> 
> No, it'll always be A53.
> 
> I did think of that, but I also wanted to use the new option! I also
> think there is an advantage in sanely handling users '-cpu' option,
> before now we just ignored it, so I think it still does give a
> benefit. That'll be especially important on the Xilinx tree (sometimes
> people use our machines with a different CPU to 'benchmark' or test
> other CPUs with our CoSimulation setup). So I think it does make sense
> to keep in.

I see.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH v1 5/5] raspi: : Specify the valid CPUs

[Eduardo Habkost]

On Tue, Oct 03, 2017 at 07:18:44PM -0300, Philippe Mathieu-Daudé wrote:
> On 10/03/2017 06:36 PM, Alistair Francis wrote:
> > On Tue, Oct 3, 2017 at 1:39 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> On Tue, Oct 03, 2017 at 01:05:18PM -0700, Alistair Francis wrote:
> >>> List all possible valid CPU options.
> >>>
> >>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >>> ---
> >>>
> >>>  hw/arm/raspi.c | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> >>> index 5941c9f751..555db0f258 100644
> >>> --- a/hw/arm/raspi.c
> >>> +++ b/hw/arm/raspi.c
> >>> @@ -158,6 +158,10 @@ static void raspi2_init(MachineState *machine)
> >>>      setup_boot(machine, 2, machine->ram_size - vcram_size);
> >>>  }
> >>>
> >>> +const char *raspi2_valid_cpus[] = { ARM_CPU_TYPE_NAME("cortex-a7"),
> >>> +                                    NULL
> >>> +                                  };
> >>> +
> >>>  static void raspi2_machine_init(MachineClass *mc)
> >>>  {
> >>>      mc->desc = "Raspberry Pi 2";
> >>> @@ -169,5 +173,7 @@ static void raspi2_machine_init(MachineClass *mc)
> >>>      mc->max_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;
> >>
> >> I'm confused: bcm2836_init() is hardcoded to cortex-a15, not
> >> cortex-a7.
> > 
> > Odd. I just looked up the Raspberry Pi 2 and it says a Cortex-A7:
> > https://www.raspberrypi.org/products/raspberry-pi-2-model-b/
> 
> The BCM2836 SoC definitively is Cortex-A7.
> 
> git history says the A7 was added after (dcf578ed8cec) the raspi2 board
> (bad5623690b1).

Shouldn't we update TYPE_BCM2836 to use cpu_type instead of
cortex-a15 before applying this patch, then?

> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> > 
> > Thanks,
> > Alistair
> > 
> >>
> >>>  };
> >>>  DEFINE_MACHINE("raspi2", raspi2_machine_init)
> >>> --
> >>> 2.11.0
> >>>
> >>
> >> --
> >> Eduardo

-- 
Eduardo

Re: [Qemu-devel] [PATCH v1 2/5] netduino2: Specify the valid CPUs

[Igor Mammedov]

On Tue, 3 Oct 2017 13:05:11 -0700
Alistair Francis <alistair.francis@xilinx.com> wrote:

> 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>
> ---
> 
> RFC v2:
>  - Use a NULL terminated list
>  - Add the Cortex-M4 for testing
> 
> 
>  hw/arm/netduino2.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> index f936017d4a..b68ecf2c08 100644
> --- a/hw/arm/netduino2.c
> +++ b/hw/arm/netduino2.c
> @@ -34,18 +34,25 @@ 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"),
style nit,                               ^^^ put entries on new line with typical 4 space alignment
> +                                      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)

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zcu102: Specify the valid CPUs

[Igor Mammedov]

On Tue, 3 Oct 2017 14:41:17 -0700
Alistair Francis <alistair.francis@xilinx.com> wrote:

> On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:  
> >> List all possible valid CPU options.
> >>
> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >> ---
> >>
> >>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
> >>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
> >>  include/hw/arm/xlnx-zynqmp.h |  1 +
> >>  3 files changed, 20 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> >> index 519a16ed98..039649e522 100644
> >> --- a/hw/arm/xlnx-zcu102.c
> >> +++ b/hw/arm/xlnx-zcu102.c
> >> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
> >>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> >>                                &error_abort);
> >>
> >> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> >> +                            &error_fatal);  
> >
> > Do you have plans to support other CPU types to xlnx_zynqmp in
> > the future?  If not, I wouldn't bother adding the cpu-type
> > property and the extra boilerplate code if it's always going to
> > be set to cortex-a53.  
> 
> No, it'll always be A53.
> 
> I did think of that, but I also wanted to use the new option! I also
> think there is an advantage in sanely handling users '-cpu' option,
> before now we just ignored it, so I think it still does give a
> benefit. That'll be especially important on the Xilinx tree (sometimes
> people use our machines with a different CPU to 'benchmark' or test
> other CPUs with our CoSimulation setup). So I think it does make sense
> to keep in.
if cpu isn't user settable, one could just outright die if cpu_type
is not NULL and say that user's CLI is wrong.
(i.e. don't give users illusion that they allowed to use '-cpu')

> 
> Thanks,
> Alistair
> 
> >
> >  
> >>      object_property_set_link(OBJECT(&s->soc), OBJECT(&s->ddr_ram),
> >>                           "ddr-ram", &error_abort);
> >>      object_property_set_bool(OBJECT(&s->soc), s->secure, "secure",
> >> @@ -160,6 +162,10 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
> >>      arm_load_kernel(s->soc.boot_cpu_ptr, &xlnx_zcu102_binfo);
> >>  }
> >>
> >> +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);
> >> @@ -185,6 +191,8 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data)
> >>      mc->block_default_type = IF_IDE;
> >>      mc->units_per_default_bus = 1;
> >>      mc->ignore_memory_transaction_failures = true;
> >> +    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> >> +    mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
> >>  }
> >>
> >>  static const TypeInfo xlnx_ep108_machine_init_typeinfo = {
> >> @@ -240,6 +248,8 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data)
> >>      mc->block_default_type = IF_IDE;
> >>      mc->units_per_default_bus = 1;
> >>      mc->ignore_memory_transaction_failures = true;
> >> +    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> >> +    mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
> >>  }
> >>
> >>  static const TypeInfo xlnx_zcu102_machine_init_typeinfo = {
> >> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> >> index 2b27daf51d..1bff099ec1 100644
> >> --- a/hw/arm/xlnx-zynqmp.c
> >> +++ b/hw/arm/xlnx-zynqmp.c
> >> @@ -133,13 +133,6 @@ static void xlnx_zynqmp_init(Object *obj)
> >>      XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
> >>      int i;
> >>
> >> -    for (i = 0; i < XLNX_ZYNQMP_NUM_APU_CPUS; i++) {
> >> -        object_initialize(&s->apu_cpu[i], sizeof(s->apu_cpu[i]),
> >> -                          "cortex-a53-" TYPE_ARM_CPU);
> >> -        object_property_add_child(obj, "apu-cpu[*]", OBJECT(&s->apu_cpu[i]),
> >> -                                  &error_abort);
> >> -    }
> >> -
> >>      object_initialize(&s->gic, sizeof(s->gic), gic_class_name());
> >>      qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
> >>
> >> @@ -187,6 +180,14 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
> >>      qemu_irq gic_spi[GIC_NUM_SPI_INTR];
> >>      Error *err = NULL;
> >>
> >> +    /* We need to do this here to ensure the cpu_type property is set. */
> >> +    for (i = 0; i < XLNX_ZYNQMP_NUM_APU_CPUS; i++) {
> >> +        object_initialize(&s->apu_cpu[i], sizeof(s->apu_cpu[i]),
> >> +                          s->cpu_type);
> >> +        object_property_add_child(OBJECT(dev), "apu-cpu[*]", OBJECT(&s->apu_cpu[i]),
> >> +                                  &error_abort);
> >> +    }
> >> +
> >>      ram_size = memory_region_size(s->ddr_ram);
> >>
> >>      /* Create the DDR Memory Regions. User friendly checks should happen at
> >> @@ -425,6 +426,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
> >>  }
> >>
> >>  static Property xlnx_zynqmp_props[] = {
> >> +    DEFINE_PROP_STRING("cpu-type", XlnxZynqMPState, cpu_type),
> >>      DEFINE_PROP_STRING("boot-cpu", XlnxZynqMPState, boot_cpu),
> >>      DEFINE_PROP_BOOL("secure", XlnxZynqMPState, secure, false),
> >>      DEFINE_PROP_BOOL("virtualization", XlnxZynqMPState, virt, false),
> >> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> >> index 6eff81a995..5afb8de11e 100644
> >> --- a/include/hw/arm/xlnx-zynqmp.h
> >> +++ b/include/hw/arm/xlnx-zynqmp.h
> >> @@ -86,6 +86,7 @@ typedef struct XlnxZynqMPState {
> >>      XlnxDPState dp;
> >>      XlnxDPDMAState dpdma;
> >>
> >> +    char *cpu_type;
> >>      char *boot_cpu;
> >>      ARMCPU *boot_cpu_ptr;
> >>
> >> --
> >> 2.11.0
> >>  
> >
> > --
> > Eduardo  
> 

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zcu102: Specify the valid CPUs

[Eduardo Habkost]

On Wed, Oct 04, 2017 at 01:12:32PM +0200, Igor Mammedov wrote:
> On Tue, 3 Oct 2017 14:41:17 -0700
> Alistair Francis <alistair.francis@xilinx.com> wrote:
> 
> > On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:  
> > >> List all possible valid CPU options.
> > >>
> > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > >> ---
> > >>
> > >>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
> > >>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
> > >>  include/hw/arm/xlnx-zynqmp.h |  1 +
> > >>  3 files changed, 20 insertions(+), 7 deletions(-)
> > >>
> > >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> > >> index 519a16ed98..039649e522 100644
> > >> --- a/hw/arm/xlnx-zcu102.c
> > >> +++ b/hw/arm/xlnx-zcu102.c
> > >> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
> > >>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> > >>                                &error_abort);
> > >>
> > >> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> > >> +                            &error_fatal);  
> > >
> > > Do you have plans to support other CPU types to xlnx_zynqmp in
> > > the future?  If not, I wouldn't bother adding the cpu-type
> > > property and the extra boilerplate code if it's always going to
> > > be set to cortex-a53.  
> > 
> > No, it'll always be A53.
> > 
> > I did think of that, but I also wanted to use the new option! I also
> > think there is an advantage in sanely handling users '-cpu' option,
> > before now we just ignored it, so I think it still does give a
> > benefit. That'll be especially important on the Xilinx tree (sometimes
> > people use our machines with a different CPU to 'benchmark' or test
> > other CPUs with our CoSimulation setup). So I think it does make sense
> > to keep in.
> if cpu isn't user settable, one could just outright die if cpu_type
> is not NULL and say that user's CLI is wrong.
> (i.e. don't give users illusion that they allowed to use '-cpu')

Isn't it exactly what this patch does, by setting:
    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
    mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
?

Except that "-cpu cortex-a53" won't die, which is a good thing.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zcu102: Specify the valid CPUs

[Igor Mammedov]

On Wed, 4 Oct 2017 09:28:51 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Oct 04, 2017 at 01:12:32PM +0200, Igor Mammedov wrote:
> > On Tue, 3 Oct 2017 14:41:17 -0700
> > Alistair Francis <alistair.francis@xilinx.com> wrote:
> >   
> > > On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:  
> > > > On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:    
> > > >> List all possible valid CPU options.
> > > >>
> > > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > > >> ---
> > > >>
> > > >>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
> > > >>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
> > > >>  include/hw/arm/xlnx-zynqmp.h |  1 +
> > > >>  3 files changed, 20 insertions(+), 7 deletions(-)
> > > >>
> > > >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> > > >> index 519a16ed98..039649e522 100644
> > > >> --- a/hw/arm/xlnx-zcu102.c
> > > >> +++ b/hw/arm/xlnx-zcu102.c
> > > >> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
> > > >>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> > > >>                                &error_abort);
> > > >>
> > > >> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> > > >> +                            &error_fatal);    
> > > >
> > > > Do you have plans to support other CPU types to xlnx_zynqmp in
> > > > the future?  If not, I wouldn't bother adding the cpu-type
> > > > property and the extra boilerplate code if it's always going to
> > > > be set to cortex-a53.    
> > > 
> > > No, it'll always be A53.
> > > 
> > > I did think of that, but I also wanted to use the new option! I also
> > > think there is an advantage in sanely handling users '-cpu' option,
> > > before now we just ignored it, so I think it still does give a
> > > benefit. That'll be especially important on the Xilinx tree (sometimes
> > > people use our machines with a different CPU to 'benchmark' or test
> > > other CPUs with our CoSimulation setup). So I think it does make sense
> > > to keep in.  
> > if cpu isn't user settable, one could just outright die if cpu_type
> > is not NULL and say that user's CLI is wrong.
> > (i.e. don't give users illusion that they allowed to use '-cpu')  
> 
> Isn't it exactly what this patch does, by setting:
>     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
>     mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
> ?
> 
> Except that "-cpu cortex-a53" won't die, which is a good thing.
allowing "-cpu cortex-a53" here, would allow to use feature parsing
which weren't allowed or were ignored before if user supplied '-cpu'.
so I'd more strict and refuse any -cpu and break CLI that tries to use it
if board has non configurable cpu type. It would be easier to relax
restriction later if necessary.

using validate_cpus here just to have users for the new code,
doesn't seem like valid justification and at that it makes board
code more complex where it's not necessary and build in cpu type
works just fine.

wrt centralized way to refuse -cpu if board doesn't support it,
(which is not really related to this series) following could be done:

when cpu_model removal is completely done I plan to replace
  vl.c
     cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model)
with
     cpu_parse_cpu_model(DEFAULT_TARGET_CPU_TYPE, cpu_model)

so that we could drop temporary guard

     if (machine_class->default_cpu_type) {

with that it would be possible to tell from machine_run_board_init()
that board doesn't provide cpu but user provided '-cpu'
so we would be able to:
  if ((machine_class->default_cpu_type == NULL) &&
      (machine->cpu_type != NULL))
          error_fatal("machine doesn't support -cpu option");

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zcu102: Specify the valid CPUs

[Eduardo Habkost]

On Wed, Oct 04, 2017 at 03:08:16PM +0200, Igor Mammedov wrote:
> On Wed, 4 Oct 2017 09:28:51 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Oct 04, 2017 at 01:12:32PM +0200, Igor Mammedov wrote:
> > > On Tue, 3 Oct 2017 14:41:17 -0700
> > > Alistair Francis <alistair.francis@xilinx.com> wrote:
> > >   
> > > > On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:  
> > > > > On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:    
> > > > >> List all possible valid CPU options.
> > > > >>
> > > > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > > > >> ---
> > > > >>
> > > > >>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
> > > > >>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
> > > > >>  include/hw/arm/xlnx-zynqmp.h |  1 +
> > > > >>  3 files changed, 20 insertions(+), 7 deletions(-)
> > > > >>
> > > > >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> > > > >> index 519a16ed98..039649e522 100644
> > > > >> --- a/hw/arm/xlnx-zcu102.c
> > > > >> +++ b/hw/arm/xlnx-zcu102.c
> > > > >> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
> > > > >>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> > > > >>                                &error_abort);
> > > > >>
> > > > >> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> > > > >> +                            &error_fatal);    
> > > > >
> > > > > Do you have plans to support other CPU types to xlnx_zynqmp in
> > > > > the future?  If not, I wouldn't bother adding the cpu-type
> > > > > property and the extra boilerplate code if it's always going to
> > > > > be set to cortex-a53.    
> > > > 
> > > > No, it'll always be A53.
> > > > 
> > > > I did think of that, but I also wanted to use the new option! I also
> > > > think there is an advantage in sanely handling users '-cpu' option,
> > > > before now we just ignored it, so I think it still does give a
> > > > benefit. That'll be especially important on the Xilinx tree (sometimes
> > > > people use our machines with a different CPU to 'benchmark' or test
> > > > other CPUs with our CoSimulation setup). So I think it does make sense
> > > > to keep in.  
> > > if cpu isn't user settable, one could just outright die if cpu_type
> > > is not NULL and say that user's CLI is wrong.
> > > (i.e. don't give users illusion that they allowed to use '-cpu')  
> > 
> > Isn't it exactly what this patch does, by setting:
> >     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> >     mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
> > ?
> > 
> > Except that "-cpu cortex-a53" won't die, which is a good thing.
> allowing "-cpu cortex-a53" here, would allow to use feature parsing
> which weren't allowed or were ignored before if user supplied '-cpu'.
> so I'd more strict and refuse any -cpu and break CLI that tries to use it
> if board has non configurable cpu type. It would be easier to relax
> restriction later if necessary.
> 
> using validate_cpus here just to have users for the new code,
> doesn't seem like valid justification and at that it makes board
> code more complex where it's not necessary and build in cpu type
> works just fine.

It's up to the board maintainer to decide what's the best option.
Both features are independent from each other and can be
implemented by machine core.

In either case, the valid_cpu_types feature will be still very
useful for boards like pxa270 and sa1110, which support -cpu but
only with specific families of CPU types (grep for
"strncmp(cpu_type").

> 
> wrt centralized way to refuse -cpu if board doesn't support it,
> (which is not really related to this series) following could be done:
> 
> when cpu_model removal is completely done I plan to replace
>   vl.c
>      cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model)
> with
>      cpu_parse_cpu_model(DEFAULT_TARGET_CPU_TYPE, cpu_model)
> 
> so that we could drop temporary guard
> 
>      if (machine_class->default_cpu_type) {

This sounds good to me, even if we don't reject -cpu on any
board.


> 
> with that it would be possible to tell from machine_run_board_init()
> that board doesn't provide cpu but user provided '-cpu'
> so we would be able to:
>   if ((machine_class->default_cpu_type == NULL) &&
>       (machine->cpu_type != NULL))
>           error_fatal("machine doesn't support -cpu option");

I won't complain too much if a board maintainer really wants to
make the board reject -cpu completely, but it's up to them.

Personally, I'd prefer to have all boards setting
default_cpu_type even if they support only one CPU model, so
clients don't need a special case for boards that don't support
-cpu.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zcu102: Specify the valid CPUs

[Alistair Francis]

On Wed, Oct 4, 2017 at 9:34 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Wed, Oct 04, 2017 at 03:08:16PM +0200, Igor Mammedov wrote:
>> On Wed, 4 Oct 2017 09:28:51 -0300
>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>
>> > On Wed, Oct 04, 2017 at 01:12:32PM +0200, Igor Mammedov wrote:
>> > > On Tue, 3 Oct 2017 14:41:17 -0700
>> > > Alistair Francis <alistair.francis@xilinx.com> wrote:
>> > >
>> > > > On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > > > > On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:
>> > > > >> List all possible valid CPU options.
>> > > > >>
>> > > > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> > > > >> ---
>> > > > >>
>> > > > >>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
>> > > > >>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
>> > > > >>  include/hw/arm/xlnx-zynqmp.h |  1 +
>> > > > >>  3 files changed, 20 insertions(+), 7 deletions(-)
>> > > > >>
>> > > > >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
>> > > > >> index 519a16ed98..039649e522 100644
>> > > > >> --- a/hw/arm/xlnx-zcu102.c
>> > > > >> +++ b/hw/arm/xlnx-zcu102.c
>> > > > >> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
>> > > > >>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
>> > > > >>                                &error_abort);
>> > > > >>
>> > > > >> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
>> > > > >> +                            &error_fatal);
>> > > > >
>> > > > > Do you have plans to support other CPU types to xlnx_zynqmp in
>> > > > > the future?  If not, I wouldn't bother adding the cpu-type
>> > > > > property and the extra boilerplate code if it's always going to
>> > > > > be set to cortex-a53.
>> > > >
>> > > > No, it'll always be A53.
>> > > >
>> > > > I did think of that, but I also wanted to use the new option! I also
>> > > > think there is an advantage in sanely handling users '-cpu' option,
>> > > > before now we just ignored it, so I think it still does give a
>> > > > benefit. That'll be especially important on the Xilinx tree (sometimes
>> > > > people use our machines with a different CPU to 'benchmark' or test
>> > > > other CPUs with our CoSimulation setup). So I think it does make sense
>> > > > to keep in.
>> > > if cpu isn't user settable, one could just outright die if cpu_type
>> > > is not NULL and say that user's CLI is wrong.
>> > > (i.e. don't give users illusion that they allowed to use '-cpu')
>> >
>> > Isn't it exactly what this patch does, by setting:
>> >     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
>> >     mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
>> > ?
>> >
>> > Except that "-cpu cortex-a53" won't die, which is a good thing.
>> allowing "-cpu cortex-a53" here, would allow to use feature parsing
>> which weren't allowed or were ignored before if user supplied '-cpu'.
>> so I'd more strict and refuse any -cpu and break CLI that tries to use it
>> if board has non configurable cpu type. It would be easier to relax
>> restriction later if necessary.
>>
>> using validate_cpus here just to have users for the new code,
>> doesn't seem like valid justification and at that it makes board
>> code more complex where it's not necessary and build in cpu type
>> works just fine.
>
> It's up to the board maintainer to decide what's the best option.
> Both features are independent from each other and can be
> implemented by machine core.

Noooo!

My hope with this series is that eventually we could hit a state where
every single machine acts the same way with the -cpu option.

I really don't like what we do now where some boards use it, some
boards error and some boars just ignore the option. I think we should
agree on something and every machine should follow the same flow so
that users know what to expect when they use the -cpu option.

If this means we allow machines to specify they don't support the
option or only have a single element in the list of supported options
doesn't really matter, but all machines should do the same thing.

>
> In either case, the valid_cpu_types feature will be still very
> useful for boards like pxa270 and sa1110, which support -cpu but
> only with specific families of CPU types (grep for
> "strncmp(cpu_type").
>
>>
>> wrt centralized way to refuse -cpu if board doesn't support it,
>> (which is not really related to this series) following could be done:
>>
>> when cpu_model removal is completely done I plan to replace
>>   vl.c
>>      cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model)
>> with
>>      cpu_parse_cpu_model(DEFAULT_TARGET_CPU_TYPE, cpu_model)
>>
>> so that we could drop temporary guard
>>
>>      if (machine_class->default_cpu_type) {
>
> This sounds good to me, even if we don't reject -cpu on any
> board.
>
>
>>
>> with that it would be possible to tell from machine_run_board_init()
>> that board doesn't provide cpu but user provided '-cpu'
>> so we would be able to:
>>   if ((machine_class->default_cpu_type == NULL) &&
>>       (machine->cpu_type != NULL))
>>           error_fatal("machine doesn't support -cpu option");
>
> I won't complain too much if a board maintainer really wants to
> make the board reject -cpu completely, but it's up to them.

I disagree. I think a standard way of doing it is better. At least for
each architecture. The ARM -cpu option is very confusing at the moment
and it really doesn't need to be that bad.

>
> Personally, I'd prefer to have all boards setting
> default_cpu_type even if they support only one CPU model, so
> clients don't need a special case for boards that don't support
> -cpu.

I agree, I think having one CPU makes more sense. It makes it easier
to add support for more cpus in the future and allows the users to use
the -cpu option without killing QEMU.

Thanks,
Alistair

>
> --
> Eduardo
>

Re: [Qemu-devel] [PATCH v1 2/5] netduino2: Specify the valid CPUs

[Alistair Francis]

On Wed, Oct 4, 2017 at 4:02 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Tue, 3 Oct 2017 13:05:11 -0700
> Alistair Francis <alistair.francis@xilinx.com> wrote:
>
>> 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>
>> ---
>>
>> RFC v2:
>>  - Use a NULL terminated list
>>  - Add the Cortex-M4 for testing
>>
>>
>>  hw/arm/netduino2.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
>> index f936017d4a..b68ecf2c08 100644
>> --- a/hw/arm/netduino2.c
>> +++ b/hw/arm/netduino2.c
>> @@ -34,18 +34,25 @@ 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"),
> style nit,                               ^^^ put entries on new line with typical 4 space alignment

Do you mean like this?

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

Thanks,
Alistair

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

Re: [Qemu-devel] [PATCH v1 2/5] netduino2: Specify the valid CPUs

[Philippe Mathieu-Daudé]

>>> +const char *netduino_valid_cpus[] = { ARM_CPU_TYPE_NAME("cortex-m3"),
>> style nit,                               ^^^ put entries on new line with typical 4 space alignment
> 
> Do you mean like this?
> 
> const char *netduino_valid_cpus[] = {
>                                     ARM_CPU_TYPE_NAME("cortex-m3"),
>                                     ARM_CPU_TYPE_NAME("cortex-m4"),
>                                     NULL };

I suppose he meant:

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

Re: [Qemu-devel] [PATCH v1 2/5] netduino2: Specify the valid CPUs

[Igor Mammedov]

On Wed, 4 Oct 2017 19:21:09 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> >>> +const char *netduino_valid_cpus[] = { ARM_CPU_TYPE_NAME("cortex-m3"),  
> >> style nit,                               ^^^ put entries on new line with typical 4 space alignment  
> > 
> > Do you mean like this?
> > 
> > const char *netduino_valid_cpus[] = {
> >                                     ARM_CPU_TYPE_NAME("cortex-m3"),
> >                                     ARM_CPU_TYPE_NAME("cortex-m4"),
> >                                     NULL };  
> 
> I suppose he meant:
> 
> static const char *netduino_valid_cpus[] = {
>     ARM_CPU_TYPE_NAME("cortex-m3"),
>     ARM_CPU_TYPE_NAME("cortex-m4"),
>     NULL
> };

Thanks,
that's exactly what I've meant.

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zcu102: Specify the valid CPUs

[Igor Mammedov]

On Wed, 4 Oct 2017 14:39:20 -0700
Alistair Francis <alistair.francis@xilinx.com> wrote:

> On Wed, Oct 4, 2017 at 9:34 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Wed, Oct 04, 2017 at 03:08:16PM +0200, Igor Mammedov wrote:  
> >> On Wed, 4 Oct 2017 09:28:51 -0300
> >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>  
> >> > On Wed, Oct 04, 2017 at 01:12:32PM +0200, Igor Mammedov wrote:  
> >> > > On Tue, 3 Oct 2017 14:41:17 -0700
> >> > > Alistair Francis <alistair.francis@xilinx.com> wrote:
> >> > >  
> >> > > > On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:  
> >> > > > > On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:  
> >> > > > >> List all possible valid CPU options.
> >> > > > >>
> >> > > > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >> > > > >> ---
> >> > > > >>
> >> > > > >>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
> >> > > > >>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
> >> > > > >>  include/hw/arm/xlnx-zynqmp.h |  1 +
> >> > > > >>  3 files changed, 20 insertions(+), 7 deletions(-)
> >> > > > >>
> >> > > > >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> >> > > > >> index 519a16ed98..039649e522 100644
> >> > > > >> --- a/hw/arm/xlnx-zcu102.c
> >> > > > >> +++ b/hw/arm/xlnx-zcu102.c
> >> > > > >> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
> >> > > > >>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> >> > > > >>                                &error_abort);
> >> > > > >>
> >> > > > >> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> >> > > > >> +                            &error_fatal);  
> >> > > > >
> >> > > > > Do you have plans to support other CPU types to xlnx_zynqmp in
> >> > > > > the future?  If not, I wouldn't bother adding the cpu-type
> >> > > > > property and the extra boilerplate code if it's always going to
> >> > > > > be set to cortex-a53.  
> >> > > >
> >> > > > No, it'll always be A53.
> >> > > >
> >> > > > I did think of that, but I also wanted to use the new option! I also
> >> > > > think there is an advantage in sanely handling users '-cpu' option,
> >> > > > before now we just ignored it, so I think it still does give a
> >> > > > benefit. That'll be especially important on the Xilinx tree (sometimes
> >> > > > people use our machines with a different CPU to 'benchmark' or test
> >> > > > other CPUs with our CoSimulation setup). So I think it does make sense
> >> > > > to keep in.  
> >> > > if cpu isn't user settable, one could just outright die if cpu_type
> >> > > is not NULL and say that user's CLI is wrong.
> >> > > (i.e. don't give users illusion that they allowed to use '-cpu')  
> >> >
> >> > Isn't it exactly what this patch does, by setting:
> >> >     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> >> >     mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
> >> > ?
> >> >
> >> > Except that "-cpu cortex-a53" won't die, which is a good thing.  
> >> allowing "-cpu cortex-a53" here, would allow to use feature parsing
> >> which weren't allowed or were ignored before if user supplied '-cpu'.
> >> so I'd more strict and refuse any -cpu and break CLI that tries to use it
> >> if board has non configurable cpu type. It would be easier to relax
> >> restriction later if necessary.
> >>
> >> using validate_cpus here just to have users for the new code,
> >> doesn't seem like valid justification and at that it makes board
> >> code more complex where it's not necessary and build in cpu type
> >> works just fine.  
> >
> > It's up to the board maintainer to decide what's the best option.
> > Both features are independent from each other and can be
> > implemented by machine core.  
> 
> Noooo!
> 
> My hope with this series is that eventually we could hit a state where
> every single machine acts the same way with the -cpu option.
> 
> I really don't like what we do now where some boards use it, some
> boards error and some boars just ignore the option. I think we should
> agree on something and every machine should follow the same flow so
> that users know what to expect when they use the -cpu option.
> 
> If this means we allow machines to specify they don't support the
> option or only have a single element in the list of supported options
> doesn't really matter, but all machines should do the same thing.
> 
> >
> > In either case, the valid_cpu_types feature will be still very
> > useful for boards like pxa270 and sa1110, which support -cpu but
> > only with specific families of CPU types (grep for
> > "strncmp(cpu_type").
> >  
> >>
> >> wrt centralized way to refuse -cpu if board doesn't support it,
> >> (which is not really related to this series) following could be done:
> >>
> >> when cpu_model removal is completely done I plan to replace
> >>   vl.c
> >>      cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model)
> >> with
> >>      cpu_parse_cpu_model(DEFAULT_TARGET_CPU_TYPE, cpu_model)
> >>
> >> so that we could drop temporary guard
> >>
> >>      if (machine_class->default_cpu_type) {  
> >
> > This sounds good to me, even if we don't reject -cpu on any
> > board.
> >
> >  
> >>
> >> with that it would be possible to tell from machine_run_board_init()
> >> that board doesn't provide cpu but user provided '-cpu'
> >> so we would be able to:
> >>   if ((machine_class->default_cpu_type == NULL) &&
> >>       (machine->cpu_type != NULL))
> >>           error_fatal("machine doesn't support -cpu option");  
> >
> > I won't complain too much if a board maintainer really wants to
> > make the board reject -cpu completely, but it's up to them.  
> 
> I disagree. I think a standard way of doing it is better. At least for
> each architecture. The ARM -cpu option is very confusing at the moment
> and it really doesn't need to be that bad.
> 
> >
> > Personally, I'd prefer to have all boards setting
> > default_cpu_type even if they support only one CPU model, so
> > clients don't need a special case for boards that don't support
> > -cpu.  
> 
> I agree, I think having one CPU makes more sense. It makes it easier
> to add support for more cpus in the future and allows the users to use
> the -cpu option without killing QEMU.
I'm considering -cpu option as a legacy one that server 2 purposes now
 1: pick cpu type for running instance
 2: convert optional features/legacy syntax to global properties
    for related cpu type

It plays ok for machines with single type of cpu but doesn't really scale
to more and doesn't work well nor needed if we were to specify cpus on CLI
with -device (i.e. build machine from config/CLI)

So I would not extend usage '-cpu' to boards that have fixed cpu type,
because it really useless in that case and confuses users with idea that
they have ability/need to specify -cpu on fixed cpu board.

I'd be upfront with users and fail explicitly if -cpu is not supported
(yes, it is not uniform CLI behavior across machines but it makes
sense since not all machines are the same, there probably are other
options with which some machines error out with unsupported error,
-cpu is not any different case).

> 
> Thanks,
> Alistair
> 
> >
> > --
> > Eduardo
> >  

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zcu102: Specify the valid CPUs

[Eduardo Habkost]

On Thu, Oct 05, 2017 at 11:04:27AM +0200, Igor Mammedov wrote:
> On Wed, 4 Oct 2017 14:39:20 -0700
> Alistair Francis <alistair.francis@xilinx.com> wrote:
> 
> > On Wed, Oct 4, 2017 at 9:34 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > On Wed, Oct 04, 2017 at 03:08:16PM +0200, Igor Mammedov wrote:  
> > >> On Wed, 4 Oct 2017 09:28:51 -0300
> > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >>  
> > >> > On Wed, Oct 04, 2017 at 01:12:32PM +0200, Igor Mammedov wrote:  
> > >> > > On Tue, 3 Oct 2017 14:41:17 -0700
> > >> > > Alistair Francis <alistair.francis@xilinx.com> wrote:
> > >> > >  
> > >> > > > On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:  
> > >> > > > > On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:  
> > >> > > > >> List all possible valid CPU options.
> > >> > > > >>
> > >> > > > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > >> > > > >> ---
> > >> > > > >>
> > >> > > > >>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
> > >> > > > >>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
> > >> > > > >>  include/hw/arm/xlnx-zynqmp.h |  1 +
> > >> > > > >>  3 files changed, 20 insertions(+), 7 deletions(-)
> > >> > > > >>
> > >> > > > >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> > >> > > > >> index 519a16ed98..039649e522 100644
> > >> > > > >> --- a/hw/arm/xlnx-zcu102.c
> > >> > > > >> +++ b/hw/arm/xlnx-zcu102.c
> > >> > > > >> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
> > >> > > > >>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> > >> > > > >>                                &error_abort);
> > >> > > > >>
> > >> > > > >> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> > >> > > > >> +                            &error_fatal);  
> > >> > > > >
> > >> > > > > Do you have plans to support other CPU types to xlnx_zynqmp in
> > >> > > > > the future?  If not, I wouldn't bother adding the cpu-type
> > >> > > > > property and the extra boilerplate code if it's always going to
> > >> > > > > be set to cortex-a53.  
> > >> > > >
> > >> > > > No, it'll always be A53.
> > >> > > >
> > >> > > > I did think of that, but I also wanted to use the new option! I also
> > >> > > > think there is an advantage in sanely handling users '-cpu' option,
> > >> > > > before now we just ignored it, so I think it still does give a
> > >> > > > benefit. That'll be especially important on the Xilinx tree (sometimes
> > >> > > > people use our machines with a different CPU to 'benchmark' or test
> > >> > > > other CPUs with our CoSimulation setup). So I think it does make sense
> > >> > > > to keep in.  
> > >> > > if cpu isn't user settable, one could just outright die if cpu_type
> > >> > > is not NULL and say that user's CLI is wrong.
> > >> > > (i.e. don't give users illusion that they allowed to use '-cpu')  
> > >> >
> > >> > Isn't it exactly what this patch does, by setting:
> > >> >     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> > >> >     mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
> > >> > ?
> > >> >
> > >> > Except that "-cpu cortex-a53" won't die, which is a good thing.  
> > >> allowing "-cpu cortex-a53" here, would allow to use feature parsing
> > >> which weren't allowed or were ignored before if user supplied '-cpu'.
> > >> so I'd more strict and refuse any -cpu and break CLI that tries to use it
> > >> if board has non configurable cpu type. It would be easier to relax
> > >> restriction later if necessary.
> > >>
> > >> using validate_cpus here just to have users for the new code,
> > >> doesn't seem like valid justification and at that it makes board
> > >> code more complex where it's not necessary and build in cpu type
> > >> works just fine.  
> > >
> > > It's up to the board maintainer to decide what's the best option.
> > > Both features are independent from each other and can be
> > > implemented by machine core.  
> > 
> > Noooo!
> > 
> > My hope with this series is that eventually we could hit a state where
> > every single machine acts the same way with the -cpu option.
> > 
> > I really don't like what we do now where some boards use it, some
> > boards error and some boars just ignore the option. I think we should
> > agree on something and every machine should follow the same flow so
> > that users know what to expect when they use the -cpu option.
> > 
> > If this means we allow machines to specify they don't support the
> > option or only have a single element in the list of supported options
> > doesn't really matter, but all machines should do the same thing.
> > 
> > >
> > > In either case, the valid_cpu_types feature will be still very
> > > useful for boards like pxa270 and sa1110, which support -cpu but
> > > only with specific families of CPU types (grep for
> > > "strncmp(cpu_type").
> > >  
> > >>
> > >> wrt centralized way to refuse -cpu if board doesn't support it,
> > >> (which is not really related to this series) following could be done:
> > >>
> > >> when cpu_model removal is completely done I plan to replace
> > >>   vl.c
> > >>      cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model)
> > >> with
> > >>      cpu_parse_cpu_model(DEFAULT_TARGET_CPU_TYPE, cpu_model)
> > >>
> > >> so that we could drop temporary guard
> > >>
> > >>      if (machine_class->default_cpu_type) {  
> > >
> > > This sounds good to me, even if we don't reject -cpu on any
> > > board.
> > >
> > >  
> > >>
> > >> with that it would be possible to tell from machine_run_board_init()
> > >> that board doesn't provide cpu but user provided '-cpu'
> > >> so we would be able to:
> > >>   if ((machine_class->default_cpu_type == NULL) &&
> > >>       (machine->cpu_type != NULL))
> > >>           error_fatal("machine doesn't support -cpu option");  
> > >
> > > I won't complain too much if a board maintainer really wants to
> > > make the board reject -cpu completely, but it's up to them.  
> > 
> > I disagree. I think a standard way of doing it is better. At least for
> > each architecture. The ARM -cpu option is very confusing at the moment
> > and it really doesn't need to be that bad.
> > 
> > >
> > > Personally, I'd prefer to have all boards setting
> > > default_cpu_type even if they support only one CPU model, so
> > > clients don't need a special case for boards that don't support
> > > -cpu.  
> > 
> > I agree, I think having one CPU makes more sense. It makes it easier
> > to add support for more cpus in the future and allows the users to use
> > the -cpu option without killing QEMU.
> I'm considering -cpu option as a legacy one that server 2 purposes now

I'm not sure about "legacy", but the list of purposes looks
accurate:

>  1: pick cpu type for running instance

This one has no replacement yet, so can we really call it legacy?

>  2: convert optional features/legacy syntax to global properties
>     for related cpu type

This one has a replacement: -global.  But there's a difference
between saying "-cpu features are implemented using -global" and
"-cpu features are obsoleted by -global".  I don't think we can
say it's obsolete or legacy unless existing management software
is changed to be using something else.


> 
> It plays ok for machines with single type of cpu but doesn't really scale
> to more and doesn't work well nor needed if we were to specify cpus on CLI
> with -device (i.e. build machine from config/CLI)

This is a good point.  But -cpu is still a useful shortcut for
boards that have a single CPU type.  What are the arguments we
have to get rid of it completely?


> 
> So I would not extend usage '-cpu' to boards that have fixed cpu type,
> because it really useless in that case and confuses users with idea that
> they have ability/need to specify -cpu on fixed cpu board.

If they try to choose any other CPU model, they will see an error
message explicitly saying only one CPU type is supported.  What
would be the harm?

> 
> I'd be upfront with users and fail explicitly if -cpu is not supported
> (yes, it is not uniform CLI behavior across machines but it makes
> sense since not all machines are the same, there probably are other
> options with which some machines error out with unsupported error,
> -cpu is not any different case).

I'm not strongly for/against neither of those two approaches, but
I'm inclined towards letting all (or most) machines support -cpu
as suggested by Alistair.

I see advantages in having less code relying on -cpu, and
replacing it with something more generic.  But I also see
advantages into reusing the same logic (both inside QEMU and on
management software) to query/configure/create CPUs for the cases
where a single CPU type is used.

I'd be more inclined to agree with you if -cpu was really an
obsolete option that was already completely replaced by something
else.  But the reality is that there's no generic mechanism to
choose the CPU type yet.

Unless we officially document -cpu as obsolete and point
users/developers to a replacement, I don't see the problem with
making "-cpu <model>" work on more (or all?) boards.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zcu102: Specify the valid CPUs

[Igor Mammedov]

On Thu, 5 Oct 2017 14:09:06 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Oct 05, 2017 at 11:04:27AM +0200, Igor Mammedov wrote:
> > On Wed, 4 Oct 2017 14:39:20 -0700
> > Alistair Francis <alistair.francis@xilinx.com> wrote:
> >   
> > > On Wed, Oct 4, 2017 at 9:34 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:  
> > > > On Wed, Oct 04, 2017 at 03:08:16PM +0200, Igor Mammedov wrote:    
> > > >> On Wed, 4 Oct 2017 09:28:51 -0300
> > > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >>    
> > > >> > On Wed, Oct 04, 2017 at 01:12:32PM +0200, Igor Mammedov wrote:    
> > > >> > > On Tue, 3 Oct 2017 14:41:17 -0700
> > > >> > > Alistair Francis <alistair.francis@xilinx.com> wrote:
> > > >> > >    
> > > >> > > > On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:    
> > > >> > > > > On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:    
> > > >> > > > >> List all possible valid CPU options.
> > > >> > > > >>
> > > >> > > > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > > >> > > > >> ---
> > > >> > > > >>
> > > >> > > > >>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
> > > >> > > > >>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
> > > >> > > > >>  include/hw/arm/xlnx-zynqmp.h |  1 +
> > > >> > > > >>  3 files changed, 20 insertions(+), 7 deletions(-)
> > > >> > > > >>
> > > >> > > > >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> > > >> > > > >> index 519a16ed98..039649e522 100644
> > > >> > > > >> --- a/hw/arm/xlnx-zcu102.c
> > > >> > > > >> +++ b/hw/arm/xlnx-zcu102.c
> > > >> > > > >> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
> > > >> > > > >>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> > > >> > > > >>                                &error_abort);
> > > >> > > > >>
> > > >> > > > >> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> > > >> > > > >> +                            &error_fatal);    
> > > >> > > > >
> > > >> > > > > Do you have plans to support other CPU types to xlnx_zynqmp in
> > > >> > > > > the future?  If not, I wouldn't bother adding the cpu-type
> > > >> > > > > property and the extra boilerplate code if it's always going to
> > > >> > > > > be set to cortex-a53.    
> > > >> > > >
> > > >> > > > No, it'll always be A53.
> > > >> > > >
> > > >> > > > I did think of that, but I also wanted to use the new option! I also
> > > >> > > > think there is an advantage in sanely handling users '-cpu' option,
> > > >> > > > before now we just ignored it, so I think it still does give a
> > > >> > > > benefit. That'll be especially important on the Xilinx tree (sometimes
> > > >> > > > people use our machines with a different CPU to 'benchmark' or test
> > > >> > > > other CPUs with our CoSimulation setup). So I think it does make sense
> > > >> > > > to keep in.    
> > > >> > > if cpu isn't user settable, one could just outright die if cpu_type
> > > >> > > is not NULL and say that user's CLI is wrong.
> > > >> > > (i.e. don't give users illusion that they allowed to use '-cpu')    
> > > >> >
> > > >> > Isn't it exactly what this patch does, by setting:
> > > >> >     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> > > >> >     mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
> > > >> > ?
> > > >> >
> > > >> > Except that "-cpu cortex-a53" won't die, which is a good thing.    
> > > >> allowing "-cpu cortex-a53" here, would allow to use feature parsing
> > > >> which weren't allowed or were ignored before if user supplied '-cpu'.
> > > >> so I'd more strict and refuse any -cpu and break CLI that tries to use it
> > > >> if board has non configurable cpu type. It would be easier to relax
> > > >> restriction later if necessary.
> > > >>
> > > >> using validate_cpus here just to have users for the new code,
> > > >> doesn't seem like valid justification and at that it makes board
> > > >> code more complex where it's not necessary and build in cpu type
> > > >> works just fine.    
> > > >
> > > > It's up to the board maintainer to decide what's the best option.
> > > > Both features are independent from each other and can be
> > > > implemented by machine core.    
> > > 
> > > Noooo!
> > > 
> > > My hope with this series is that eventually we could hit a state where
> > > every single machine acts the same way with the -cpu option.
> > > 
> > > I really don't like what we do now where some boards use it, some
> > > boards error and some boars just ignore the option. I think we should
> > > agree on something and every machine should follow the same flow so
> > > that users know what to expect when they use the -cpu option.
> > > 
> > > If this means we allow machines to specify they don't support the
> > > option or only have a single element in the list of supported options
> > > doesn't really matter, but all machines should do the same thing.
> > >   
> > > >
> > > > In either case, the valid_cpu_types feature will be still very
> > > > useful for boards like pxa270 and sa1110, which support -cpu but
> > > > only with specific families of CPU types (grep for
> > > > "strncmp(cpu_type").
> > > >    
> > > >>
> > > >> wrt centralized way to refuse -cpu if board doesn't support it,
> > > >> (which is not really related to this series) following could be done:
> > > >>
> > > >> when cpu_model removal is completely done I plan to replace
> > > >>   vl.c
> > > >>      cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model)
> > > >> with
> > > >>      cpu_parse_cpu_model(DEFAULT_TARGET_CPU_TYPE, cpu_model)
> > > >>
> > > >> so that we could drop temporary guard
> > > >>
> > > >>      if (machine_class->default_cpu_type) {    
> > > >
> > > > This sounds good to me, even if we don't reject -cpu on any
> > > > board.
> > > >
> > > >    
> > > >>
> > > >> with that it would be possible to tell from machine_run_board_init()
> > > >> that board doesn't provide cpu but user provided '-cpu'
> > > >> so we would be able to:
> > > >>   if ((machine_class->default_cpu_type == NULL) &&
> > > >>       (machine->cpu_type != NULL))
> > > >>           error_fatal("machine doesn't support -cpu option");    
> > > >
> > > > I won't complain too much if a board maintainer really wants to
> > > > make the board reject -cpu completely, but it's up to them.    
> > > 
> > > I disagree. I think a standard way of doing it is better. At least for
> > > each architecture. The ARM -cpu option is very confusing at the moment
> > > and it really doesn't need to be that bad.
> > >   
> > > >
> > > > Personally, I'd prefer to have all boards setting
> > > > default_cpu_type even if they support only one CPU model, so
> > > > clients don't need a special case for boards that don't support
> > > > -cpu.    
> > > 
> > > I agree, I think having one CPU makes more sense. It makes it easier
> > > to add support for more cpus in the future and allows the users to use
> > > the -cpu option without killing QEMU.  
> > I'm considering -cpu option as a legacy one that server 2 purposes now  
> 
> I'm not sure about "legacy", but the list of purposes looks
> accurate:
> 
> >  1: pick cpu type for running instance  
> 
> This one has no replacement yet, so can we really call it legacy?
not really, it's not going anywhere in near future

> 
> >  2: convert optional features/legacy syntax to global properties
> >     for related cpu type  
> 
> This one has a replacement: -global.  But there's a difference
> between saying "-cpu features are implemented using -global" and
> "-cpu features are obsoleted by -global".  I don't think we can
> say it's obsolete or legacy unless existing management software
> is changed to be using something else.
> 
> 
> > 
> > It plays ok for machines with single type of cpu but doesn't really scale
> > to more and doesn't work well nor needed if we were to specify cpus on CLI
> > with -device (i.e. build machine from config/CLI)  
> 
> This is a good point.  But -cpu is still a useful shortcut for
> boards that have a single CPU type.  What are the arguments we
> have to get rid of it completely?
boards that have single cpu type don't need -cpu. since cpu is not
configurable there.


> > So I would not extend usage '-cpu' to boards that have fixed cpu type,
> > because it really useless in that case and confuses users with idea that
> > they have ability/need to specify -cpu on fixed cpu board.  
> 
> If they try to choose any other CPU model, they will see an error
> message explicitly saying only one CPU type is supported.  What
> would be the harm?
I guess I've already pointed drawbacks from interface point of view,
from maintainer pov it will be extra code to maintain valid cpus
vs just 'create_cpu(MY_CPU_TYPE)'
this patch is vivid example of the case


> > I'd be upfront with users and fail explicitly if -cpu is not supported
> > (yes, it is not uniform CLI behavior across machines but it makes
> > sense since not all machines are the same, there probably are other
> > options with which some machines error out with unsupported error,
> > -cpu is not any different case).  
> 
> I'm not strongly for/against neither of those two approaches, but
> I'm inclined towards letting all (or most) machines support -cpu
> as suggested by Alistair.
Alistair said 'I also wanted to use the new option!'
and not allow users to specify a cpu for 'testing' that will be ignored anyways.
there are 2 ways to do the later 
  1. complicated, do it using valid_cpus as in this patch
     and error out if wrong cpu is specified
  2. simple, error out if board doesn't allow to change cpu type.
     could also be done from one centralized place and
     a board developer won't need to add extra to to support
     default/valid cpus at all
 
> I see advantages in having less code relying on -cpu, and
> replacing it with something more generic.  But I also see
> advantages into reusing the same logic (both inside QEMU and on
> management software) to query/configure/create CPUs for the cases
> where a single CPU type is used.
management shouldn't care about querying cpu types for machines
with fixed cpu as it won't be really able to configure it.


> I'd be more inclined to agree with you if -cpu was really an
> obsolete option that was already completely replaced by something
> else.  But the reality is that there's no generic mechanism to
> choose the CPU type yet.
there is no choice with fixed cpu boards, it's just soldered on.
 
> Unless we officially document -cpu as obsolete and point
> users/developers to a replacement, I don't see the problem with
> making "-cpu <model>" work on more (or all?) boards.
as I've already pointed out issues are:
 - it's confusing for user (he/she sees ability to specify cpu)
 - using -cpu won't have any effect in practice
 - extra code vs just creating build in cpu, confusing for developer

all of above could be avoided by bailing out if -cpu is used with
fixed cpu boards.

PS:
I can come up with another option that have a fixed value
for a some boards, should we replace their hardcoded values
with extra generic handling of useless for board option too?
Lets not go down the road of enabling something where it
doesn't make much sense and only adds up to confusion/maintenance.

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zcu102: Specify the valid CPUs

[Eduardo Habkost]

On Fri, Oct 06, 2017 at 10:23:12AM +0200, Igor Mammedov wrote:
> On Thu, 5 Oct 2017 14:09:06 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Oct 05, 2017 at 11:04:27AM +0200, Igor Mammedov wrote:
> > > On Wed, 4 Oct 2017 14:39:20 -0700
> > > Alistair Francis <alistair.francis@xilinx.com> wrote:
> > >   
> > > > On Wed, Oct 4, 2017 at 9:34 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:  
> > > > > On Wed, Oct 04, 2017 at 03:08:16PM +0200, Igor Mammedov wrote:    
> > > > >> On Wed, 4 Oct 2017 09:28:51 -0300
> > > > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > >>    
> > > > >> > On Wed, Oct 04, 2017 at 01:12:32PM +0200, Igor Mammedov wrote:    
> > > > >> > > On Tue, 3 Oct 2017 14:41:17 -0700
> > > > >> > > Alistair Francis <alistair.francis@xilinx.com> wrote:
> > > > >> > >    
> > > > >> > > > On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:    
> > > > >> > > > > On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:    
> > > > >> > > > >> List all possible valid CPU options.
> > > > >> > > > >>
> > > > >> > > > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > > > >> > > > >> ---
> > > > >> > > > >>
> > > > >> > > > >>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
> > > > >> > > > >>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
> > > > >> > > > >>  include/hw/arm/xlnx-zynqmp.h |  1 +
> > > > >> > > > >>  3 files changed, 20 insertions(+), 7 deletions(-)
> > > > >> > > > >>
> > > > >> > > > >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> > > > >> > > > >> index 519a16ed98..039649e522 100644
> > > > >> > > > >> --- a/hw/arm/xlnx-zcu102.c
> > > > >> > > > >> +++ b/hw/arm/xlnx-zcu102.c
> > > > >> > > > >> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
> > > > >> > > > >>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> > > > >> > > > >>                                &error_abort);
> > > > >> > > > >>
> > > > >> > > > >> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> > > > >> > > > >> +                            &error_fatal);    
> > > > >> > > > >
> > > > >> > > > > Do you have plans to support other CPU types to xlnx_zynqmp in
> > > > >> > > > > the future?  If not, I wouldn't bother adding the cpu-type
> > > > >> > > > > property and the extra boilerplate code if it's always going to
> > > > >> > > > > be set to cortex-a53.    
> > > > >> > > >
> > > > >> > > > No, it'll always be A53.
> > > > >> > > >
> > > > >> > > > I did think of that, but I also wanted to use the new option! I also
> > > > >> > > > think there is an advantage in sanely handling users '-cpu' option,
> > > > >> > > > before now we just ignored it, so I think it still does give a
> > > > >> > > > benefit. That'll be especially important on the Xilinx tree (sometimes
> > > > >> > > > people use our machines with a different CPU to 'benchmark' or test
> > > > >> > > > other CPUs with our CoSimulation setup). So I think it does make sense
> > > > >> > > > to keep in.    
> > > > >> > > if cpu isn't user settable, one could just outright die if cpu_type
> > > > >> > > is not NULL and say that user's CLI is wrong.
> > > > >> > > (i.e. don't give users illusion that they allowed to use '-cpu')    
> > > > >> >
> > > > >> > Isn't it exactly what this patch does, by setting:
> > > > >> >     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> > > > >> >     mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
> > > > >> > ?
> > > > >> >
> > > > >> > Except that "-cpu cortex-a53" won't die, which is a good thing.    
> > > > >> allowing "-cpu cortex-a53" here, would allow to use feature parsing
> > > > >> which weren't allowed or were ignored before if user supplied '-cpu'.
> > > > >> so I'd more strict and refuse any -cpu and break CLI that tries to use it
> > > > >> if board has non configurable cpu type. It would be easier to relax
> > > > >> restriction later if necessary.
> > > > >>
> > > > >> using validate_cpus here just to have users for the new code,
> > > > >> doesn't seem like valid justification and at that it makes board
> > > > >> code more complex where it's not necessary and build in cpu type
> > > > >> works just fine.    
> > > > >
> > > > > It's up to the board maintainer to decide what's the best option.
> > > > > Both features are independent from each other and can be
> > > > > implemented by machine core.    
> > > > 
> > > > Noooo!
> > > > 
> > > > My hope with this series is that eventually we could hit a state where
> > > > every single machine acts the same way with the -cpu option.
> > > > 
> > > > I really don't like what we do now where some boards use it, some
> > > > boards error and some boars just ignore the option. I think we should
> > > > agree on something and every machine should follow the same flow so
> > > > that users know what to expect when they use the -cpu option.
> > > > 
> > > > If this means we allow machines to specify they don't support the
> > > > option or only have a single element in the list of supported options
> > > > doesn't really matter, but all machines should do the same thing.
> > > >   
> > > > >
> > > > > In either case, the valid_cpu_types feature will be still very
> > > > > useful for boards like pxa270 and sa1110, which support -cpu but
> > > > > only with specific families of CPU types (grep for
> > > > > "strncmp(cpu_type").
> > > > >    
> > > > >>
> > > > >> wrt centralized way to refuse -cpu if board doesn't support it,
> > > > >> (which is not really related to this series) following could be done:
> > > > >>
> > > > >> when cpu_model removal is completely done I plan to replace
> > > > >>   vl.c
> > > > >>      cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model)
> > > > >> with
> > > > >>      cpu_parse_cpu_model(DEFAULT_TARGET_CPU_TYPE, cpu_model)
> > > > >>
> > > > >> so that we could drop temporary guard
> > > > >>
> > > > >>      if (machine_class->default_cpu_type) {    
> > > > >
> > > > > This sounds good to me, even if we don't reject -cpu on any
> > > > > board.
> > > > >
> > > > >    
> > > > >>
> > > > >> with that it would be possible to tell from machine_run_board_init()
> > > > >> that board doesn't provide cpu but user provided '-cpu'
> > > > >> so we would be able to:
> > > > >>   if ((machine_class->default_cpu_type == NULL) &&
> > > > >>       (machine->cpu_type != NULL))
> > > > >>           error_fatal("machine doesn't support -cpu option");    
> > > > >
> > > > > I won't complain too much if a board maintainer really wants to
> > > > > make the board reject -cpu completely, but it's up to them.    
> > > > 
> > > > I disagree. I think a standard way of doing it is better. At least for
> > > > each architecture. The ARM -cpu option is very confusing at the moment
> > > > and it really doesn't need to be that bad.
> > > >   
> > > > >
> > > > > Personally, I'd prefer to have all boards setting
> > > > > default_cpu_type even if they support only one CPU model, so
> > > > > clients don't need a special case for boards that don't support
> > > > > -cpu.    
> > > > 
> > > > I agree, I think having one CPU makes more sense. It makes it easier
> > > > to add support for more cpus in the future and allows the users to use
> > > > the -cpu option without killing QEMU.  
> > > I'm considering -cpu option as a legacy one that server 2 purposes now  
> > 
> > I'm not sure about "legacy", but the list of purposes looks
> > accurate:
> > 
> > >  1: pick cpu type for running instance  
> > 
> > This one has no replacement yet, so can we really call it legacy?
> not really, it's not going anywhere in near future
> 
> > 
> > >  2: convert optional features/legacy syntax to global properties
> > >     for related cpu type  
> > 
> > This one has a replacement: -global.  But there's a difference
> > between saying "-cpu features are implemented using -global" and
> > "-cpu features are obsoleted by -global".  I don't think we can
> > say it's obsolete or legacy unless existing management software
> > is changed to be using something else.
> > 
> > 
> > > 
> > > It plays ok for machines with single type of cpu but doesn't really scale
> > > to more and doesn't work well nor needed if we were to specify cpus on CLI
> > > with -device (i.e. build machine from config/CLI)  
> > 
> > This is a good point.  But -cpu is still a useful shortcut for
> > boards that have a single CPU type.  What are the arguments we
> > have to get rid of it completely?
> boards that have single cpu type don't need -cpu. since cpu is not
> configurable there.

They don't need -cpu, but there's no need to reject "-cpu FOO" if
we know FOO is the CPU model used by the board.  This is the only
difference between what you propose and what Alistair proposes,
right?


> 
> 
> > > So I would not extend usage '-cpu' to boards that have fixed cpu type,
> > > because it really useless in that case and confuses users with idea that
> > > they have ability/need to specify -cpu on fixed cpu board.  
> > 
> > If they try to choose any other CPU model, they will see an error
> > message explicitly saying only one CPU type is supported.  What
> > would be the harm?
> I guess I've already pointed drawbacks from interface point of view,
> from maintainer pov it will be extra code to maintain valid cpus
> vs just 'create_cpu(MY_CPU_TYPE)'
> this patch is vivid example of the case

With this part I agree.  We don't need to add boilerplate code to
board init if the CPU model will always be the same.

But I would still prefer to do this:

  create_cpu(MY_CPU_TYPE);  // at XXX_init()
[...]
  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 = { MY_CPU_TYPE, NULL };
  }

because this will let management software know that the board
creates CPU of type MY_CPU_TYPE.

> 
> 
> > > I'd be upfront with users and fail explicitly if -cpu is not supported
> > > (yes, it is not uniform CLI behavior across machines but it makes
> > > sense since not all machines are the same, there probably are other
> > > options with which some machines error out with unsupported error,
> > > -cpu is not any different case).  
> > 
> > I'm not strongly for/against neither of those two approaches, but
> > I'm inclined towards letting all (or most) machines support -cpu
> > as suggested by Alistair.
> Alistair said 'I also wanted to use the new option!'
> and not allow users to specify a cpu for 'testing' that will be ignored anyways.
> there are 2 ways to do the later 
>   1. complicated, do it using valid_cpus as in this patch
>      and error out if wrong cpu is specified
>   2. simple, error out if board doesn't allow to change cpu type.
>      could also be done from one centralized place and
>      a board developer won't need to add extra to to support
>      default/valid cpus at all

Well, the "complicated" option is just 2 lines of code at
class_init.  (see above)


>  
> > I see advantages in having less code relying on -cpu, and
> > replacing it with something more generic.  But I also see
> > advantages into reusing the same logic (both inside QEMU and on
> > management software) to query/configure/create CPUs for the cases
> > where a single CPU type is used.
> management shouldn't care about querying cpu types for machines
> with fixed cpu as it won't be really able to configure it.

Management could show the user what's the CPU used by the board.
"-machine BOARD -cpu help" could show the user what's the CPU
used by the board.

> 
> 
> > I'd be more inclined to agree with you if -cpu was really an
> > obsolete option that was already completely replaced by something
> > else.  But the reality is that there's no generic mechanism to
> > choose the CPU type yet.
> there is no choice with fixed cpu boards, it's just soldered on.

True, but what's the harm in saying "there's no choice, and the
only choice is cortex-a53" instead of "there's no choice, and I
won't tell you what's the CPU type"?

>  
> > Unless we officially document -cpu as obsolete and point
> > users/developers to a replacement, I don't see the problem with
> > making "-cpu <model>" work on more (or all?) boards.
> as I've already pointed out issues are:
>  - it's confusing for user (he/she sees ability to specify cpu)

Where exactly does the user "sees" the ability to specify the CPU?

>  - using -cpu won't have any effect in practice

True, but why this is a problem?

"-cpu qemu64" doesn't have any effect in practice in x86 but we
don't make PC reject "-cpu qemu64".  "-cpu cortex-a53" won't have
any effect on xlnz-zcu102, but we don't need to make QEMU error
out, either.


>  - extra code vs just creating build in cpu, confusing for developer

The extra code is just 2 lines of code in class_init.

We could even make it 1 line of code, if we define
valid_cpu_types=NULL as equivalent to { default_cpu_type, NULL }
(but only after we make all boards that truly support -cpu today
set valid_cpu_types).

> 
> all of above could be avoided by bailing out if -cpu is used with
> fixed cpu boards.

The only problem I see above is "extra code", but it's only
2 lines of code on class_init.

This means I don't think it's an argument against doing it on a
specific board if the board maintainer wants to.

However, this might be an argument for not requiring it to be
done on all boards, unless there's a visible benefit for the user
or management software.

> 
> PS:
> I can come up with another option that have a fixed value
> for a some boards, should we replace their hardcoded values
> with extra generic handling of useless for board option too?
> Lets not go down the road of enabling something where it
> doesn't make much sense and only adds up to confusion/maintenance.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zcu102: Specify the valid CPUs

[Alistair Francis]

On Fri, Oct 6, 2017 at 4:45 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Fri, Oct 06, 2017 at 10:23:12AM +0200, Igor Mammedov wrote:
>> On Thu, 5 Oct 2017 14:09:06 -0300
>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>
>> > On Thu, Oct 05, 2017 at 11:04:27AM +0200, Igor Mammedov wrote:
>> > > On Wed, 4 Oct 2017 14:39:20 -0700
>> > > Alistair Francis <alistair.francis@xilinx.com> wrote:
>> > >
>> > > > On Wed, Oct 4, 2017 at 9:34 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > > > > On Wed, Oct 04, 2017 at 03:08:16PM +0200, Igor Mammedov wrote:
>> > > > >> On Wed, 4 Oct 2017 09:28:51 -0300
>> > > > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > > > >>
>> > > > >> > On Wed, Oct 04, 2017 at 01:12:32PM +0200, Igor Mammedov wrote:
>> > > > >> > > On Tue, 3 Oct 2017 14:41:17 -0700
>> > > > >> > > Alistair Francis <alistair.francis@xilinx.com> wrote:
>> > > > >> > >
>> > > > >> > > > On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > > > >> > > > > On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:
>> > > > >> > > > >> List all possible valid CPU options.
>> > > > >> > > > >>
>> > > > >> > > > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> > > > >> > > > >> ---
>> > > > >> > > > >>
>> > > > >> > > > >>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
>> > > > >> > > > >>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
>> > > > >> > > > >>  include/hw/arm/xlnx-zynqmp.h |  1 +
>> > > > >> > > > >>  3 files changed, 20 insertions(+), 7 deletions(-)
>> > > > >> > > > >>
>> > > > >> > > > >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
>> > > > >> > > > >> index 519a16ed98..039649e522 100644
>> > > > >> > > > >> --- a/hw/arm/xlnx-zcu102.c
>> > > > >> > > > >> +++ b/hw/arm/xlnx-zcu102.c
>> > > > >> > > > >> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
>> > > > >> > > > >>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
>> > > > >> > > > >>                                &error_abort);
>> > > > >> > > > >>
>> > > > >> > > > >> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
>> > > > >> > > > >> +                            &error_fatal);
>> > > > >> > > > >
>> > > > >> > > > > Do you have plans to support other CPU types to xlnx_zynqmp in
>> > > > >> > > > > the future?  If not, I wouldn't bother adding the cpu-type
>> > > > >> > > > > property and the extra boilerplate code if it's always going to
>> > > > >> > > > > be set to cortex-a53.
>> > > > >> > > >
>> > > > >> > > > No, it'll always be A53.
>> > > > >> > > >
>> > > > >> > > > I did think of that, but I also wanted to use the new option! I also
>> > > > >> > > > think there is an advantage in sanely handling users '-cpu' option,
>> > > > >> > > > before now we just ignored it, so I think it still does give a
>> > > > >> > > > benefit. That'll be especially important on the Xilinx tree (sometimes
>> > > > >> > > > people use our machines with a different CPU to 'benchmark' or test
>> > > > >> > > > other CPUs with our CoSimulation setup). So I think it does make sense
>> > > > >> > > > to keep in.
>> > > > >> > > if cpu isn't user settable, one could just outright die if cpu_type
>> > > > >> > > is not NULL and say that user's CLI is wrong.
>> > > > >> > > (i.e. don't give users illusion that they allowed to use '-cpu')
>> > > > >> >
>> > > > >> > Isn't it exactly what this patch does, by setting:
>> > > > >> >     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
>> > > > >> >     mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
>> > > > >> > ?
>> > > > >> >
>> > > > >> > Except that "-cpu cortex-a53" won't die, which is a good thing.
>> > > > >> allowing "-cpu cortex-a53" here, would allow to use feature parsing
>> > > > >> which weren't allowed or were ignored before if user supplied '-cpu'.
>> > > > >> so I'd more strict and refuse any -cpu and break CLI that tries to use it
>> > > > >> if board has non configurable cpu type. It would be easier to relax
>> > > > >> restriction later if necessary.
>> > > > >>
>> > > > >> using validate_cpus here just to have users for the new code,
>> > > > >> doesn't seem like valid justification and at that it makes board
>> > > > >> code more complex where it's not necessary and build in cpu type
>> > > > >> works just fine.
>> > > > >
>> > > > > It's up to the board maintainer to decide what's the best option.
>> > > > > Both features are independent from each other and can be
>> > > > > implemented by machine core.
>> > > >
>> > > > Noooo!
>> > > >
>> > > > My hope with this series is that eventually we could hit a state where
>> > > > every single machine acts the same way with the -cpu option.
>> > > >
>> > > > I really don't like what we do now where some boards use it, some
>> > > > boards error and some boars just ignore the option. I think we should
>> > > > agree on something and every machine should follow the same flow so
>> > > > that users know what to expect when they use the -cpu option.
>> > > >
>> > > > If this means we allow machines to specify they don't support the
>> > > > option or only have a single element in the list of supported options
>> > > > doesn't really matter, but all machines should do the same thing.
>> > > >
>> > > > >
>> > > > > In either case, the valid_cpu_types feature will be still very
>> > > > > useful for boards like pxa270 and sa1110, which support -cpu but
>> > > > > only with specific families of CPU types (grep for
>> > > > > "strncmp(cpu_type").
>> > > > >
>> > > > >>
>> > > > >> wrt centralized way to refuse -cpu if board doesn't support it,
>> > > > >> (which is not really related to this series) following could be done:
>> > > > >>
>> > > > >> when cpu_model removal is completely done I plan to replace
>> > > > >>   vl.c
>> > > > >>      cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model)
>> > > > >> with
>> > > > >>      cpu_parse_cpu_model(DEFAULT_TARGET_CPU_TYPE, cpu_model)
>> > > > >>
>> > > > >> so that we could drop temporary guard
>> > > > >>
>> > > > >>      if (machine_class->default_cpu_type) {
>> > > > >
>> > > > > This sounds good to me, even if we don't reject -cpu on any
>> > > > > board.
>> > > > >
>> > > > >
>> > > > >>
>> > > > >> with that it would be possible to tell from machine_run_board_init()
>> > > > >> that board doesn't provide cpu but user provided '-cpu'
>> > > > >> so we would be able to:
>> > > > >>   if ((machine_class->default_cpu_type == NULL) &&
>> > > > >>       (machine->cpu_type != NULL))
>> > > > >>           error_fatal("machine doesn't support -cpu option");
>> > > > >
>> > > > > I won't complain too much if a board maintainer really wants to
>> > > > > make the board reject -cpu completely, but it's up to them.
>> > > >
>> > > > I disagree. I think a standard way of doing it is better. At least for
>> > > > each architecture. The ARM -cpu option is very confusing at the moment
>> > > > and it really doesn't need to be that bad.
>> > > >
>> > > > >
>> > > > > Personally, I'd prefer to have all boards setting
>> > > > > default_cpu_type even if they support only one CPU model, so
>> > > > > clients don't need a special case for boards that don't support
>> > > > > -cpu.
>> > > >
>> > > > I agree, I think having one CPU makes more sense. It makes it easier
>> > > > to add support for more cpus in the future and allows the users to use
>> > > > the -cpu option without killing QEMU.
>> > > I'm considering -cpu option as a legacy one that server 2 purposes now
>> >
>> > I'm not sure about "legacy", but the list of purposes looks
>> > accurate:
>> >
>> > >  1: pick cpu type for running instance
>> >
>> > This one has no replacement yet, so can we really call it legacy?
>> not really, it's not going anywhere in near future
>>
>> >
>> > >  2: convert optional features/legacy syntax to global properties
>> > >     for related cpu type
>> >
>> > This one has a replacement: -global.  But there's a difference
>> > between saying "-cpu features are implemented using -global" and
>> > "-cpu features are obsoleted by -global".  I don't think we can
>> > say it's obsolete or legacy unless existing management software
>> > is changed to be using something else.
>> >
>> >
>> > >
>> > > It plays ok for machines with single type of cpu but doesn't really scale
>> > > to more and doesn't work well nor needed if we were to specify cpus on CLI
>> > > with -device (i.e. build machine from config/CLI)
>> >
>> > This is a good point.  But -cpu is still a useful shortcut for
>> > boards that have a single CPU type.  What are the arguments we
>> > have to get rid of it completely?
>> boards that have single cpu type don't need -cpu. since cpu is not
>> configurable there.
>
> They don't need -cpu, but there's no need to reject "-cpu FOO" if
> we know FOO is the CPU model used by the board.  This is the only
> difference between what you propose and what Alistair proposes,
> right?
>
>
>>
>>
>> > > So I would not extend usage '-cpu' to boards that have fixed cpu type,
>> > > because it really useless in that case and confuses users with idea that
>> > > they have ability/need to specify -cpu on fixed cpu board.
>> >
>> > If they try to choose any other CPU model, they will see an error
>> > message explicitly saying only one CPU type is supported.  What
>> > would be the harm?
>> I guess I've already pointed drawbacks from interface point of view,
>> from maintainer pov it will be extra code to maintain valid cpus
>> vs just 'create_cpu(MY_CPU_TYPE)'
>> this patch is vivid example of the case
>
> With this part I agree.  We don't need to add boilerplate code to
> board init if the CPU model will always be the same.
>
> But I would still prefer to do this:
>
>   create_cpu(MY_CPU_TYPE);  // at XXX_init()
> [...]
>   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 = { MY_CPU_TYPE, NULL };
>   }

I like this option. It doesn't add much code and I think makes it very
clear to users.

Another thing to point out is that I see users specifying options to
QEMU all the time that QEMU will just ignore. Imagine people see
somewhere online that others use '-cpu' and suddenly they think they
have to. Having this throw an error that '-cpu' isn't supported in
this case (but is in others) will create confusion of when it
should/shouldn't be use. I think always allowing it and telling users
the supported CPUs clears this up.

Thanks,
Alistair

>
> because this will let management software know that the board
> creates CPU of type MY_CPU_TYPE.
>
>>
>>
>> > > I'd be upfront with users and fail explicitly if -cpu is not supported
>> > > (yes, it is not uniform CLI behavior across machines but it makes
>> > > sense since not all machines are the same, there probably are other
>> > > options with which some machines error out with unsupported error,
>> > > -cpu is not any different case).
>> >
>> > I'm not strongly for/against neither of those two approaches, but
>> > I'm inclined towards letting all (or most) machines support -cpu
>> > as suggested by Alistair.
>> Alistair said 'I also wanted to use the new option!'
>> and not allow users to specify a cpu for 'testing' that will be ignored anyways.
>> there are 2 ways to do the later
>>   1. complicated, do it using valid_cpus as in this patch
>>      and error out if wrong cpu is specified
>>   2. simple, error out if board doesn't allow to change cpu type.
>>      could also be done from one centralized place and
>>      a board developer won't need to add extra to to support
>>      default/valid cpus at all
>
> Well, the "complicated" option is just 2 lines of code at
> class_init.  (see above)
>
>
>>
>> > I see advantages in having less code relying on -cpu, and
>> > replacing it with something more generic.  But I also see
>> > advantages into reusing the same logic (both inside QEMU and on
>> > management software) to query/configure/create CPUs for the cases
>> > where a single CPU type is used.
>> management shouldn't care about querying cpu types for machines
>> with fixed cpu as it won't be really able to configure it.
>
> Management could show the user what's the CPU used by the board.
> "-machine BOARD -cpu help" could show the user what's the CPU
> used by the board.
>
>>
>>
>> > I'd be more inclined to agree with you if -cpu was really an
>> > obsolete option that was already completely replaced by something
>> > else.  But the reality is that there's no generic mechanism to
>> > choose the CPU type yet.
>> there is no choice with fixed cpu boards, it's just soldered on.
>
> True, but what's the harm in saying "there's no choice, and the
> only choice is cortex-a53" instead of "there's no choice, and I
> won't tell you what's the CPU type"?
>
>>
>> > Unless we officially document -cpu as obsolete and point
>> > users/developers to a replacement, I don't see the problem with
>> > making "-cpu <model>" work on more (or all?) boards.
>> as I've already pointed out issues are:
>>  - it's confusing for user (he/she sees ability to specify cpu)
>
> Where exactly does the user "sees" the ability to specify the CPU?
>
>>  - using -cpu won't have any effect in practice
>
> True, but why this is a problem?
>
> "-cpu qemu64" doesn't have any effect in practice in x86 but we
> don't make PC reject "-cpu qemu64".  "-cpu cortex-a53" won't have
> any effect on xlnz-zcu102, but we don't need to make QEMU error
> out, either.
>
>
>>  - extra code vs just creating build in cpu, confusing for developer
>
> The extra code is just 2 lines of code in class_init.
>
> We could even make it 1 line of code, if we define
> valid_cpu_types=NULL as equivalent to { default_cpu_type, NULL }
> (but only after we make all boards that truly support -cpu today
> set valid_cpu_types).
>
>>
>> all of above could be avoided by bailing out if -cpu is used with
>> fixed cpu boards.
>
> The only problem I see above is "extra code", but it's only
> 2 lines of code on class_init.
>
> This means I don't think it's an argument against doing it on a
> specific board if the board maintainer wants to.
>
> However, this might be an argument for not requiring it to be
> done on all boards, unless there's a visible benefit for the user
> or management software.
>
>>
>> PS:
>> I can come up with another option that have a fixed value
>> for a some boards, should we replace their hardcoded values
>> with extra generic handling of useless for board option too?
>> Lets not go down the road of enabling something where it
>> doesn't make much sense and only adds up to confusion/maintenance.
>
> --
> Eduardo
>

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zcu102: Specify the valid CPUs

[Igor Mammedov]

On Fri, 6 Oct 2017 15:06:57 -0700
Alistair Francis <alistair.francis@xilinx.com> wrote:

> On Fri, Oct 6, 2017 at 4:45 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Fri, Oct 06, 2017 at 10:23:12AM +0200, Igor Mammedov wrote:  
> >> On Thu, 5 Oct 2017 14:09:06 -0300
> >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>  
> >> > On Thu, Oct 05, 2017 at 11:04:27AM +0200, Igor Mammedov wrote:  
> >> > > On Wed, 4 Oct 2017 14:39:20 -0700
> >> > > Alistair Francis <alistair.francis@xilinx.com> wrote:
> >> > >  
> >> > > > On Wed, Oct 4, 2017 at 9:34 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:  
> >> > > > > On Wed, Oct 04, 2017 at 03:08:16PM +0200, Igor Mammedov wrote:  
> >> > > > >> On Wed, 4 Oct 2017 09:28:51 -0300
> >> > > > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> > > > >>  
> >> > > > >> > On Wed, Oct 04, 2017 at 01:12:32PM +0200, Igor Mammedov wrote:  
> >> > > > >> > > On Tue, 3 Oct 2017 14:41:17 -0700
> >> > > > >> > > Alistair Francis <alistair.francis@xilinx.com> wrote:
> >> > > > >> > >  
> >> > > > >> > > > On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:  
> >> > > > >> > > > > On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:  
> >> > > > >> > > > >> List all possible valid CPU options.
> >> > > > >> > > > >>
> >> > > > >> > > > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >> > > > >> > > > >> ---
> >> > > > >> > > > >>
> >> > > > >> > > > >>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
> >> > > > >> > > > >>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
> >> > > > >> > > > >>  include/hw/arm/xlnx-zynqmp.h |  1 +
> >> > > > >> > > > >>  3 files changed, 20 insertions(+), 7 deletions(-)
> >> > > > >> > > > >>
> >> > > > >> > > > >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> >> > > > >> > > > >> index 519a16ed98..039649e522 100644
> >> > > > >> > > > >> --- a/hw/arm/xlnx-zcu102.c
> >> > > > >> > > > >> +++ b/hw/arm/xlnx-zcu102.c
> >> > > > >> > > > >> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
> >> > > > >> > > > >>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> >> > > > >> > > > >>                                &error_abort);
> >> > > > >> > > > >>
> >> > > > >> > > > >> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> >> > > > >> > > > >> +                            &error_fatal);  
> >> > > > >> > > > >
> >> > > > >> > > > > Do you have plans to support other CPU types to xlnx_zynqmp in
> >> > > > >> > > > > the future?  If not, I wouldn't bother adding the cpu-type
> >> > > > >> > > > > property and the extra boilerplate code if it's always going to
> >> > > > >> > > > > be set to cortex-a53.  
> >> > > > >> > > >
> >> > > > >> > > > No, it'll always be A53.
> >> > > > >> > > >
> >> > > > >> > > > I did think of that, but I also wanted to use the new option! I also
> >> > > > >> > > > think there is an advantage in sanely handling users '-cpu' option,
> >> > > > >> > > > before now we just ignored it, so I think it still does give a
> >> > > > >> > > > benefit. That'll be especially important on the Xilinx tree (sometimes
> >> > > > >> > > > people use our machines with a different CPU to 'benchmark' or test
> >> > > > >> > > > other CPUs with our CoSimulation setup). So I think it does make sense
> >> > > > >> > > > to keep in.  
> >> > > > >> > > if cpu isn't user settable, one could just outright die if cpu_type
> >> > > > >> > > is not NULL and say that user's CLI is wrong.
> >> > > > >> > > (i.e. don't give users illusion that they allowed to use '-cpu')  
> >> > > > >> >
> >> > > > >> > Isn't it exactly what this patch does, by setting:
> >> > > > >> >     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> >> > > > >> >     mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
> >> > > > >> > ?
> >> > > > >> >
> >> > > > >> > Except that "-cpu cortex-a53" won't die, which is a good thing.  
> >> > > > >> allowing "-cpu cortex-a53" here, would allow to use feature parsing
> >> > > > >> which weren't allowed or were ignored before if user supplied '-cpu'.
> >> > > > >> so I'd more strict and refuse any -cpu and break CLI that tries to use it
> >> > > > >> if board has non configurable cpu type. It would be easier to relax
> >> > > > >> restriction later if necessary.
> >> > > > >>
> >> > > > >> using validate_cpus here just to have users for the new code,
> >> > > > >> doesn't seem like valid justification and at that it makes board
> >> > > > >> code more complex where it's not necessary and build in cpu type
> >> > > > >> works just fine.  
> >> > > > >
> >> > > > > It's up to the board maintainer to decide what's the best option.
> >> > > > > Both features are independent from each other and can be
> >> > > > > implemented by machine core.  
> >> > > >
> >> > > > Noooo!
> >> > > >
> >> > > > My hope with this series is that eventually we could hit a state where
> >> > > > every single machine acts the same way with the -cpu option.
> >> > > >
> >> > > > I really don't like what we do now where some boards use it, some
> >> > > > boards error and some boars just ignore the option. I think we should
> >> > > > agree on something and every machine should follow the same flow so
> >> > > > that users know what to expect when they use the -cpu option.
> >> > > >
> >> > > > If this means we allow machines to specify they don't support the
> >> > > > option or only have a single element in the list of supported options
> >> > > > doesn't really matter, but all machines should do the same thing.
> >> > > >  
> >> > > > >
> >> > > > > In either case, the valid_cpu_types feature will be still very
> >> > > > > useful for boards like pxa270 and sa1110, which support -cpu but
> >> > > > > only with specific families of CPU types (grep for
> >> > > > > "strncmp(cpu_type").
> >> > > > >  
> >> > > > >>
> >> > > > >> wrt centralized way to refuse -cpu if board doesn't support it,
> >> > > > >> (which is not really related to this series) following could be done:
> >> > > > >>
> >> > > > >> when cpu_model removal is completely done I plan to replace
> >> > > > >>   vl.c
> >> > > > >>      cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model)
> >> > > > >> with
> >> > > > >>      cpu_parse_cpu_model(DEFAULT_TARGET_CPU_TYPE, cpu_model)
> >> > > > >>
> >> > > > >> so that we could drop temporary guard
> >> > > > >>
> >> > > > >>      if (machine_class->default_cpu_type) {  
> >> > > > >
> >> > > > > This sounds good to me, even if we don't reject -cpu on any
> >> > > > > board.
> >> > > > >
> >> > > > >  
> >> > > > >>
> >> > > > >> with that it would be possible to tell from machine_run_board_init()
> >> > > > >> that board doesn't provide cpu but user provided '-cpu'
> >> > > > >> so we would be able to:
> >> > > > >>   if ((machine_class->default_cpu_type == NULL) &&
> >> > > > >>       (machine->cpu_type != NULL))
> >> > > > >>           error_fatal("machine doesn't support -cpu option");  
> >> > > > >
> >> > > > > I won't complain too much if a board maintainer really wants to
> >> > > > > make the board reject -cpu completely, but it's up to them.  
> >> > > >
> >> > > > I disagree. I think a standard way of doing it is better. At least for
> >> > > > each architecture. The ARM -cpu option is very confusing at the moment
> >> > > > and it really doesn't need to be that bad.
> >> > > >  
> >> > > > >
> >> > > > > Personally, I'd prefer to have all boards setting
> >> > > > > default_cpu_type even if they support only one CPU model, so
> >> > > > > clients don't need a special case for boards that don't support
> >> > > > > -cpu.  
> >> > > >
> >> > > > I agree, I think having one CPU makes more sense. It makes it easier
> >> > > > to add support for more cpus in the future and allows the users to use
> >> > > > the -cpu option without killing QEMU.  
> >> > > I'm considering -cpu option as a legacy one that server 2 purposes now  
> >> >
> >> > I'm not sure about "legacy", but the list of purposes looks
> >> > accurate:
> >> >  
> >> > >  1: pick cpu type for running instance  
> >> >
> >> > This one has no replacement yet, so can we really call it legacy?  
> >> not really, it's not going anywhere in near future
> >>  
> >> >  
> >> > >  2: convert optional features/legacy syntax to global properties
> >> > >     for related cpu type  
> >> >
> >> > This one has a replacement: -global.  But there's a difference
> >> > between saying "-cpu features are implemented using -global" and
> >> > "-cpu features are obsoleted by -global".  I don't think we can
> >> > say it's obsolete or legacy unless existing management software
> >> > is changed to be using something else.
> >> >
> >> >  
> >> > >
> >> > > It plays ok for machines with single type of cpu but doesn't really scale
> >> > > to more and doesn't work well nor needed if we were to specify cpus on CLI
> >> > > with -device (i.e. build machine from config/CLI)  
> >> >
> >> > This is a good point.  But -cpu is still a useful shortcut for
> >> > boards that have a single CPU type.  What are the arguments we
> >> > have to get rid of it completely?  
> >> boards that have single cpu type don't need -cpu. since cpu is not
> >> configurable there.  
> >
> > They don't need -cpu, but there's no need to reject "-cpu FOO" if
> > we know FOO is the CPU model used by the board.  This is the only
> > difference between what you propose and what Alistair proposes,
> > right?
> >
> >  
> >>
> >>  
> >> > > So I would not extend usage '-cpu' to boards that have fixed cpu type,
> >> > > because it really useless in that case and confuses users with idea that
> >> > > they have ability/need to specify -cpu on fixed cpu board.  
> >> >
> >> > If they try to choose any other CPU model, they will see an error
> >> > message explicitly saying only one CPU type is supported.  What
> >> > would be the harm?  
> >> I guess I've already pointed drawbacks from interface point of view,
> >> from maintainer pov it will be extra code to maintain valid cpus
> >> vs just 'create_cpu(MY_CPU_TYPE)'
> >> this patch is vivid example of the case  
> >
> > With this part I agree.  We don't need to add boilerplate code to
> > board init if the CPU model will always be the same.
> >
> > But I would still prefer to do this:
> >
> >   create_cpu(MY_CPU_TYPE);  // at XXX_init()
> > [...]
> >   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 = { MY_CPU_TYPE, NULL };
> >   }  
> 
> I like this option. It doesn't add much code and I think makes it very
> clear to users.
> 
> Another thing to point out is that I see users specifying options to
> QEMU all the time that QEMU will just ignore. Imagine people see
> somewhere online that others use '-cpu' and suddenly they think they
> have to. Having this throw an error that '-cpu' isn't supported in
> this case (but is in others) will create confusion of when it
> should/shouldn't be use. I think always allowing it and telling users
> the supported CPUs clears this up.

patch would look better with what Eduardo suggested above.
at least it will minimize amount of not need code, so I'd go for it.

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zcu102: Specify the valid CPUs

[Eduardo Habkost]

On Mon, Oct 09, 2017 at 09:12:29AM +0200, Igor Mammedov wrote:
> On Fri, 6 Oct 2017 15:06:57 -0700
> Alistair Francis <alistair.francis@xilinx.com> wrote:
> 
> > On Fri, Oct 6, 2017 at 4:45 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > On Fri, Oct 06, 2017 at 10:23:12AM +0200, Igor Mammedov wrote:  
> > >> On Thu, 5 Oct 2017 14:09:06 -0300
> > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >>  
> > >> > On Thu, Oct 05, 2017 at 11:04:27AM +0200, Igor Mammedov wrote:  
> > >> > > On Wed, 4 Oct 2017 14:39:20 -0700
> > >> > > Alistair Francis <alistair.francis@xilinx.com> wrote:
> > >> > >  
> > >> > > > On Wed, Oct 4, 2017 at 9:34 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:  
> > >> > > > > On Wed, Oct 04, 2017 at 03:08:16PM +0200, Igor Mammedov wrote:  
> > >> > > > >> On Wed, 4 Oct 2017 09:28:51 -0300
> > >> > > > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >> > > > >>  
> > >> > > > >> > On Wed, Oct 04, 2017 at 01:12:32PM +0200, Igor Mammedov wrote:  
> > >> > > > >> > > On Tue, 3 Oct 2017 14:41:17 -0700
> > >> > > > >> > > Alistair Francis <alistair.francis@xilinx.com> wrote:
> > >> > > > >> > >  
> > >> > > > >> > > > On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:  
> > >> > > > >> > > > > On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:  
> > >> > > > >> > > > >> List all possible valid CPU options.
> > >> > > > >> > > > >>
> > >> > > > >> > > > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > >> > > > >> > > > >> ---
> > >> > > > >> > > > >>
> > >> > > > >> > > > >>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
> > >> > > > >> > > > >>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
> > >> > > > >> > > > >>  include/hw/arm/xlnx-zynqmp.h |  1 +
> > >> > > > >> > > > >>  3 files changed, 20 insertions(+), 7 deletions(-)
> > >> > > > >> > > > >>
> > >> > > > >> > > > >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> > >> > > > >> > > > >> index 519a16ed98..039649e522 100644
> > >> > > > >> > > > >> --- a/hw/arm/xlnx-zcu102.c
> > >> > > > >> > > > >> +++ b/hw/arm/xlnx-zcu102.c
> > >> > > > >> > > > >> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
> > >> > > > >> > > > >>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> > >> > > > >> > > > >>                                &error_abort);
> > >> > > > >> > > > >>
> > >> > > > >> > > > >> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> > >> > > > >> > > > >> +                            &error_fatal);  
> > >> > > > >> > > > >
> > >> > > > >> > > > > Do you have plans to support other CPU types to xlnx_zynqmp in
> > >> > > > >> > > > > the future?  If not, I wouldn't bother adding the cpu-type
> > >> > > > >> > > > > property and the extra boilerplate code if it's always going to
> > >> > > > >> > > > > be set to cortex-a53.  
> > >> > > > >> > > >
> > >> > > > >> > > > No, it'll always be A53.
> > >> > > > >> > > >
> > >> > > > >> > > > I did think of that, but I also wanted to use the new option! I also
> > >> > > > >> > > > think there is an advantage in sanely handling users '-cpu' option,
> > >> > > > >> > > > before now we just ignored it, so I think it still does give a
> > >> > > > >> > > > benefit. That'll be especially important on the Xilinx tree (sometimes
> > >> > > > >> > > > people use our machines with a different CPU to 'benchmark' or test
> > >> > > > >> > > > other CPUs with our CoSimulation setup). So I think it does make sense
> > >> > > > >> > > > to keep in.  
> > >> > > > >> > > if cpu isn't user settable, one could just outright die if cpu_type
> > >> > > > >> > > is not NULL and say that user's CLI is wrong.
> > >> > > > >> > > (i.e. don't give users illusion that they allowed to use '-cpu')  
> > >> > > > >> >
> > >> > > > >> > Isn't it exactly what this patch does, by setting:
> > >> > > > >> >     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> > >> > > > >> >     mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
> > >> > > > >> > ?
> > >> > > > >> >
> > >> > > > >> > Except that "-cpu cortex-a53" won't die, which is a good thing.  
> > >> > > > >> allowing "-cpu cortex-a53" here, would allow to use feature parsing
> > >> > > > >> which weren't allowed or were ignored before if user supplied '-cpu'.
> > >> > > > >> so I'd more strict and refuse any -cpu and break CLI that tries to use it
> > >> > > > >> if board has non configurable cpu type. It would be easier to relax
> > >> > > > >> restriction later if necessary.
> > >> > > > >>
> > >> > > > >> using validate_cpus here just to have users for the new code,
> > >> > > > >> doesn't seem like valid justification and at that it makes board
> > >> > > > >> code more complex where it's not necessary and build in cpu type
> > >> > > > >> works just fine.  
> > >> > > > >
> > >> > > > > It's up to the board maintainer to decide what's the best option.
> > >> > > > > Both features are independent from each other and can be
> > >> > > > > implemented by machine core.  
> > >> > > >
> > >> > > > Noooo!
> > >> > > >
> > >> > > > My hope with this series is that eventually we could hit a state where
> > >> > > > every single machine acts the same way with the -cpu option.
> > >> > > >
> > >> > > > I really don't like what we do now where some boards use it, some
> > >> > > > boards error and some boars just ignore the option. I think we should
> > >> > > > agree on something and every machine should follow the same flow so
> > >> > > > that users know what to expect when they use the -cpu option.
> > >> > > >
> > >> > > > If this means we allow machines to specify they don't support the
> > >> > > > option or only have a single element in the list of supported options
> > >> > > > doesn't really matter, but all machines should do the same thing.
> > >> > > >  
> > >> > > > >
> > >> > > > > In either case, the valid_cpu_types feature will be still very
> > >> > > > > useful for boards like pxa270 and sa1110, which support -cpu but
> > >> > > > > only with specific families of CPU types (grep for
> > >> > > > > "strncmp(cpu_type").
> > >> > > > >  
> > >> > > > >>
> > >> > > > >> wrt centralized way to refuse -cpu if board doesn't support it,
> > >> > > > >> (which is not really related to this series) following could be done:
> > >> > > > >>
> > >> > > > >> when cpu_model removal is completely done I plan to replace
> > >> > > > >>   vl.c
> > >> > > > >>      cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model)
> > >> > > > >> with
> > >> > > > >>      cpu_parse_cpu_model(DEFAULT_TARGET_CPU_TYPE, cpu_model)
> > >> > > > >>
> > >> > > > >> so that we could drop temporary guard
> > >> > > > >>
> > >> > > > >>      if (machine_class->default_cpu_type) {  
> > >> > > > >
> > >> > > > > This sounds good to me, even if we don't reject -cpu on any
> > >> > > > > board.
> > >> > > > >
> > >> > > > >  
> > >> > > > >>
> > >> > > > >> with that it would be possible to tell from machine_run_board_init()
> > >> > > > >> that board doesn't provide cpu but user provided '-cpu'
> > >> > > > >> so we would be able to:
> > >> > > > >>   if ((machine_class->default_cpu_type == NULL) &&
> > >> > > > >>       (machine->cpu_type != NULL))
> > >> > > > >>           error_fatal("machine doesn't support -cpu option");  
> > >> > > > >
> > >> > > > > I won't complain too much if a board maintainer really wants to
> > >> > > > > make the board reject -cpu completely, but it's up to them.  
> > >> > > >
> > >> > > > I disagree. I think a standard way of doing it is better. At least for
> > >> > > > each architecture. The ARM -cpu option is very confusing at the moment
> > >> > > > and it really doesn't need to be that bad.
> > >> > > >  
> > >> > > > >
> > >> > > > > Personally, I'd prefer to have all boards setting
> > >> > > > > default_cpu_type even if they support only one CPU model, so
> > >> > > > > clients don't need a special case for boards that don't support
> > >> > > > > -cpu.  
> > >> > > >
> > >> > > > I agree, I think having one CPU makes more sense. It makes it easier
> > >> > > > to add support for more cpus in the future and allows the users to use
> > >> > > > the -cpu option without killing QEMU.  
> > >> > > I'm considering -cpu option as a legacy one that server 2 purposes now  
> > >> >
> > >> > I'm not sure about "legacy", but the list of purposes looks
> > >> > accurate:
> > >> >  
> > >> > >  1: pick cpu type for running instance  
> > >> >
> > >> > This one has no replacement yet, so can we really call it legacy?  
> > >> not really, it's not going anywhere in near future
> > >>  
> > >> >  
> > >> > >  2: convert optional features/legacy syntax to global properties
> > >> > >     for related cpu type  
> > >> >
> > >> > This one has a replacement: -global.  But there's a difference
> > >> > between saying "-cpu features are implemented using -global" and
> > >> > "-cpu features are obsoleted by -global".  I don't think we can
> > >> > say it's obsolete or legacy unless existing management software
> > >> > is changed to be using something else.
> > >> >
> > >> >  
> > >> > >
> > >> > > It plays ok for machines with single type of cpu but doesn't really scale
> > >> > > to more and doesn't work well nor needed if we were to specify cpus on CLI
> > >> > > with -device (i.e. build machine from config/CLI)  
> > >> >
> > >> > This is a good point.  But -cpu is still a useful shortcut for
> > >> > boards that have a single CPU type.  What are the arguments we
> > >> > have to get rid of it completely?  
> > >> boards that have single cpu type don't need -cpu. since cpu is not
> > >> configurable there.  
> > >
> > > They don't need -cpu, but there's no need to reject "-cpu FOO" if
> > > we know FOO is the CPU model used by the board.  This is the only
> > > difference between what you propose and what Alistair proposes,
> > > right?
> > >
> > >  
> > >>
> > >>  
> > >> > > So I would not extend usage '-cpu' to boards that have fixed cpu type,
> > >> > > because it really useless in that case and confuses users with idea that
> > >> > > they have ability/need to specify -cpu on fixed cpu board.  
> > >> >
> > >> > If they try to choose any other CPU model, they will see an error
> > >> > message explicitly saying only one CPU type is supported.  What
> > >> > would be the harm?  
> > >> I guess I've already pointed drawbacks from interface point of view,
> > >> from maintainer pov it will be extra code to maintain valid cpus
> > >> vs just 'create_cpu(MY_CPU_TYPE)'
> > >> this patch is vivid example of the case  
> > >
> > > With this part I agree.  We don't need to add boilerplate code to
> > > board init if the CPU model will always be the same.
> > >
> > > But I would still prefer to do this:
> > >
> > >   create_cpu(MY_CPU_TYPE);  // at XXX_init()
> > > [...]
> > >   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 = { MY_CPU_TYPE, NULL };
> > >   }  
> > 
> > I like this option. It doesn't add much code and I think makes it very
> > clear to users.
> > 
> > Another thing to point out is that I see users specifying options to
> > QEMU all the time that QEMU will just ignore. Imagine people see
> > somewhere online that others use '-cpu' and suddenly they think they
> > have to. Having this throw an error that '-cpu' isn't supported in
> > this case (but is in others) will create confusion of when it
> > should/shouldn't be use. I think always allowing it and telling users
> > the supported CPUs clears this up.
> 
> patch would look better with what Eduardo suggested above.
> at least it will minimize amount of not need code, so I'd go for it.

I just see one problem: I don't see an easy way for setting:
  mc->valid_cpu_types = { MY_CPU_TYPE, NULL };
without one additional static variable for holding the array.  So
my claim about "only 2 lines of code" is not accurate.

But we might do this to make the code shorter and simpler on
boards like xlnx_zynqmp:

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


-- 
Eduardo

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zcu102: Specify the valid CPUs

[Alistair Francis]

On Tue, Oct 10, 2017 at 8:25 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Oct 09, 2017 at 09:12:29AM +0200, Igor Mammedov wrote:
>> On Fri, 6 Oct 2017 15:06:57 -0700
>> Alistair Francis <alistair.francis@xilinx.com> wrote:
>>
>> > On Fri, Oct 6, 2017 at 4:45 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > > On Fri, Oct 06, 2017 at 10:23:12AM +0200, Igor Mammedov wrote:
>> > >> On Thu, 5 Oct 2017 14:09:06 -0300
>> > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > >>
>> > >> > On Thu, Oct 05, 2017 at 11:04:27AM +0200, Igor Mammedov wrote:
>> > >> > > On Wed, 4 Oct 2017 14:39:20 -0700
>> > >> > > Alistair Francis <alistair.francis@xilinx.com> wrote:
>> > >> > >
>> > >> > > > On Wed, Oct 4, 2017 at 9:34 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > >> > > > > On Wed, Oct 04, 2017 at 03:08:16PM +0200, Igor Mammedov wrote:
>> > >> > > > >> On Wed, 4 Oct 2017 09:28:51 -0300
>> > >> > > > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > >> > > > >>
>> > >> > > > >> > On Wed, Oct 04, 2017 at 01:12:32PM +0200, Igor Mammedov wrote:
>> > >> > > > >> > > On Tue, 3 Oct 2017 14:41:17 -0700
>> > >> > > > >> > > Alistair Francis <alistair.francis@xilinx.com> wrote:
>> > >> > > > >> > >
>> > >> > > > >> > > > On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > >> > > > >> > > > > On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:
>> > >> > > > >> > > > >> List all possible valid CPU options.
>> > >> > > > >> > > > >>
>> > >> > > > >> > > > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> > >> > > > >> > > > >> ---
>> > >> > > > >> > > > >>
>> > >> > > > >> > > > >>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
>> > >> > > > >> > > > >>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
>> > >> > > > >> > > > >>  include/hw/arm/xlnx-zynqmp.h |  1 +
>> > >> > > > >> > > > >>  3 files changed, 20 insertions(+), 7 deletions(-)
>> > >> > > > >> > > > >>
>> > >> > > > >> > > > >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
>> > >> > > > >> > > > >> index 519a16ed98..039649e522 100644
>> > >> > > > >> > > > >> --- a/hw/arm/xlnx-zcu102.c
>> > >> > > > >> > > > >> +++ b/hw/arm/xlnx-zcu102.c
>> > >> > > > >> > > > >> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
>> > >> > > > >> > > > >>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
>> > >> > > > >> > > > >>                                &error_abort);
>> > >> > > > >> > > > >>
>> > >> > > > >> > > > >> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
>> > >> > > > >> > > > >> +                            &error_fatal);
>> > >> > > > >> > > > >
>> > >> > > > >> > > > > Do you have plans to support other CPU types to xlnx_zynqmp in
>> > >> > > > >> > > > > the future?  If not, I wouldn't bother adding the cpu-type
>> > >> > > > >> > > > > property and the extra boilerplate code if it's always going to
>> > >> > > > >> > > > > be set to cortex-a53.
>> > >> > > > >> > > >
>> > >> > > > >> > > > No, it'll always be A53.
>> > >> > > > >> > > >
>> > >> > > > >> > > > I did think of that, but I also wanted to use the new option! I also
>> > >> > > > >> > > > think there is an advantage in sanely handling users '-cpu' option,
>> > >> > > > >> > > > before now we just ignored it, so I think it still does give a
>> > >> > > > >> > > > benefit. That'll be especially important on the Xilinx tree (sometimes
>> > >> > > > >> > > > people use our machines with a different CPU to 'benchmark' or test
>> > >> > > > >> > > > other CPUs with our CoSimulation setup). So I think it does make sense
>> > >> > > > >> > > > to keep in.
>> > >> > > > >> > > if cpu isn't user settable, one could just outright die if cpu_type
>> > >> > > > >> > > is not NULL and say that user's CLI is wrong.
>> > >> > > > >> > > (i.e. don't give users illusion that they allowed to use '-cpu')
>> > >> > > > >> >
>> > >> > > > >> > Isn't it exactly what this patch does, by setting:
>> > >> > > > >> >     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
>> > >> > > > >> >     mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
>> > >> > > > >> > ?
>> > >> > > > >> >
>> > >> > > > >> > Except that "-cpu cortex-a53" won't die, which is a good thing.
>> > >> > > > >> allowing "-cpu cortex-a53" here, would allow to use feature parsing
>> > >> > > > >> which weren't allowed or were ignored before if user supplied '-cpu'.
>> > >> > > > >> so I'd more strict and refuse any -cpu and break CLI that tries to use it
>> > >> > > > >> if board has non configurable cpu type. It would be easier to relax
>> > >> > > > >> restriction later if necessary.
>> > >> > > > >>
>> > >> > > > >> using validate_cpus here just to have users for the new code,
>> > >> > > > >> doesn't seem like valid justification and at that it makes board
>> > >> > > > >> code more complex where it's not necessary and build in cpu type
>> > >> > > > >> works just fine.
>> > >> > > > >
>> > >> > > > > It's up to the board maintainer to decide what's the best option.
>> > >> > > > > Both features are independent from each other and can be
>> > >> > > > > implemented by machine core.
>> > >> > > >
>> > >> > > > Noooo!
>> > >> > > >
>> > >> > > > My hope with this series is that eventually we could hit a state where
>> > >> > > > every single machine acts the same way with the -cpu option.
>> > >> > > >
>> > >> > > > I really don't like what we do now where some boards use it, some
>> > >> > > > boards error and some boars just ignore the option. I think we should
>> > >> > > > agree on something and every machine should follow the same flow so
>> > >> > > > that users know what to expect when they use the -cpu option.
>> > >> > > >
>> > >> > > > If this means we allow machines to specify they don't support the
>> > >> > > > option or only have a single element in the list of supported options
>> > >> > > > doesn't really matter, but all machines should do the same thing.
>> > >> > > >
>> > >> > > > >
>> > >> > > > > In either case, the valid_cpu_types feature will be still very
>> > >> > > > > useful for boards like pxa270 and sa1110, which support -cpu but
>> > >> > > > > only with specific families of CPU types (grep for
>> > >> > > > > "strncmp(cpu_type").
>> > >> > > > >
>> > >> > > > >>
>> > >> > > > >> wrt centralized way to refuse -cpu if board doesn't support it,
>> > >> > > > >> (which is not really related to this series) following could be done:
>> > >> > > > >>
>> > >> > > > >> when cpu_model removal is completely done I plan to replace
>> > >> > > > >>   vl.c
>> > >> > > > >>      cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model)
>> > >> > > > >> with
>> > >> > > > >>      cpu_parse_cpu_model(DEFAULT_TARGET_CPU_TYPE, cpu_model)
>> > >> > > > >>
>> > >> > > > >> so that we could drop temporary guard
>> > >> > > > >>
>> > >> > > > >>      if (machine_class->default_cpu_type) {
>> > >> > > > >
>> > >> > > > > This sounds good to me, even if we don't reject -cpu on any
>> > >> > > > > board.
>> > >> > > > >
>> > >> > > > >
>> > >> > > > >>
>> > >> > > > >> with that it would be possible to tell from machine_run_board_init()
>> > >> > > > >> that board doesn't provide cpu but user provided '-cpu'
>> > >> > > > >> so we would be able to:
>> > >> > > > >>   if ((machine_class->default_cpu_type == NULL) &&
>> > >> > > > >>       (machine->cpu_type != NULL))
>> > >> > > > >>           error_fatal("machine doesn't support -cpu option");
>> > >> > > > >
>> > >> > > > > I won't complain too much if a board maintainer really wants to
>> > >> > > > > make the board reject -cpu completely, but it's up to them.
>> > >> > > >
>> > >> > > > I disagree. I think a standard way of doing it is better. At least for
>> > >> > > > each architecture. The ARM -cpu option is very confusing at the moment
>> > >> > > > and it really doesn't need to be that bad.
>> > >> > > >
>> > >> > > > >
>> > >> > > > > Personally, I'd prefer to have all boards setting
>> > >> > > > > default_cpu_type even if they support only one CPU model, so
>> > >> > > > > clients don't need a special case for boards that don't support
>> > >> > > > > -cpu.
>> > >> > > >
>> > >> > > > I agree, I think having one CPU makes more sense. It makes it easier
>> > >> > > > to add support for more cpus in the future and allows the users to use
>> > >> > > > the -cpu option without killing QEMU.
>> > >> > > I'm considering -cpu option as a legacy one that server 2 purposes now
>> > >> >
>> > >> > I'm not sure about "legacy", but the list of purposes looks
>> > >> > accurate:
>> > >> >
>> > >> > >  1: pick cpu type for running instance
>> > >> >
>> > >> > This one has no replacement yet, so can we really call it legacy?
>> > >> not really, it's not going anywhere in near future
>> > >>
>> > >> >
>> > >> > >  2: convert optional features/legacy syntax to global properties
>> > >> > >     for related cpu type
>> > >> >
>> > >> > This one has a replacement: -global.  But there's a difference
>> > >> > between saying "-cpu features are implemented using -global" and
>> > >> > "-cpu features are obsoleted by -global".  I don't think we can
>> > >> > say it's obsolete or legacy unless existing management software
>> > >> > is changed to be using something else.
>> > >> >
>> > >> >
>> > >> > >
>> > >> > > It plays ok for machines with single type of cpu but doesn't really scale
>> > >> > > to more and doesn't work well nor needed if we were to specify cpus on CLI
>> > >> > > with -device (i.e. build machine from config/CLI)
>> > >> >
>> > >> > This is a good point.  But -cpu is still a useful shortcut for
>> > >> > boards that have a single CPU type.  What are the arguments we
>> > >> > have to get rid of it completely?
>> > >> boards that have single cpu type don't need -cpu. since cpu is not
>> > >> configurable there.
>> > >
>> > > They don't need -cpu, but there's no need to reject "-cpu FOO" if
>> > > we know FOO is the CPU model used by the board.  This is the only
>> > > difference between what you propose and what Alistair proposes,
>> > > right?
>> > >
>> > >
>> > >>
>> > >>
>> > >> > > So I would not extend usage '-cpu' to boards that have fixed cpu type,
>> > >> > > because it really useless in that case and confuses users with idea that
>> > >> > > they have ability/need to specify -cpu on fixed cpu board.
>> > >> >
>> > >> > If they try to choose any other CPU model, they will see an error
>> > >> > message explicitly saying only one CPU type is supported.  What
>> > >> > would be the harm?
>> > >> I guess I've already pointed drawbacks from interface point of view,
>> > >> from maintainer pov it will be extra code to maintain valid cpus
>> > >> vs just 'create_cpu(MY_CPU_TYPE)'
>> > >> this patch is vivid example of the case
>> > >
>> > > With this part I agree.  We don't need to add boilerplate code to
>> > > board init if the CPU model will always be the same.
>> > >
>> > > But I would still prefer to do this:
>> > >
>> > >   create_cpu(MY_CPU_TYPE);  // at XXX_init()
>> > > [...]
>> > >   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 = { MY_CPU_TYPE, NULL };
>> > >   }
>> >
>> > I like this option. It doesn't add much code and I think makes it very
>> > clear to users.
>> >
>> > Another thing to point out is that I see users specifying options to
>> > QEMU all the time that QEMU will just ignore. Imagine people see
>> > somewhere online that others use '-cpu' and suddenly they think they
>> > have to. Having this throw an error that '-cpu' isn't supported in
>> > this case (but is in others) will create confusion of when it
>> > should/shouldn't be use. I think always allowing it and telling users
>> > the supported CPUs clears this up.
>>
>> patch would look better with what Eduardo suggested above.
>> at least it will minimize amount of not need code, so I'd go for it.
>
> I just see one problem: I don't see an easy way for setting:
>   mc->valid_cpu_types = { MY_CPU_TYPE, NULL };
> without one additional static variable for holding the array.  So
> my claim about "only 2 lines of code" is not accurate.
>
> But we might do this to make the code shorter and simpler on
> boards like xlnx_zynqmp:
>
> 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;
>    }

This is fine with me.

I had prepared a patch series with your earlier approach and was about
to send it out before I saw this email. I was then going to wait until
something was decided but I think I'm just going to send my series out
anyway. It has a fix for the wrong CPU in the Raspberry Pi 2 which I
think should go in now.

We can still continue this discussion though.

Thanks,
Alistair

>
>
> --
> Eduardo
>

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zcu102: Specify the valid CPUs

[Eduardo Habkost]

On Thu, Oct 12, 2017 at 04:59:48PM -0700, Alistair Francis wrote:
> On Tue, Oct 10, 2017 at 8:25 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Mon, Oct 09, 2017 at 09:12:29AM +0200, Igor Mammedov wrote:
> >> On Fri, 6 Oct 2017 15:06:57 -0700
> >> Alistair Francis <alistair.francis@xilinx.com> wrote:
> >>
> >> > On Fri, Oct 6, 2017 at 4:45 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> > > On Fri, Oct 06, 2017 at 10:23:12AM +0200, Igor Mammedov wrote:
> >> > >> On Thu, 5 Oct 2017 14:09:06 -0300
> >> > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> > >>
> >> > >> > On Thu, Oct 05, 2017 at 11:04:27AM +0200, Igor Mammedov wrote:
> >> > >> > > On Wed, 4 Oct 2017 14:39:20 -0700
> >> > >> > > Alistair Francis <alistair.francis@xilinx.com> wrote:
> >> > >> > >
> >> > >> > > > On Wed, Oct 4, 2017 at 9:34 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> > >> > > > > On Wed, Oct 04, 2017 at 03:08:16PM +0200, Igor Mammedov wrote:
> >> > >> > > > >> On Wed, 4 Oct 2017 09:28:51 -0300
> >> > >> > > > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> > >> > > > >>
> >> > >> > > > >> > On Wed, Oct 04, 2017 at 01:12:32PM +0200, Igor Mammedov wrote:
> >> > >> > > > >> > > On Tue, 3 Oct 2017 14:41:17 -0700
> >> > >> > > > >> > > Alistair Francis <alistair.francis@xilinx.com> wrote:
> >> > >> > > > >> > >
> >> > >> > > > >> > > > On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> > >> > > > >> > > > > On Tue, Oct 03, 2017 at 01:05:13PM -0700, Alistair Francis wrote:
> >> > >> > > > >> > > > >> List all possible valid CPU options.
> >> > >> > > > >> > > > >>
> >> > >> > > > >> > > > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >> > >> > > > >> > > > >> ---
> >> > >> > > > >> > > > >>
> >> > >> > > > >> > > > >>  hw/arm/xlnx-zcu102.c         | 10 ++++++++++
> >> > >> > > > >> > > > >>  hw/arm/xlnx-zynqmp.c         | 16 +++++++++-------
> >> > >> > > > >> > > > >>  include/hw/arm/xlnx-zynqmp.h |  1 +
> >> > >> > > > >> > > > >>  3 files changed, 20 insertions(+), 7 deletions(-)
> >> > >> > > > >> > > > >>
> >> > >> > > > >> > > > >> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> >> > >> > > > >> > > > >> index 519a16ed98..039649e522 100644
> >> > >> > > > >> > > > >> --- a/hw/arm/xlnx-zcu102.c
> >> > >> > > > >> > > > >> +++ b/hw/arm/xlnx-zcu102.c
> >> > >> > > > >> > > > >> @@ -98,6 +98,8 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
> >> > >> > > > >> > > > >>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> >> > >> > > > >> > > > >>                                &error_abort);
> >> > >> > > > >> > > > >>
> >> > >> > > > >> > > > >> +    object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
> >> > >> > > > >> > > > >> +                            &error_fatal);
> >> > >> > > > >> > > > >
> >> > >> > > > >> > > > > Do you have plans to support other CPU types to xlnx_zynqmp in
> >> > >> > > > >> > > > > the future?  If not, I wouldn't bother adding the cpu-type
> >> > >> > > > >> > > > > property and the extra boilerplate code if it's always going to
> >> > >> > > > >> > > > > be set to cortex-a53.
> >> > >> > > > >> > > >
> >> > >> > > > >> > > > No, it'll always be A53.
> >> > >> > > > >> > > >
> >> > >> > > > >> > > > I did think of that, but I also wanted to use the new option! I also
> >> > >> > > > >> > > > think there is an advantage in sanely handling users '-cpu' option,
> >> > >> > > > >> > > > before now we just ignored it, so I think it still does give a
> >> > >> > > > >> > > > benefit. That'll be especially important on the Xilinx tree (sometimes
> >> > >> > > > >> > > > people use our machines with a different CPU to 'benchmark' or test
> >> > >> > > > >> > > > other CPUs with our CoSimulation setup). So I think it does make sense
> >> > >> > > > >> > > > to keep in.
> >> > >> > > > >> > > if cpu isn't user settable, one could just outright die if cpu_type
> >> > >> > > > >> > > is not NULL and say that user's CLI is wrong.
> >> > >> > > > >> > > (i.e. don't give users illusion that they allowed to use '-cpu')
> >> > >> > > > >> >
> >> > >> > > > >> > Isn't it exactly what this patch does, by setting:
> >> > >> > > > >> >     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> >> > >> > > > >> >     mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
> >> > >> > > > >> > ?
> >> > >> > > > >> >
> >> > >> > > > >> > Except that "-cpu cortex-a53" won't die, which is a good thing.
> >> > >> > > > >> allowing "-cpu cortex-a53" here, would allow to use feature parsing
> >> > >> > > > >> which weren't allowed or were ignored before if user supplied '-cpu'.
> >> > >> > > > >> so I'd more strict and refuse any -cpu and break CLI that tries to use it
> >> > >> > > > >> if board has non configurable cpu type. It would be easier to relax
> >> > >> > > > >> restriction later if necessary.
> >> > >> > > > >>
> >> > >> > > > >> using validate_cpus here just to have users for the new code,
> >> > >> > > > >> doesn't seem like valid justification and at that it makes board
> >> > >> > > > >> code more complex where it's not necessary and build in cpu type
> >> > >> > > > >> works just fine.
> >> > >> > > > >
> >> > >> > > > > It's up to the board maintainer to decide what's the best option.
> >> > >> > > > > Both features are independent from each other and can be
> >> > >> > > > > implemented by machine core.
> >> > >> > > >
> >> > >> > > > Noooo!
> >> > >> > > >
> >> > >> > > > My hope with this series is that eventually we could hit a state where
> >> > >> > > > every single machine acts the same way with the -cpu option.
> >> > >> > > >
> >> > >> > > > I really don't like what we do now where some boards use it, some
> >> > >> > > > boards error and some boars just ignore the option. I think we should
> >> > >> > > > agree on something and every machine should follow the same flow so
> >> > >> > > > that users know what to expect when they use the -cpu option.
> >> > >> > > >
> >> > >> > > > If this means we allow machines to specify they don't support the
> >> > >> > > > option or only have a single element in the list of supported options
> >> > >> > > > doesn't really matter, but all machines should do the same thing.
> >> > >> > > >
> >> > >> > > > >
> >> > >> > > > > In either case, the valid_cpu_types feature will be still very
> >> > >> > > > > useful for boards like pxa270 and sa1110, which support -cpu but
> >> > >> > > > > only with specific families of CPU types (grep for
> >> > >> > > > > "strncmp(cpu_type").
> >> > >> > > > >
> >> > >> > > > >>
> >> > >> > > > >> wrt centralized way to refuse -cpu if board doesn't support it,
> >> > >> > > > >> (which is not really related to this series) following could be done:
> >> > >> > > > >>
> >> > >> > > > >> when cpu_model removal is completely done I plan to replace
> >> > >> > > > >>   vl.c
> >> > >> > > > >>      cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model)
> >> > >> > > > >> with
> >> > >> > > > >>      cpu_parse_cpu_model(DEFAULT_TARGET_CPU_TYPE, cpu_model)
> >> > >> > > > >>
> >> > >> > > > >> so that we could drop temporary guard
> >> > >> > > > >>
> >> > >> > > > >>      if (machine_class->default_cpu_type) {
> >> > >> > > > >
> >> > >> > > > > This sounds good to me, even if we don't reject -cpu on any
> >> > >> > > > > board.
> >> > >> > > > >
> >> > >> > > > >
> >> > >> > > > >>
> >> > >> > > > >> with that it would be possible to tell from machine_run_board_init()
> >> > >> > > > >> that board doesn't provide cpu but user provided '-cpu'
> >> > >> > > > >> so we would be able to:
> >> > >> > > > >>   if ((machine_class->default_cpu_type == NULL) &&
> >> > >> > > > >>       (machine->cpu_type != NULL))
> >> > >> > > > >>           error_fatal("machine doesn't support -cpu option");
> >> > >> > > > >
> >> > >> > > > > I won't complain too much if a board maintainer really wants to
> >> > >> > > > > make the board reject -cpu completely, but it's up to them.
> >> > >> > > >
> >> > >> > > > I disagree. I think a standard way of doing it is better. At least for
> >> > >> > > > each architecture. The ARM -cpu option is very confusing at the moment
> >> > >> > > > and it really doesn't need to be that bad.
> >> > >> > > >
> >> > >> > > > >
> >> > >> > > > > Personally, I'd prefer to have all boards setting
> >> > >> > > > > default_cpu_type even if they support only one CPU model, so
> >> > >> > > > > clients don't need a special case for boards that don't support
> >> > >> > > > > -cpu.
> >> > >> > > >
> >> > >> > > > I agree, I think having one CPU makes more sense. It makes it easier
> >> > >> > > > to add support for more cpus in the future and allows the users to use
> >> > >> > > > the -cpu option without killing QEMU.
> >> > >> > > I'm considering -cpu option as a legacy one that server 2 purposes now
> >> > >> >
> >> > >> > I'm not sure about "legacy", but the list of purposes looks
> >> > >> > accurate:
> >> > >> >
> >> > >> > >  1: pick cpu type for running instance
> >> > >> >
> >> > >> > This one has no replacement yet, so can we really call it legacy?
> >> > >> not really, it's not going anywhere in near future
> >> > >>
> >> > >> >
> >> > >> > >  2: convert optional features/legacy syntax to global properties
> >> > >> > >     for related cpu type
> >> > >> >
> >> > >> > This one has a replacement: -global.  But there's a difference
> >> > >> > between saying "-cpu features are implemented using -global" and
> >> > >> > "-cpu features are obsoleted by -global".  I don't think we can
> >> > >> > say it's obsolete or legacy unless existing management software
> >> > >> > is changed to be using something else.
> >> > >> >
> >> > >> >
> >> > >> > >
> >> > >> > > It plays ok for machines with single type of cpu but doesn't really scale
> >> > >> > > to more and doesn't work well nor needed if we were to specify cpus on CLI
> >> > >> > > with -device (i.e. build machine from config/CLI)
> >> > >> >
> >> > >> > This is a good point.  But -cpu is still a useful shortcut for
> >> > >> > boards that have a single CPU type.  What are the arguments we
> >> > >> > have to get rid of it completely?
> >> > >> boards that have single cpu type don't need -cpu. since cpu is not
> >> > >> configurable there.
> >> > >
> >> > > They don't need -cpu, but there's no need to reject "-cpu FOO" if
> >> > > we know FOO is the CPU model used by the board.  This is the only
> >> > > difference between what you propose and what Alistair proposes,
> >> > > right?
> >> > >
> >> > >
> >> > >>
> >> > >>
> >> > >> > > So I would not extend usage '-cpu' to boards that have fixed cpu type,
> >> > >> > > because it really useless in that case and confuses users with idea that
> >> > >> > > they have ability/need to specify -cpu on fixed cpu board.
> >> > >> >
> >> > >> > If they try to choose any other CPU model, they will see an error
> >> > >> > message explicitly saying only one CPU type is supported.  What
> >> > >> > would be the harm?
> >> > >> I guess I've already pointed drawbacks from interface point of view,
> >> > >> from maintainer pov it will be extra code to maintain valid cpus
> >> > >> vs just 'create_cpu(MY_CPU_TYPE)'
> >> > >> this patch is vivid example of the case
> >> > >
> >> > > With this part I agree.  We don't need to add boilerplate code to
> >> > > board init if the CPU model will always be the same.
> >> > >
> >> > > But I would still prefer to do this:
> >> > >
> >> > >   create_cpu(MY_CPU_TYPE);  // at XXX_init()
> >> > > [...]
> >> > >   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 = { MY_CPU_TYPE, NULL };
> >> > >   }
> >> >
> >> > I like this option. It doesn't add much code and I think makes it very
> >> > clear to users.
> >> >
> >> > Another thing to point out is that I see users specifying options to
> >> > QEMU all the time that QEMU will just ignore. Imagine people see
> >> > somewhere online that others use '-cpu' and suddenly they think they
> >> > have to. Having this throw an error that '-cpu' isn't supported in
> >> > this case (but is in others) will create confusion of when it
> >> > should/shouldn't be use. I think always allowing it and telling users
> >> > the supported CPUs clears this up.
> >>
> >> patch would look better with what Eduardo suggested above.
> >> at least it will minimize amount of not need code, so I'd go for it.
> >
> > I just see one problem: I don't see an easy way for setting:
> >   mc->valid_cpu_types = { MY_CPU_TYPE, NULL };
> > without one additional static variable for holding the array.  So
> > my claim about "only 2 lines of code" is not accurate.
> >
> > But we might do this to make the code shorter and simpler on
> > boards like xlnx_zynqmp:
> >
> > 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;
> >    }
> 
> This is fine with me.
> 
> I had prepared a patch series with your earlier approach and was about
> to send it out before I saw this email. I was then going to wait until
> something was decided but I think I'm just going to send my series out
> anyway. It has a fix for the wrong CPU in the Raspberry Pi 2 which I
> think should go in now.
> 
> We can still continue this discussion though.
> 

No problem, my suggestion can be implemented as a follow-up
series.  I plan to review your series tomorrow.  Thanks!

-- 
Eduardo