Add a valid_cpu_types property

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 <[email protected]>

---

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

hw/core/machine.c +35 -0

index 80647edc2a..3afc6a7b5b 100644
@@ -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);
 }
 

include/hw/boards.h +1 -0

index 156e0a5701..191a5b3cd8 100644
@@ -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);
 

Signature · 1 line

2.11.0

Quoted 7 lines · 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 <[email protected]>

---

Thanks!

Reviewed-by Eduardo Habkost <[email protected]>

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.

hw/core/machine.c +0 -3

index 3afc6a7b5b..36c2fb069c 100644
@@ -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])) {

Quoted 75 lines

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

hw/core/machine.c +35 -0

index 80647edc2a..3afc6a7b5b 100644
@@ -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);
 }
 

include/hw/boards.h +1 -1

index 156e0a5701..191a5b3cd8 100644
@@ -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);
 
--

Quoted 2 lines

2.11.0

--
Eduardo

Quoted 37 lines · Eduardo Habkost

Quoted 7 lines · 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 <[email protected]>

---

Thanks!

Reviewed-by Eduardo Habkost <[email protected]>

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.

hw/core/machine.c +0 -3

index 3afc6a7b5b..36c2fb069c 100644
@@ -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])) {

hw/core/machine.c void machine_run_board_init(MachineState *machine) · line 783

Quoted 8 lines

hw/core/machine.c +1 -1

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

Quoted 49 lines · Alistair Francis

Quoted 37 lines · Eduardo Habkost

Quoted 7 lines · 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 <[email protected]>

---

Thanks!

Reviewed-by Eduardo Habkost <[email protected]>

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.

hw/core/machine.c +0 -3

index 3afc6a7b5b..36c2fb069c 100644
@@ -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])) {

hw/core/machine.c void machine_run_board_init(MachineState *machine) · line 783

Quoted 8 lines

hw/core/machine.c +1 -1

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

Quoted 1 line

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

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 <[email protected]>

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 <[email protected]>

---

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

hw/arm/netduino2.c +8 -1

index f936017d4a..b68ecf2c08 100644
@@ -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)

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 <[email protected]>

---

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

hw/arm/netduino2.c +0 -0

index f936017d4a..b68ecf2c08 100644
@@ -34,18 +34,25 @@ static void netduino2_init(MachineState *machine)
     DeviceState *dev;
 

hw/arm/netduino2.c +4 -0

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

Quoted 29 lines · On Tue, 3 Oct 2017 13:05:11 -0700 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 <[email protected]>

---

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

hw/arm/netduino2.c +1 -1

index f936017d4a..b68ecf2c08 100644
@@ -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);
 

hw/arm/netduino2.c static void netduino2_init(MachineState *machine) · line 44

Quoted 5 lines

hw/arm/netduino2.c +1 -0

     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

Quoted 15 lines

hw/arm/netduino2.c +6 -0

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

Quoted 2 lines

Quoted 1 line

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

Quoted 9 lines

Quoted 2 lines

Quoted 1 line

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

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/xilinx_zynq.c +5 -0

index 1836a4ed45..de1e0bbce1 100644
@@ -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)

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/raspi.c +4 -0

index 5941c9f751..555db0f258 100644
@@ -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";

hw/arm/raspi.c +0 -0

 };
 DEFINE_MACHINE("raspi2", raspi2_machine_init)

Signature · 1 line

2.11.0

Quoted 16 lines · Alistair Francis

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/raspi.c +0 -0

index 5941c9f751..555db0f258 100644
@@ -158,6 +158,10 @@ static void raspi2_init(MachineState *machine)
     setup_boot(machine, 2, machine->ram_size - vcram_size);
 }
 

hw/arm/raspi.c static void raspi2_machine_init(MachineClass *mc) · line 177

Quoted 13 lines

hw/arm/raspi.c +6 -0

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

Quoted 6 lines

hw/arm/raspi.c +0 -1

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

2.11.0

--
Eduardo

Quoted 17 lines · Eduardo Habkost

Quoted 16 lines · Alistair Francis

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/raspi.c +0 -0

index 5941c9f751..555db0f258 100644
@@ -158,6 +158,10 @@ static void raspi2_init(MachineState *machine)
     setup_boot(machine, 2, machine->ram_size - vcram_size);
 }
 

Quoted 16 lines

hw/arm/raspi.c static void raspi2_machine_init(MachineClass *mc) · line 177

Quoted 13 lines

hw/arm/raspi.c +6 -0

+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

Quoted 9 lines

Quoted 6 lines

hw/arm/raspi.c +0 -1

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

2.11.0

--
Eduardo

Quoted 35 lines · Alistair Francis

Quoted 33 lines · Eduardo Habkost

Quoted 24 lines · Alistair Francis

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/raspi.c +4 -0

index 5941c9f751..555db0f258 100644
@@ -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";

hw/arm/raspi.c static void raspi2_machine_init(MachineClass *mc) · line 177

Quoted 5 lines

hw/arm/raspi.c +2 -0

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

Quoted 2 lines

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é <[email protected]>

Quoted 13 lines

Thanks,
Alistair

Quoted 9 lines

Quoted 6 lines

hw/arm/raspi.c +0 -1

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

2.11.0

--
Eduardo

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/xlnx-zcu102.c +0 -0

index 519a16ed98..039649e522 100644

hw/arm/xlnx-zcu102.c +8 -0

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

hw/arm/xlnx-zynqmp.c +9 -7

index 2b27daf51d..1bff099ec1 100644
@@ -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),

include/hw/arm/xlnx-zynqmp.h +1 -0

index 6eff81a995..5afb8de11e 100644
@@ -86,6 +86,7 @@ typedef struct XlnxZynqMPState {
     XlnxDPState dp;
     XlnxDPDMAState dpdma;
 
+    char *cpu_type;
     char *boot_cpu;
     ARMCPU *boot_cpu_ptr;
 

Signature · 1 line

2.11.0

Quoted 18 lines · Alistair Francis

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/xlnx-zcu102.c +0 -0

index 519a16ed98..039649e522 100644
@@ -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);
 

hw/arm/xlnx-zcu102.c static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine) · line 102

Quoted 2 lines

hw/arm/xlnx-zcu102.c +2 -0

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

Quoted 88 lines

hw/arm/xlnx-zcu102.c +8 -0

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

hw/arm/xlnx-zynqmp.c +9 -7

index 2b27daf51d..1bff099ec1 100644
@@ -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),

include/hw/arm/xlnx-zynqmp.h +1 -1

index 6eff81a995..5afb8de11e 100644
@@ -86,6 +86,7 @@ typedef struct XlnxZynqMPState {
     XlnxDPState dp;
     XlnxDPDMAState dpdma;
 
+    char *cpu_type;
     char *boot_cpu;
     ARMCPU *boot_cpu_ptr;
 
--

Quoted 2 lines

2.11.0

--
Eduardo

Quoted 19 lines · Eduardo Habkost

Quoted 18 lines · Alistair Francis

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/xlnx-zcu102.c +0 -0

index 519a16ed98..039649e522 100644
@@ -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);
 

Quoted 7 lines

hw/arm/xlnx-zcu102.c static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine) · line 102

Quoted 2 lines

hw/arm/xlnx-zcu102.c +2 -0

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

Quoted 19 lines · Eduardo Habkost

Quoted 18 lines · Alistair Francis

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/xlnx-zcu102.c +0 -0

index 519a16ed98..039649e522 100644
@@ -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);
 

Quoted 7 lines

hw/arm/xlnx-zcu102.c static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine) · line 102

Quoted 2 lines

hw/arm/xlnx-zcu102.c +2 -0

+    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

Quoted 93 lines

Quoted 88 lines

hw/arm/xlnx-zcu102.c +8 -0

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

hw/arm/xlnx-zynqmp.c +9 -7

index 2b27daf51d..1bff099ec1 100644
@@ -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),

include/hw/arm/xlnx-zynqmp.h +1 -1

index 6eff81a995..5afb8de11e 100644
@@ -86,6 +86,7 @@ typedef struct XlnxZynqMPState {
     XlnxDPState dp;
     XlnxDPDMAState dpdma;
 
+    char *cpu_type;
     char *boot_cpu;
     ARMCPU *boot_cpu_ptr;
 
--

Quoted 2 lines

2.11.0

--
Eduardo

Quoted 30 lines · On Tue, 3 Oct 2017 14:41:17 -0700 Alistair Francis…

Quoted 19 lines · Eduardo Habkost

Quoted 18 lines · Alistair Francis

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/xlnx-zcu102.c +0 -0

index 519a16ed98..039649e522 100644
@@ -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);
 

Quoted 7 lines

hw/arm/xlnx-zcu102.c static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine) · line 102

Quoted 2 lines

hw/arm/xlnx-zcu102.c +2 -0

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

Quoted 7 lines

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

Quoted 40 lines · Igor Mammedov

Quoted 30 lines · On Tue, 3 Oct 2017 14:41:17 -0700 Alistair Francis…

Quoted 19 lines · Eduardo Habkost

Quoted 18 lines · Alistair Francis

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/xlnx-zcu102.c +0 -0

index 519a16ed98..039649e522 100644
@@ -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);
 

Quoted 7 lines

hw/arm/xlnx-zcu102.c static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine) · line 102

Quoted 2 lines

hw/arm/xlnx-zcu102.c +2 -0

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

Quoted 7 lines

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.

Quoted 3 lines

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.

Quoted 45 lines · On Wed, 4 Oct 2017 09:28:51 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 40 lines · Igor Mammedov

Quoted 30 lines · On Tue, 3 Oct 2017 14:41:17 -0700 Alistair Francis…

Quoted 19 lines · Eduardo Habkost

Quoted 18 lines · Alistair Francis

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/xlnx-zcu102.c +0 -0

index 519a16ed98..039649e522 100644
@@ -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);
 

Quoted 7 lines

hw/arm/xlnx-zcu102.c static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine) · line 102

Quoted 2 lines

hw/arm/xlnx-zcu102.c +2 -0

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

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 6 lines

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");

Quoted 60 lines · Igor Mammedov

Quoted 45 lines · On Wed, 4 Oct 2017 09:28:51 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 40 lines · Igor Mammedov

Quoted 30 lines · On Tue, 3 Oct 2017 14:41:17 -0700 Alistair Francis…

Quoted 19 lines · Eduardo Habkost

Quoted 18 lines · Alistair Francis

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/xlnx-zcu102.c +0 -0

index 519a16ed98..039649e522 100644
@@ -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);
 

Quoted 7 lines

hw/arm/xlnx-zcu102.c static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine) · line 102

Quoted 2 lines

hw/arm/xlnx-zcu102.c +2 -0

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

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 6 lines

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.

Quoted 4 lines

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

Quoted 6 lines

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

Quoted 7 lines

     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.

Quoted 7 lines

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

Quoted 61 lines · Eduardo Habkost

Quoted 60 lines · Igor Mammedov

Quoted 45 lines · On Wed, 4 Oct 2017 09:28:51 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 40 lines · Igor Mammedov

Quoted 30 lines · On Tue, 3 Oct 2017 14:41:17 -0700 Alistair Francis…

Quoted 19 lines · Eduardo Habkost

Quoted 18 lines · Alistair Francis

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/xlnx-zcu102.c +0 -0

index 519a16ed98..039649e522 100644
@@ -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);
 

Quoted 7 lines

hw/arm/xlnx-zcu102.c static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine) · line 102

Quoted 2 lines

hw/arm/xlnx-zcu102.c +2 -0

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

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 6 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 32 lines

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

Quoted 6 lines

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

Quoted 7 lines

     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.

Quoted 7 lines

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");  

Quoted 2 lines

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.

Quoted 1 line

Quoted 4 lines

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

Quoted 4 lines

--
Eduardo

Quoted 130 lines · On Wed, 4 Oct 2017 14:39:20 -0700 Alistair Francis…

Quoted 61 lines · Eduardo Habkost

Quoted 60 lines · Igor Mammedov

Quoted 45 lines · On Wed, 4 Oct 2017 09:28:51 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 40 lines · Igor Mammedov

Quoted 30 lines · On Tue, 3 Oct 2017 14:41:17 -0700 Alistair Francis…

Quoted 19 lines · Eduardo Habkost

Quoted 18 lines · Alistair Francis

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/xlnx-zcu102.c +0 -0

index 519a16ed98..039649e522 100644
@@ -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);
 

Quoted 7 lines

hw/arm/xlnx-zcu102.c static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine) · line 102

Quoted 2 lines

hw/arm/xlnx-zcu102.c +2 -0

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

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 6 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 32 lines

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

Quoted 6 lines

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

Quoted 7 lines

     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.

Quoted 7 lines

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");  

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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

Quoted 136 lines · Igor Mammedov

Quoted 133 lines · On Wed, 4 Oct 2017 14:39:20 -0700 Alistair Francis…

Quoted 61 lines · Eduardo Habkost

Quoted 60 lines · Igor Mammedov

Quoted 45 lines · On Wed, 4 Oct 2017 09:28:51 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 40 lines · Igor Mammedov

Quoted 30 lines · On Tue, 3 Oct 2017 14:41:17 -0700 Alistair Francis…

Quoted 19 lines · Eduardo Habkost

Quoted 18 lines · Alistair Francis

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/xlnx-zcu102.c +0 -0

index 519a16ed98..039649e522 100644
@@ -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);
 

Quoted 7 lines

hw/arm/xlnx-zcu102.c static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine) · line 102

Quoted 2 lines

hw/arm/xlnx-zcu102.c +2 -0

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

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 6 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 32 lines

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

Quoted 6 lines

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

Quoted 7 lines

     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.

Quoted 7 lines

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");    

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 1 line

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:

Quoted 1 line

1: pick cpu type for running instance

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

Quoted 3 lines

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.

Quoted 1 line

Quoted 3 lines

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?

Quoted 3 lines

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?

Quoted 5 lines

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.

Quoted 137 lines · On Thu, 5 Oct 2017 14:09:06 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 136 lines · Igor Mammedov

Quoted 133 lines · On Wed, 4 Oct 2017 14:39:20 -0700 Alistair Francis…

Quoted 61 lines · Eduardo Habkost

Quoted 60 lines · Igor Mammedov

Quoted 45 lines · On Wed, 4 Oct 2017 09:28:51 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 40 lines · Igor Mammedov

Quoted 30 lines · On Tue, 3 Oct 2017 14:41:17 -0700 Alistair Francis…

Quoted 19 lines · Eduardo Habkost

Quoted 18 lines · Alistair Francis

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/xlnx-zcu102.c +0 -0

index 519a16ed98..039649e522 100644
@@ -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);
 

Quoted 7 lines

hw/arm/xlnx-zcu102.c static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine) · line 102

Quoted 2 lines

hw/arm/xlnx-zcu102.c +2 -0

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

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 6 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 32 lines

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

Quoted 6 lines

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

Quoted 7 lines

     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.

Quoted 7 lines

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");    

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

Quoted 1 line

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:

Quoted 1 line

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

Quoted 16 lines

Quoted 3 lines

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.

Quoted 4 lines

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)

Quoted 3 lines

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.

Quoted 6 lines

Quoted 5 lines

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.

Quoted 3 lines

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

Quoted 11 lines

Quoted 7 lines

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

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 166 lines · Igor Mammedov

Quoted 137 lines · On Thu, 5 Oct 2017 14:09:06 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 136 lines · Igor Mammedov

Quoted 133 lines · On Wed, 4 Oct 2017 14:39:20 -0700 Alistair Francis…

Quoted 61 lines · Eduardo Habkost

Quoted 60 lines · Igor Mammedov

Quoted 45 lines · On Wed, 4 Oct 2017 09:28:51 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 40 lines · Igor Mammedov

Quoted 30 lines · On Tue, 3 Oct 2017 14:41:17 -0700 Alistair Francis…

Quoted 19 lines · Eduardo Habkost

Quoted 18 lines · Alistair Francis

List all possible valid CPU options.

Signed-off-by Alistair Francis <[email protected]>

---

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

hw/arm/xlnx-zcu102.c +0 -0

index 519a16ed98..039649e522 100644
@@ -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);
 

Quoted 7 lines

hw/arm/xlnx-zcu102.c static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine) · line 102

Quoted 2 lines

hw/arm/xlnx-zcu102.c +2 -0

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

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 6 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 32 lines

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

Quoted 6 lines

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

Quoted 7 lines

     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.

Quoted 7 lines

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");

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

Quoted 1 line

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:

Quoted 1 line

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

Quoted 16 lines

Quoted 3 lines

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.

Quoted 4 lines

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)

Quoted 5 lines

Quoted 3 lines

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?

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 11 lines

Quoted 11 lines

Quoted 7 lines

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.

Quoted 9 lines

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)

Quoted 8 lines

Quoted 6 lines

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.

Quoted 7 lines

Quoted 6 lines

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

Quoted 6 lines

Quoted 4 lines

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?

Quoted 1 line

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

Quoted 1 line

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

Quoted 3 lines

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.

Quoted 7 lines

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

Quoted 198 lines · Eduardo Habkost

Quoted 166 lines · Igor Mammedov

Quoted 137 lines · On Thu, 5 Oct 2017 14:09:06 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 136 lines · Igor Mammedov

Quoted 133 lines · On Wed, 4 Oct 2017 14:39:20 -0700 Alistair Francis…

Quoted 61 lines · Eduardo Habkost

Quoted 60 lines · Igor Mammedov

Quoted 45 lines · On Wed, 4 Oct 2017 09:28:51 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 40 lines · Igor Mammedov

Quoted 30 lines · On Tue, 3 Oct 2017 14:41:17 -0700 Alistair Francis…

Quoted 19 lines · Eduardo Habkost

Quoted 18 lines · Alistair Francis

List all possible valid CPU options.

Signed-off-by: Alistair Francis <[email protected]>
---

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

Quoted 7 lines

Quoted 2 lines

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

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 6 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 32 lines

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

Quoted 6 lines

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

Quoted 7 lines

     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.

Quoted 7 lines

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");  

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

Quoted 1 line

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:

Quoted 1 line

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

Quoted 16 lines

Quoted 3 lines

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.

Quoted 4 lines

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)

Quoted 5 lines

Quoted 3 lines

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?

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 3 lines

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:

Quoted 7 lines

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.

Quoted 210 lines · On Fri, 6 Oct 2017 15:06:57 -0700 Alistair Francis…

Quoted 198 lines · Eduardo Habkost

Quoted 166 lines · Igor Mammedov

Quoted 137 lines · On Thu, 5 Oct 2017 14:09:06 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 136 lines · Igor Mammedov

Quoted 133 lines · On Wed, 4 Oct 2017 14:39:20 -0700 Alistair Francis…

Quoted 61 lines · Eduardo Habkost

Quoted 60 lines · Igor Mammedov

Quoted 45 lines · On Wed, 4 Oct 2017 09:28:51 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 40 lines · Igor Mammedov

Quoted 30 lines · On Tue, 3 Oct 2017 14:41:17 -0700 Alistair Francis…

Quoted 26 lines · 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 <[email protected]>
---

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.

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 6 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 32 lines

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

Quoted 6 lines

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

Quoted 7 lines

     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.

Quoted 7 lines

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");  

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

Quoted 1 line

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:

Quoted 1 line

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

Quoted 16 lines

Quoted 3 lines

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.

Quoted 4 lines

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)

Quoted 5 lines

Quoted 3 lines

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?

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 3 lines

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:

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 221 lines · Igor Mammedov

Quoted 210 lines · On Fri, 6 Oct 2017 15:06:57 -0700 Alistair Francis…

Quoted 198 lines · Eduardo Habkost

Quoted 166 lines · Igor Mammedov

Quoted 137 lines · On Thu, 5 Oct 2017 14:09:06 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 136 lines · Igor Mammedov

Quoted 133 lines · On Wed, 4 Oct 2017 14:39:20 -0700 Alistair Francis…

Quoted 61 lines · Eduardo Habkost

Quoted 60 lines · Igor Mammedov

Quoted 45 lines · On Wed, 4 Oct 2017 09:28:51 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 40 lines · Igor Mammedov

Quoted 37 lines · On Tue, 3 Oct 2017 14:41:17 -0700 Alistair Francis…

On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <[email protected]> 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 <[email protected]>
---

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.

Quoted 3 lines

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

Quoted 6 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 32 lines

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

Quoted 6 lines

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

Quoted 7 lines

     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.

Quoted 7 lines

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");

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

Quoted 1 line

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:

Quoted 1 line

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

Quoted 16 lines

Quoted 3 lines

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.

Quoted 4 lines

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)

Quoted 5 lines

Quoted 3 lines

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?

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 3 lines

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:

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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

Quoted 243 lines · Eduardo Habkost

Quoted 221 lines · Igor Mammedov

Quoted 210 lines · On Fri, 6 Oct 2017 15:06:57 -0700 Alistair Francis…

Quoted 198 lines · Eduardo Habkost

Quoted 166 lines · Igor Mammedov

Quoted 137 lines · On Thu, 5 Oct 2017 14:09:06 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 136 lines · Igor Mammedov

Quoted 133 lines · On Wed, 4 Oct 2017 14:39:20 -0700 Alistair Francis…

Quoted 61 lines · Eduardo Habkost

Quoted 60 lines · Igor Mammedov

Quoted 45 lines · On Wed, 4 Oct 2017 09:28:51 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 43 lines · Igor Mammedov

On Tue, 3 Oct 2017 14:41:17 -0700
Alistair Francis <[email protected]> wrote:

On Tue, Oct 3, 2017 at 1:36 PM, Eduardo Habkost <[email protected]> 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 <[email protected]>
---

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

Quoted 6 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 32 lines

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

Quoted 6 lines

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

Quoted 7 lines

     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.

Quoted 7 lines

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");

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

Quoted 1 line

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:

Quoted 1 line

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

Quoted 16 lines

Quoted 3 lines

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.

Quoted 4 lines

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)

Quoted 5 lines

Quoted 3 lines

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?

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 3 lines

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:

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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)

Quoted 7 lines

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.