qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v9 0/9] Unified CPU type check
@ 2023-12-04  0:47 Gavin Shan
  2023-12-04  0:47 ` [PATCH v9 1/9] machine: Use error handling when CPU type is checked Gavin Shan
                   ` (9 more replies)
  0 siblings, 10 replies; 19+ messages in thread
From: Gavin Shan @ 2023-12-04  0:47 UTC (permalink / raw)
  To: qemu-arm
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	philmd, armbru, wangyanan55, vijai, palmer, alistair.francis,
	bin.meng, liwei1518, dbarboza, zhiwei_liu, shan.gavin

This series bases on Phil's repository because the prepatory commits
have been queued to the branch.

  https://gitlab.com/philmd/qemu.git (branch: cpus-next)

There are two places where the user specified CPU type is checked to see
if it's supported or allowed by the board: machine_run_board_init() and
mc->init(). We don't have to maintain two duplicate sets of logic. This
series intends to move the check to machine_run_board_init() so that we
have unified CPU type check.

This series can be checked out from:

  git@github.com:gwshan/qemu.git (branch: kvm/cpu-type)

PATCH[1-4] refactors and improves the logic to validate CPU type in
           machine_run_board_init()
PATCH[5-9] validates the CPU type in machine_run_board_init() for the
           individual boards

v1: https://lists.nongnu.org/archive/html/qemu-arm/2023-07/msg00302.html
v2: https://lists.nongnu.org/archive/html/qemu-arm/2023-07/msg00528.html
v3: https://lists.nongnu.org/archive/html/qemu-arm/2023-09/msg00157.html
v4: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00005.html
v5: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00611.html
v6: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00768.html
v7: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01045.html
v8: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01168.html

Testing
=======

Before the series is applied:

  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M virt -cpu cortex-a8
  qemu-system-aarch64: mach-virt: CPU type cortex-a8-arm-cpu not supported
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu cortex-a53
  qemu-system-aarch64: sbsa-ref: CPU type cortex-a53-arm-cpu not supported
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu sa1100
  qemu-system-aarch64: sbsa-ref: CPU type sa1100-arm-cpu not supported
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu host
  qemu-system-aarch64: unable to find CPU model 'host'

After the series is applied:

  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M virt -cpu cortex-a8
  qemu-system-aarch64: Invalid CPU model: cortex-a8
  The valid models are: cortex-a7, cortex-a15, cortex-a35, cortex-a55,     \
                        cortex-a72, cortex-a76, cortex-a710, a64fx,        \
                        neoverse-n1, neoverse-v1, neoverse-n2, cortex-a53, \
                        cortex-a57, max
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu cortex-a53
  qemu-system-aarch64: Invalid CPU model: cortex-a53
  The valid models are: cortex-a57, cortex-a72, neoverse-n1, neoverse-v1,  \
                        neoverse-n2, max
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu sa1100
  qemu-system-aarch64: Invalid CPU model: sa1100
  The valid models are: cortex-a57, cortex-a72, neoverse-n1, neoverse-v1,  \
                        neoverse-n2, max
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu host
  qemu-system-aarch64: unable to find CPU model 'host'

Changelog
=========
v9:
  * Pick r-bs from Markus and Phil                               (Gavin)
  * Improved change log for PATCH[v9 1/9]                        (Markus)
  * assert(mc->valid_cpu_types[0] != NULL) and assert(cc != NULL)
    in is_cpu_type_supported() of PATCH[v9 3/9]                  (Phil)
v8:
  * Pick r-bs from Phil                                          (Gavin)
  * Drop @local_err and use @errp in machine_run_board_init()    (Phil)
  * Add PATCH[v8 3/9] to improve is_cpu_type_supported()         (Phil)
  * Use g_autofree and replace 'type' with 'model' for the
    error messages in is_cpu_type_supported()                    (Phil)
v7:
  * Add 'return' after error_propagate() in machine_run_board_init()
    to avoid calling mc->init() in the failing case              (Marcin)
v6:
  * Drop PATCH[v5 01-23], queued by Phil                         (Phil)
  * Clearer hint if only one CPU type is supported and have
    'const MachineState *' in is_cpu_type_supported()            (Phil)
  * Move valid_cpu_types[] to board's class_init() function      (Phil)

Gavin Shan (9):
  machine: Use error handling when CPU type is checked
  machine: Introduce helper is_cpu_type_supported()
  machine: Improve is_cpu_type_supported()
  machine: Print CPU model name instead of CPU type
  hw/arm/virt: Hide host CPU model for tcg
  hw/arm/virt: Check CPU type in machine_run_board_init()
  hw/arm/sbsa-ref: Check CPU type in machine_run_board_init()
  hw/arm: Check CPU type in machine_run_board_init()
  hw/riscv/shakti_c: Check CPU type in machine_run_board_init()

 hw/arm/bananapi_m2u.c   | 12 +++---
 hw/arm/cubieboard.c     | 12 +++---
 hw/arm/mps2-tz.c        | 26 +++++++++---
 hw/arm/mps2.c           | 26 +++++++++---
 hw/arm/msf2-som.c       | 12 +++---
 hw/arm/musca.c          | 12 +++---
 hw/arm/npcm7xx_boards.c | 12 +++---
 hw/arm/orangepi.c       | 12 +++---
 hw/arm/sbsa-ref.c       | 36 +++++-----------
 hw/arm/virt.c           | 60 ++++++++++----------------
 hw/core/machine.c       | 94 +++++++++++++++++++++++++----------------
 hw/riscv/shakti_c.c     | 13 +++---
 12 files changed, 170 insertions(+), 157 deletions(-)

-- 
2.42.0



^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v9 1/9] machine: Use error handling when CPU type is checked
  2023-12-04  0:47 [PATCH v9 0/9] Unified CPU type check Gavin Shan
@ 2023-12-04  0:47 ` Gavin Shan
  2024-01-05 11:00   ` Philippe Mathieu-Daudé
  2023-12-04  0:47 ` [PATCH v9 2/9] machine: Introduce helper is_cpu_type_supported() Gavin Shan
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Gavin Shan @ 2023-12-04  0:47 UTC (permalink / raw)
  To: qemu-arm
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	philmd, armbru, wangyanan55, vijai, palmer, alistair.francis,
	bin.meng, liwei1518, dbarboza, zhiwei_liu, shan.gavin

Functions that use an Error **errp parameter to return errors should
not also report them to the user, because reporting is the caller's
job. The principle is violated by machine_run_board_init() because
it calls error_report(), error_printf(), and exit(1) when the machine
doesn't support the requested CPU type.

Clean this up by using error_setg() and error_append_hint() instead.
No functional change, as the only caller passes &error_fatal.

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
v9: Improved change log                                  (Markus)
---
 hw/core/machine.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 0c17398141..bde7f4af6d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1466,15 +1466,16 @@ void machine_run_board_init(MachineState *machine, const char *mem_path, Error *
 
         if (!machine_class->valid_cpu_types[i]) {
             /* 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]);
+            error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
+            error_append_hint(errp, "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_append_hint(errp, ", %s",
+                                  machine_class->valid_cpu_types[i]);
             }
-            error_printf("\n");
 
-            exit(1);
+            error_append_hint(&errp, "\n");
+            return;
         }
     }
 
-- 
2.42.0



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v9 2/9] machine: Introduce helper is_cpu_type_supported()
  2023-12-04  0:47 [PATCH v9 0/9] Unified CPU type check Gavin Shan
  2023-12-04  0:47 ` [PATCH v9 1/9] machine: Use error handling when CPU type is checked Gavin Shan
@ 2023-12-04  0:47 ` Gavin Shan
  2023-12-04  0:47 ` [PATCH v9 3/9] machine: Improve is_cpu_type_supported() Gavin Shan
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 19+ messages in thread
From: Gavin Shan @ 2023-12-04  0:47 UTC (permalink / raw)
  To: qemu-arm
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	philmd, armbru, wangyanan55, vijai, palmer, alistair.francis,
	bin.meng, liwei1518, dbarboza, zhiwei_liu, shan.gavin

The logic, to check if the specified CPU type is supported in
machine_run_board_init(), is independent enough. Factor it out into
helper is_cpu_type_supported(). machine_run_board_init() looks a bit
clean with this. Since we're here, @machine_class is renamed to @mc to
avoid multiple line spanning of code. The comments are tweaked a bit
either.

No functional change intended.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/core/machine.c | 83 +++++++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 38 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index bde7f4af6d..1797e002f9 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1387,13 +1387,53 @@ out:
     return r;
 }
 
+static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    ObjectClass *oc = object_class_by_name(machine->cpu_type);
+    CPUClass *cc;
+    int i;
+
+    /*
+     * Check if the user specified CPU type is supported when the valid
+     * CPU types have been determined. Note that the user specified CPU
+     * type is provided through '-cpu' option.
+     */
+    if (mc->valid_cpu_types && machine->cpu_type) {
+        for (i = 0; mc->valid_cpu_types[i]; i++) {
+            if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
+                break;
+            }
+        }
+
+        /* The user specified CPU type isn't valid */
+        if (!mc->valid_cpu_types[i]) {
+            error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
+            error_append_hint(errp, "The valid types are: %s",
+                              mc->valid_cpu_types[0]);
+            for (i = 1; mc->valid_cpu_types[i]; i++) {
+                error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
+            }
+
+            error_append_hint(errp, "\n");
+            return false;
+        }
+    }
+
+    /* Check if CPU type is deprecated and warn if so */
+    cc = CPU_CLASS(oc);
+    if (cc && cc->deprecation_note) {
+        warn_report("CPU model %s is deprecated -- %s",
+                    machine->cpu_type, cc->deprecation_note);
+    }
+
+    return true;
+}
 
 void machine_run_board_init(MachineState *machine, const char *mem_path, Error **errp)
 {
     ERRP_GUARD();
     MachineClass *machine_class = MACHINE_GET_CLASS(machine);
-    ObjectClass *oc = object_class_by_name(machine->cpu_type);
-    CPUClass *cc;
 
     /* This checkpoint is required by replay to separate prior clock
        reading from the other reads, because timer polling functions query
@@ -1448,42 +1488,9 @@ void machine_run_board_init(MachineState *machine, const char *mem_path, Error *
         machine->ram = machine_consume_memdev(machine, machine->memdev);
     }
 
-    /* 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) {
-        int i;
-
-        for (i = 0; machine_class->valid_cpu_types[i]; i++) {
-            if (object_class_dynamic_cast(oc,
-                                          machine_class->valid_cpu_types[i])) {
-                /* The user specified CPU is in the valid field, we are
-                 * good to go.
-                 */
-                break;
-            }
-        }
-
-        if (!machine_class->valid_cpu_types[i]) {
-            /* The user specified CPU is not valid */
-            error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
-            error_append_hint(errp, "The valid types are: %s",
-                              machine_class->valid_cpu_types[0]);
-            for (i = 1; machine_class->valid_cpu_types[i]; i++) {
-                error_append_hint(errp, ", %s",
-                                  machine_class->valid_cpu_types[i]);
-            }
-
-            error_append_hint(&errp, "\n");
-            return;
-        }
-    }
-
-    /* Check if CPU type is deprecated and warn if so */
-    cc = CPU_CLASS(oc);
-    if (cc && cc->deprecation_note) {
-        warn_report("CPU model %s is deprecated -- %s", machine->cpu_type,
-                    cc->deprecation_note);
+    /* Check if the CPU type is supported */
+    if (!is_cpu_type_supported(machine, errp)) {
+        return;
     }
 
     if (machine->cgs) {
-- 
2.42.0



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v9 3/9] machine: Improve is_cpu_type_supported()
  2023-12-04  0:47 [PATCH v9 0/9] Unified CPU type check Gavin Shan
  2023-12-04  0:47 ` [PATCH v9 1/9] machine: Use error handling when CPU type is checked Gavin Shan
  2023-12-04  0:47 ` [PATCH v9 2/9] machine: Introduce helper is_cpu_type_supported() Gavin Shan
@ 2023-12-04  0:47 ` Gavin Shan
  2024-01-05 22:09   ` Philippe Mathieu-Daudé
  2023-12-04  0:47 ` [PATCH v9 4/9] machine: Print CPU model name instead of CPU type Gavin Shan
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Gavin Shan @ 2023-12-04  0:47 UTC (permalink / raw)
  To: qemu-arm
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	philmd, armbru, wangyanan55, vijai, palmer, alistair.francis,
	bin.meng, liwei1518, dbarboza, zhiwei_liu, shan.gavin

It's no sense to check the CPU type when mc->valid_cpu_types[0] is
NULL, which is a program error. Raise an assert on this.

A precise hint for the error message is given when mc->valid_cpu_types[0]
is the only valid entry. Besides, enumeration on mc->valid_cpu_types[0]
when we have mutiple valid entries there is avoided to increase the code
readability, as suggested by Philippe Mathieu-Daudé.

Besides, @cc comes from machine->cpu_type or mc->default_cpu_type. For
the later case, it can be NULL and it's also a program error. We should
use assert() in this case.

Signed-off-by: Gavin Shan <gshan@redhat.com>
v9: assert(mc->valid_cpu_types[0] != NULL)                   (Phil)
    assert(cc != NULL);                                      (Phil)
---
 hw/core/machine.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1797e002f9..4ae9aaee8e 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1400,6 +1400,7 @@ static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
      * type is provided through '-cpu' option.
      */
     if (mc->valid_cpu_types && machine->cpu_type) {
+        assert(mc->valid_cpu_types[0] != NULL);
         for (i = 0; mc->valid_cpu_types[i]; i++) {
             if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
                 break;
@@ -1409,20 +1410,27 @@ static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
         /* The user specified CPU type isn't valid */
         if (!mc->valid_cpu_types[i]) {
             error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
-            error_append_hint(errp, "The valid types are: %s",
-                              mc->valid_cpu_types[0]);
-            for (i = 1; mc->valid_cpu_types[i]; i++) {
-                error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
+            if (!mc->valid_cpu_types[1]) {
+                error_append_hint(errp, "The only valid type is: %s\n",
+                                  mc->valid_cpu_types[0]);
+            } else {
+                error_append_hint(errp, "The valid types are: ");
+                for (i = 0; mc->valid_cpu_types[i]; i++) {
+                    error_append_hint(errp, "%s%s",
+                                      mc->valid_cpu_types[i],
+                                      mc->valid_cpu_types[i + 1] ? ", " : "");
+                }
+                error_append_hint(errp, "\n");
             }
 
-            error_append_hint(errp, "\n");
             return false;
         }
     }
 
     /* Check if CPU type is deprecated and warn if so */
     cc = CPU_CLASS(oc);
-    if (cc && cc->deprecation_note) {
+    assert(cc != NULL);
+    if (cc->deprecation_note) {
         warn_report("CPU model %s is deprecated -- %s",
                     machine->cpu_type, cc->deprecation_note);
     }
-- 
2.42.0



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v9 4/9] machine: Print CPU model name instead of CPU type
  2023-12-04  0:47 [PATCH v9 0/9] Unified CPU type check Gavin Shan
                   ` (2 preceding siblings ...)
  2023-12-04  0:47 ` [PATCH v9 3/9] machine: Improve is_cpu_type_supported() Gavin Shan
@ 2023-12-04  0:47 ` Gavin Shan
  2023-12-04  0:47 ` [PATCH v9 5/9] hw/arm/virt: Hide host CPU model for tcg Gavin Shan
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 19+ messages in thread
From: Gavin Shan @ 2023-12-04  0:47 UTC (permalink / raw)
  To: qemu-arm
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	philmd, armbru, wangyanan55, vijai, palmer, alistair.francis,
	bin.meng, liwei1518, dbarboza, zhiwei_liu, shan.gavin

The names of supported CPU models instead of CPU types should be
printed when the user specified CPU type isn't supported, to be
consistent with the output from '-cpu ?'.

Correct the error messages to print CPU model names instead of CPU
type names.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/core/machine.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 4ae9aaee8e..bc9c3e7dcd 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1409,15 +1409,19 @@ static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
 
         /* The user specified CPU type isn't valid */
         if (!mc->valid_cpu_types[i]) {
-            error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
+            g_autofree char *requested = cpu_model_from_type(machine->cpu_type);
+            error_setg(errp, "Invalid CPU model: %s", requested);
             if (!mc->valid_cpu_types[1]) {
-                error_append_hint(errp, "The only valid type is: %s\n",
-                                  mc->valid_cpu_types[0]);
+                g_autofree char *model = cpu_model_from_type(
+                                                 mc->valid_cpu_types[0]);
+                error_append_hint(errp, "The only valid type is: %s\n", model);
             } else {
-                error_append_hint(errp, "The valid types are: ");
+                error_append_hint(errp, "The valid models are: ");
                 for (i = 0; mc->valid_cpu_types[i]; i++) {
+                    g_autofree char *model = cpu_model_from_type(
+                                                 mc->valid_cpu_types[i]);
                     error_append_hint(errp, "%s%s",
-                                      mc->valid_cpu_types[i],
+                                      model,
                                       mc->valid_cpu_types[i + 1] ? ", " : "");
                 }
                 error_append_hint(errp, "\n");
-- 
2.42.0



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v9 5/9] hw/arm/virt: Hide host CPU model for tcg
  2023-12-04  0:47 [PATCH v9 0/9] Unified CPU type check Gavin Shan
                   ` (3 preceding siblings ...)
  2023-12-04  0:47 ` [PATCH v9 4/9] machine: Print CPU model name instead of CPU type Gavin Shan
@ 2023-12-04  0:47 ` Gavin Shan
  2023-12-04  0:47 ` [PATCH v9 6/9] hw/arm/virt: Check CPU type in machine_run_board_init() Gavin Shan
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 19+ messages in thread
From: Gavin Shan @ 2023-12-04  0:47 UTC (permalink / raw)
  To: qemu-arm
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	philmd, armbru, wangyanan55, vijai, palmer, alistair.francis,
	bin.meng, liwei1518, dbarboza, zhiwei_liu, shan.gavin

The 'host' CPU model isn't available until KVM or HVF is enabled.
For example, the following error messages are seen when the guest
is started with option '-cpu cortex-a8' on tcg after the next commit
is applied to check the CPU type in machine_run_board_init().

  ERROR:../hw/core/machine.c:1423:is_cpu_type_supported: \
  assertion failed: (model != NULL)
  Bail out! ERROR:../hw/core/machine.c:1423:is_cpu_type_supported: \
  assertion failed: (model != NULL)
  Aborted (core dumped)

Hide 'host' CPU model until KVM or HVF is enabled. With this applied,
the valid CPU models can be shown.

  qemu-system-aarch64: Invalid CPU type: cortex-a8
  The valid types are: cortex-a7, cortex-a15, cortex-a35, \
  cortex-a55, cortex-a72, cortex-a76, cortex-a710, a64fx, \
  neoverse-n1, neoverse-v1, neoverse-n2, cortex-a53,      \
  cortex-a57, max

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/virt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index be2856c018..668c0d3194 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -220,7 +220,9 @@ static const char *valid_cpus[] = {
 #endif
     ARM_CPU_TYPE_NAME("cortex-a53"),
     ARM_CPU_TYPE_NAME("cortex-a57"),
+#if defined(CONFIG_KVM) || defined(CONFIG_HVF)
     ARM_CPU_TYPE_NAME("host"),
+#endif
     ARM_CPU_TYPE_NAME("max"),
 };
 
-- 
2.42.0



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v9 6/9] hw/arm/virt: Check CPU type in machine_run_board_init()
  2023-12-04  0:47 [PATCH v9 0/9] Unified CPU type check Gavin Shan
                   ` (4 preceding siblings ...)
  2023-12-04  0:47 ` [PATCH v9 5/9] hw/arm/virt: Hide host CPU model for tcg Gavin Shan
@ 2023-12-04  0:47 ` Gavin Shan
  2023-12-04  0:47 ` [PATCH v9 7/9] hw/arm/sbsa-ref: " Gavin Shan
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 19+ messages in thread
From: Gavin Shan @ 2023-12-04  0:47 UTC (permalink / raw)
  To: qemu-arm
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	philmd, armbru, wangyanan55, vijai, palmer, alistair.francis,
	bin.meng, liwei1518, dbarboza, zhiwei_liu, shan.gavin

Set mc->valid_cpu_types so that the user specified CPU type can be
validated in machine_run_board_init(). We needn't to do the check
by ourselves.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/arm/virt.c | 62 +++++++++++++++++++--------------------------------
 1 file changed, 23 insertions(+), 39 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 668c0d3194..04f9f5fa56 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -204,40 +204,6 @@ static const int a15irqmap[] = {
     [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */
 };
 
-static const char *valid_cpus[] = {
-#ifdef CONFIG_TCG
-    ARM_CPU_TYPE_NAME("cortex-a7"),
-    ARM_CPU_TYPE_NAME("cortex-a15"),
-    ARM_CPU_TYPE_NAME("cortex-a35"),
-    ARM_CPU_TYPE_NAME("cortex-a55"),
-    ARM_CPU_TYPE_NAME("cortex-a72"),
-    ARM_CPU_TYPE_NAME("cortex-a76"),
-    ARM_CPU_TYPE_NAME("cortex-a710"),
-    ARM_CPU_TYPE_NAME("a64fx"),
-    ARM_CPU_TYPE_NAME("neoverse-n1"),
-    ARM_CPU_TYPE_NAME("neoverse-v1"),
-    ARM_CPU_TYPE_NAME("neoverse-n2"),
-#endif
-    ARM_CPU_TYPE_NAME("cortex-a53"),
-    ARM_CPU_TYPE_NAME("cortex-a57"),
-#if defined(CONFIG_KVM) || defined(CONFIG_HVF)
-    ARM_CPU_TYPE_NAME("host"),
-#endif
-    ARM_CPU_TYPE_NAME("max"),
-};
-
-static bool cpu_type_valid(const char *cpu)
-{
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(valid_cpus); i++) {
-        if (strcmp(cpu, valid_cpus[i]) == 0) {
-            return true;
-        }
-    }
-    return false;
-}
-
 static void create_randomness(MachineState *ms, const char *node)
 {
     struct {
@@ -2041,11 +2007,6 @@ static void machvirt_init(MachineState *machine)
     unsigned int smp_cpus = machine->smp.cpus;
     unsigned int max_cpus = machine->smp.max_cpus;
 
-    if (!cpu_type_valid(machine->cpu_type)) {
-        error_report("mach-virt: CPU type %s not supported", machine->cpu_type);
-        exit(1);
-    }
-
     possible_cpus = mc->possible_cpu_arch_ids(machine);
 
     /*
@@ -2939,6 +2900,28 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
+    static const char * const valid_cpu_types[] = {
+#ifdef CONFIG_TCG
+        ARM_CPU_TYPE_NAME("cortex-a7"),
+        ARM_CPU_TYPE_NAME("cortex-a15"),
+        ARM_CPU_TYPE_NAME("cortex-a35"),
+        ARM_CPU_TYPE_NAME("cortex-a55"),
+        ARM_CPU_TYPE_NAME("cortex-a72"),
+        ARM_CPU_TYPE_NAME("cortex-a76"),
+        ARM_CPU_TYPE_NAME("cortex-a710"),
+        ARM_CPU_TYPE_NAME("a64fx"),
+        ARM_CPU_TYPE_NAME("neoverse-n1"),
+        ARM_CPU_TYPE_NAME("neoverse-v1"),
+        ARM_CPU_TYPE_NAME("neoverse-n2"),
+#endif
+        ARM_CPU_TYPE_NAME("cortex-a53"),
+        ARM_CPU_TYPE_NAME("cortex-a57"),
+#if defined(CONFIG_KVM) || defined(CONFIG_HVF)
+        ARM_CPU_TYPE_NAME("host"),
+#endif
+        ARM_CPU_TYPE_NAME("max"),
+        NULL
+    };
 
     mc->init = machvirt_init;
     /* Start with max_cpus set to 512, which is the maximum supported by KVM.
@@ -2965,6 +2948,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
 #else
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("max");
 #endif
+    mc->valid_cpu_types = valid_cpu_types;
     mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
     mc->kvm_type = virt_kvm_type;
     assert(!mc->get_hotplug_handler);
-- 
2.42.0



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v9 7/9] hw/arm/sbsa-ref: Check CPU type in machine_run_board_init()
  2023-12-04  0:47 [PATCH v9 0/9] Unified CPU type check Gavin Shan
                   ` (5 preceding siblings ...)
  2023-12-04  0:47 ` [PATCH v9 6/9] hw/arm/virt: Check CPU type in machine_run_board_init() Gavin Shan
@ 2023-12-04  0:47 ` Gavin Shan
  2023-12-04  0:47 ` [PATCH v9 8/9] hw/arm: " Gavin Shan
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 19+ messages in thread
From: Gavin Shan @ 2023-12-04  0:47 UTC (permalink / raw)
  To: qemu-arm
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	philmd, armbru, wangyanan55, vijai, palmer, alistair.francis,
	bin.meng, liwei1518, dbarboza, zhiwei_liu, shan.gavin

Set mc->valid_cpu_types so that the user specified CPU type can
be validated in machine_run_board_init(). We needn't to do it
by ourselves.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/arm/sbsa-ref.c | 36 ++++++++++--------------------------
 1 file changed, 10 insertions(+), 26 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index f3c9704693..477dca0637 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -145,27 +145,6 @@ static const int sbsa_ref_irqmap[] = {
     [SBSA_GWDT_WS0] = 16,
 };
 
-static const char * const valid_cpus[] = {
-    ARM_CPU_TYPE_NAME("cortex-a57"),
-    ARM_CPU_TYPE_NAME("cortex-a72"),
-    ARM_CPU_TYPE_NAME("neoverse-n1"),
-    ARM_CPU_TYPE_NAME("neoverse-v1"),
-    ARM_CPU_TYPE_NAME("neoverse-n2"),
-    ARM_CPU_TYPE_NAME("max"),
-};
-
-static bool cpu_type_valid(const char *cpu)
-{
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(valid_cpus); i++) {
-        if (strcmp(cpu, valid_cpus[i]) == 0) {
-            return true;
-        }
-    }
-    return false;
-}
-
 static uint64_t sbsa_ref_cpu_mp_affinity(SBSAMachineState *sms, int idx)
 {
     uint8_t clustersz = ARM_DEFAULT_CPUS_PER_CLUSTER;
@@ -733,11 +712,6 @@ static void sbsa_ref_init(MachineState *machine)
     const CPUArchIdList *possible_cpus;
     int n, sbsa_max_cpus;
 
-    if (!cpu_type_valid(machine->cpu_type)) {
-        error_report("sbsa-ref: CPU type %s not supported", machine->cpu_type);
-        exit(1);
-    }
-
     if (kvm_enabled()) {
         error_report("sbsa-ref: KVM is not supported for this machine");
         exit(1);
@@ -898,10 +872,20 @@ static void sbsa_ref_instance_init(Object *obj)
 static void sbsa_ref_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    static const char * const valid_cpu_types[] = {
+        ARM_CPU_TYPE_NAME("cortex-a57"),
+        ARM_CPU_TYPE_NAME("cortex-a72"),
+        ARM_CPU_TYPE_NAME("neoverse-n1"),
+        ARM_CPU_TYPE_NAME("neoverse-v1"),
+        ARM_CPU_TYPE_NAME("neoverse-n2"),
+        ARM_CPU_TYPE_NAME("max"),
+        NULL,
+    };
 
     mc->init = sbsa_ref_init;
     mc->desc = "QEMU 'SBSA Reference' ARM Virtual Machine";
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("neoverse-n1");
+    mc->valid_cpu_types = valid_cpu_types;
     mc->max_cpus = 512;
     mc->pci_allow_0_address = true;
     mc->minimum_page_bits = 12;
-- 
2.42.0



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v9 8/9] hw/arm: Check CPU type in machine_run_board_init()
  2023-12-04  0:47 [PATCH v9 0/9] Unified CPU type check Gavin Shan
                   ` (6 preceding siblings ...)
  2023-12-04  0:47 ` [PATCH v9 7/9] hw/arm/sbsa-ref: " Gavin Shan
@ 2023-12-04  0:47 ` Gavin Shan
  2023-12-04  0:47 ` [PATCH v9 9/9] hw/riscv/shakti_c: " Gavin Shan
  2023-12-12  4:55 ` [PATCH v9 0/9] Unified CPU type check Gavin Shan
  9 siblings, 0 replies; 19+ messages in thread
From: Gavin Shan @ 2023-12-04  0:47 UTC (permalink / raw)
  To: qemu-arm
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	philmd, armbru, wangyanan55, vijai, palmer, alistair.francis,
	bin.meng, liwei1518, dbarboza, zhiwei_liu, shan.gavin

Set mc->valid_cpu_types so that the user specified CPU type can
be validated in machine_run_board_init(). We needn't to do it by
ourselves.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/bananapi_m2u.c   | 12 ++++++------
 hw/arm/cubieboard.c     | 12 ++++++------
 hw/arm/mps2-tz.c        | 26 ++++++++++++++++++++------
 hw/arm/mps2.c           | 26 ++++++++++++++++++++------
 hw/arm/msf2-som.c       | 12 ++++++------
 hw/arm/musca.c          | 12 +++++-------
 hw/arm/npcm7xx_boards.c | 12 +++++-------
 hw/arm/orangepi.c       | 12 ++++++------
 8 files changed, 74 insertions(+), 50 deletions(-)

diff --git a/hw/arm/bananapi_m2u.c b/hw/arm/bananapi_m2u.c
index 8f24b18d8c..0a4b6f29b1 100644
--- a/hw/arm/bananapi_m2u.c
+++ b/hw/arm/bananapi_m2u.c
@@ -71,12 +71,6 @@ static void bpim2u_init(MachineState *machine)
         exit(1);
     }
 
-    /* Only allow Cortex-A7 for this board */
-    if (strcmp(machine->cpu_type, ARM_CPU_TYPE_NAME("cortex-a7")) != 0) {
-        error_report("This board can only be used with cortex-a7 CPU");
-        exit(1);
-    }
-
     r40 = AW_R40(object_new(TYPE_AW_R40));
     object_property_add_child(OBJECT(machine), "soc", OBJECT(r40));
     object_unref(OBJECT(r40));
@@ -133,12 +127,18 @@ static void bpim2u_init(MachineState *machine)
 
 static void bpim2u_machine_init(MachineClass *mc)
 {
+    static const char * const valid_cpu_types[] = {
+        ARM_CPU_TYPE_NAME("cortex-a7"),
+        NULL
+    };
+
     mc->desc = "Bananapi M2U (Cortex-A7)";
     mc->init = bpim2u_init;
     mc->min_cpus = AW_R40_NUM_CPUS;
     mc->max_cpus = AW_R40_NUM_CPUS;
     mc->default_cpus = AW_R40_NUM_CPUS;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a7");
+    mc->valid_cpu_types = valid_cpu_types;
     mc->default_ram_size = 1 * GiB;
     mc->default_ram_id = "bpim2u.ram";
 }
diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
index 29146f5018..b976727eef 100644
--- a/hw/arm/cubieboard.c
+++ b/hw/arm/cubieboard.c
@@ -52,12 +52,6 @@ static void cubieboard_init(MachineState *machine)
         exit(1);
     }
 
-    /* Only allow Cortex-A8 for this board */
-    if (strcmp(machine->cpu_type, ARM_CPU_TYPE_NAME("cortex-a8")) != 0) {
-        error_report("This board can only be used with cortex-a8 CPU");
-        exit(1);
-    }
-
     a10 = AW_A10(object_new(TYPE_AW_A10));
     object_property_add_child(OBJECT(machine), "soc", OBJECT(a10));
     object_unref(OBJECT(a10));
@@ -114,8 +108,14 @@ static void cubieboard_init(MachineState *machine)
 
 static void cubieboard_machine_init(MachineClass *mc)
 {
+    static const char * const valid_cpu_types[] = {
+        ARM_CPU_TYPE_NAME("cortex-a8"),
+        NULL
+    };
+
     mc->desc = "cubietech cubieboard (Cortex-A8)";
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a8");
+    mc->valid_cpu_types = valid_cpu_types;
     mc->default_ram_size = 1 * GiB;
     mc->init = cubieboard_init;
     mc->block_default_type = IF_IDE;
diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index 668db5ed61..5d8cdc1a4c 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -813,12 +813,6 @@ static void mps2tz_common_init(MachineState *machine)
     int num_ppcs;
     int i;
 
-    if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) {
-        error_report("This board can only be used with CPU %s",
-                     mc->default_cpu_type);
-        exit(1);
-    }
-
     if (machine->ram_size != mc->default_ram_size) {
         char *sz = size_to_str(mc->default_ram_size);
         error_report("Invalid RAM size, should be %s", sz);
@@ -1318,6 +1312,10 @@ static void mps2tz_an505_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     MPS2TZMachineClass *mmc = MPS2TZ_MACHINE_CLASS(oc);
+    static const char * const valid_cpu_types[] = {
+        ARM_CPU_TYPE_NAME("cortex-m33"),
+        NULL
+    };
 
     mc->desc = "ARM MPS2 with AN505 FPGA image for Cortex-M33";
     mc->default_cpus = 1;
@@ -1325,6 +1323,7 @@ static void mps2tz_an505_class_init(ObjectClass *oc, void *data)
     mc->max_cpus = mc->default_cpus;
     mmc->fpga_type = FPGA_AN505;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m33");
+    mc->valid_cpu_types = valid_cpu_types;
     mmc->scc_id = 0x41045050;
     mmc->sysclk_frq = 20 * 1000 * 1000; /* 20MHz */
     mmc->apb_periph_frq = mmc->sysclk_frq;
@@ -1347,6 +1346,10 @@ static void mps2tz_an521_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     MPS2TZMachineClass *mmc = MPS2TZ_MACHINE_CLASS(oc);
+    static const char * const valid_cpu_types[] = {
+        ARM_CPU_TYPE_NAME("cortex-m33"),
+        NULL
+    };
 
     mc->desc = "ARM MPS2 with AN521 FPGA image for dual Cortex-M33";
     mc->default_cpus = 2;
@@ -1354,6 +1357,7 @@ static void mps2tz_an521_class_init(ObjectClass *oc, void *data)
     mc->max_cpus = mc->default_cpus;
     mmc->fpga_type = FPGA_AN521;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m33");
+    mc->valid_cpu_types = valid_cpu_types;
     mmc->scc_id = 0x41045210;
     mmc->sysclk_frq = 20 * 1000 * 1000; /* 20MHz */
     mmc->apb_periph_frq = mmc->sysclk_frq;
@@ -1376,6 +1380,10 @@ static void mps3tz_an524_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     MPS2TZMachineClass *mmc = MPS2TZ_MACHINE_CLASS(oc);
+    static const char * const valid_cpu_types[] = {
+        ARM_CPU_TYPE_NAME("cortex-m33"),
+        NULL
+    };
 
     mc->desc = "ARM MPS3 with AN524 FPGA image for dual Cortex-M33";
     mc->default_cpus = 2;
@@ -1383,6 +1391,7 @@ static void mps3tz_an524_class_init(ObjectClass *oc, void *data)
     mc->max_cpus = mc->default_cpus;
     mmc->fpga_type = FPGA_AN524;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m33");
+    mc->valid_cpu_types = valid_cpu_types;
     mmc->scc_id = 0x41045240;
     mmc->sysclk_frq = 32 * 1000 * 1000; /* 32MHz */
     mmc->apb_periph_frq = mmc->sysclk_frq;
@@ -1410,6 +1419,10 @@ static void mps3tz_an547_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     MPS2TZMachineClass *mmc = MPS2TZ_MACHINE_CLASS(oc);
+    static const char * const valid_cpu_types[] = {
+        ARM_CPU_TYPE_NAME("cortex-m55"),
+        NULL
+    };
 
     mc->desc = "ARM MPS3 with AN547 FPGA image for Cortex-M55";
     mc->default_cpus = 1;
@@ -1417,6 +1430,7 @@ static void mps3tz_an547_class_init(ObjectClass *oc, void *data)
     mc->max_cpus = mc->default_cpus;
     mmc->fpga_type = FPGA_AN547;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m55");
+    mc->valid_cpu_types = valid_cpu_types;
     mmc->scc_id = 0x41055470;
     mmc->sysclk_frq = 32 * 1000 * 1000; /* 32MHz */
     mmc->apb_periph_frq = 25 * 1000 * 1000; /* 25MHz */
diff --git a/hw/arm/mps2.c b/hw/arm/mps2.c
index 292a180ad2..bd873cc5de 100644
--- a/hw/arm/mps2.c
+++ b/hw/arm/mps2.c
@@ -142,12 +142,6 @@ static void mps2_common_init(MachineState *machine)
     QList *oscclk;
     int i;
 
-    if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) {
-        error_report("This board can only be used with CPU %s",
-                     mc->default_cpu_type);
-        exit(1);
-    }
-
     if (machine->ram_size != mc->default_ram_size) {
         char *sz = size_to_str(mc->default_ram_size);
         error_report("Invalid RAM size, should be %s", sz);
@@ -484,10 +478,15 @@ static void mps2_an385_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     MPS2MachineClass *mmc = MPS2_MACHINE_CLASS(oc);
+    static const char * const valid_cpu_types[] = {
+        ARM_CPU_TYPE_NAME("cortex-m3"),
+        NULL
+    };
 
     mc->desc = "ARM MPS2 with AN385 FPGA image for Cortex-M3";
     mmc->fpga_type = FPGA_AN385;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m3");
+    mc->valid_cpu_types = valid_cpu_types;
     mmc->scc_id = 0x41043850;
     mmc->psram_base = 0x21000000;
     mmc->ethernet_base = 0x40200000;
@@ -498,10 +497,15 @@ static void mps2_an386_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     MPS2MachineClass *mmc = MPS2_MACHINE_CLASS(oc);
+    static const char * const valid_cpu_types[] = {
+        ARM_CPU_TYPE_NAME("cortex-m4"),
+        NULL
+    };
 
     mc->desc = "ARM MPS2 with AN386 FPGA image for Cortex-M4";
     mmc->fpga_type = FPGA_AN386;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m4");
+    mc->valid_cpu_types = valid_cpu_types;
     mmc->scc_id = 0x41043860;
     mmc->psram_base = 0x21000000;
     mmc->ethernet_base = 0x40200000;
@@ -512,10 +516,15 @@ static void mps2_an500_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     MPS2MachineClass *mmc = MPS2_MACHINE_CLASS(oc);
+    static const char * const valid_cpu_types[] = {
+        ARM_CPU_TYPE_NAME("cortex-m7"),
+        NULL
+    };
 
     mc->desc = "ARM MPS2 with AN500 FPGA image for Cortex-M7";
     mmc->fpga_type = FPGA_AN500;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m7");
+    mc->valid_cpu_types = valid_cpu_types;
     mmc->scc_id = 0x41045000;
     mmc->psram_base = 0x60000000;
     mmc->ethernet_base = 0xa0000000;
@@ -526,10 +535,15 @@ static void mps2_an511_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     MPS2MachineClass *mmc = MPS2_MACHINE_CLASS(oc);
+    static const char * const valid_cpu_types[] = {
+        ARM_CPU_TYPE_NAME("cortex-m3"),
+        NULL
+    };
 
     mc->desc = "ARM MPS2 with AN511 DesignStart FPGA image for Cortex-M3";
     mmc->fpga_type = FPGA_AN511;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m3");
+    mc->valid_cpu_types = valid_cpu_types;
     mmc->scc_id = 0x41045110;
     mmc->psram_base = 0x21000000;
     mmc->ethernet_base = 0x40200000;
diff --git a/hw/arm/msf2-som.c b/hw/arm/msf2-som.c
index 7b3106c790..eb74b23797 100644
--- a/hw/arm/msf2-som.c
+++ b/hw/arm/msf2-som.c
@@ -55,12 +55,6 @@ static void emcraft_sf2_s2s010_init(MachineState *machine)
     MemoryRegion *ddr = g_new(MemoryRegion, 1);
     Clock *m3clk;
 
-    if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) {
-        error_report("This board can only be used with CPU %s",
-                     mc->default_cpu_type);
-        exit(1);
-    }
-
     memory_region_init_ram(ddr, NULL, "ddr-ram", DDR_SIZE,
                            &error_fatal);
     memory_region_add_subregion(sysmem, DDR_BASE_ADDRESS, ddr);
@@ -106,9 +100,15 @@ static void emcraft_sf2_s2s010_init(MachineState *machine)
 
 static void emcraft_sf2_machine_init(MachineClass *mc)
 {
+    static const char * const valid_cpu_types[] = {
+        ARM_CPU_TYPE_NAME("cortex-m3"),
+        NULL
+    };
+
     mc->desc = "SmartFusion2 SOM kit from Emcraft (M2S010)";
     mc->init = emcraft_sf2_s2s010_init;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m3");
+    mc->valid_cpu_types = valid_cpu_types;
 }
 
 DEFINE_MACHINE("emcraft-sf2", emcraft_sf2_machine_init)
diff --git a/hw/arm/musca.c b/hw/arm/musca.c
index 6eeee57c9d..770ec1a15c 100644
--- a/hw/arm/musca.c
+++ b/hw/arm/musca.c
@@ -355,7 +355,6 @@ static void musca_init(MachineState *machine)
 {
     MuscaMachineState *mms = MUSCA_MACHINE(machine);
     MuscaMachineClass *mmc = MUSCA_MACHINE_GET_CLASS(mms);
-    MachineClass *mc = MACHINE_GET_CLASS(machine);
     MemoryRegion *system_memory = get_system_memory();
     DeviceState *ssedev;
     DeviceState *dev_splitter;
@@ -366,12 +365,6 @@ static void musca_init(MachineState *machine)
     assert(mmc->num_irqs <= MUSCA_NUMIRQ_MAX);
     assert(mmc->num_mpcs <= MUSCA_MPC_MAX);
 
-    if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) {
-        error_report("This board can only be used with CPU %s",
-                     mc->default_cpu_type);
-        exit(1);
-    }
-
     mms->sysclk = clock_new(OBJECT(machine), "SYSCLK");
     clock_set_hz(mms->sysclk, SYSCLK_FRQ);
     mms->s32kclk = clock_new(OBJECT(machine), "S32KCLK");
@@ -604,11 +597,16 @@ static void musca_init(MachineState *machine)
 static void musca_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    static const char * const valid_cpu_types[] = {
+        ARM_CPU_TYPE_NAME("cortex-m33"),
+        NULL
+    };
 
     mc->default_cpus = 2;
     mc->min_cpus = mc->default_cpus;
     mc->max_cpus = mc->default_cpus;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m33");
+    mc->valid_cpu_types = valid_cpu_types;
     mc->init = musca_init;
 }
 
diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
index 2aef579aac..2999b8b96d 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -121,15 +121,8 @@ static NPCM7xxState *npcm7xx_create_soc(MachineState *machine,
                                         uint32_t hw_straps)
 {
     NPCM7xxMachineClass *nmc = NPCM7XX_MACHINE_GET_CLASS(machine);
-    MachineClass *mc = MACHINE_CLASS(nmc);
     Object *obj;
 
-    if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) {
-        error_report("This board can only be used with %s",
-                     mc->default_cpu_type);
-        exit(1);
-    }
-
     obj = object_new_with_props(nmc->soc_type, OBJECT(machine), "soc",
                                 &error_abort, NULL);
     object_property_set_uint(obj, "power-on-straps", hw_straps, &error_abort);
@@ -463,12 +456,17 @@ static void npcm7xx_set_soc_type(NPCM7xxMachineClass *nmc, const char *type)
 static void npcm7xx_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    static const char * const valid_cpu_types[] = {
+        ARM_CPU_TYPE_NAME("cortex-a9"),
+        NULL
+    };
 
     mc->no_floppy = 1;
     mc->no_cdrom = 1;
     mc->no_parallel = 1;
     mc->default_ram_id = "ram";
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a9");
+    mc->valid_cpu_types = valid_cpu_types;
 }
 
 /*
diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
index f3784d45ca..77e328191d 100644
--- a/hw/arm/orangepi.c
+++ b/hw/arm/orangepi.c
@@ -49,12 +49,6 @@ static void orangepi_init(MachineState *machine)
         exit(1);
     }
 
-    /* Only allow Cortex-A7 for this board */
-    if (strcmp(machine->cpu_type, ARM_CPU_TYPE_NAME("cortex-a7")) != 0) {
-        error_report("This board can only be used with cortex-a7 CPU");
-        exit(1);
-    }
-
     h3 = AW_H3(object_new(TYPE_AW_H3));
     object_property_add_child(OBJECT(machine), "soc", OBJECT(h3));
     object_unref(OBJECT(h3));
@@ -111,6 +105,11 @@ static void orangepi_init(MachineState *machine)
 
 static void orangepi_machine_init(MachineClass *mc)
 {
+    static const char * const valid_cpu_types[] = {
+        ARM_CPU_TYPE_NAME("cortex-a7"),
+        NULL
+    };
+
     mc->desc = "Orange Pi PC (Cortex-A7)";
     mc->init = orangepi_init;
     mc->block_default_type = IF_SD;
@@ -119,6 +118,7 @@ static void orangepi_machine_init(MachineClass *mc)
     mc->max_cpus = AW_H3_NUM_CPUS;
     mc->default_cpus = AW_H3_NUM_CPUS;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a7");
+    mc->valid_cpu_types = valid_cpu_types;
     mc->default_ram_size = 1 * GiB;
     mc->default_ram_id = "orangepi.ram";
 }
-- 
2.42.0



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v9 9/9] hw/riscv/shakti_c: Check CPU type in machine_run_board_init()
  2023-12-04  0:47 [PATCH v9 0/9] Unified CPU type check Gavin Shan
                   ` (7 preceding siblings ...)
  2023-12-04  0:47 ` [PATCH v9 8/9] hw/arm: " Gavin Shan
@ 2023-12-04  0:47 ` Gavin Shan
  2023-12-12  4:55 ` [PATCH v9 0/9] Unified CPU type check Gavin Shan
  9 siblings, 0 replies; 19+ messages in thread
From: Gavin Shan @ 2023-12-04  0:47 UTC (permalink / raw)
  To: qemu-arm
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	philmd, armbru, wangyanan55, vijai, palmer, alistair.francis,
	bin.meng, liwei1518, dbarboza, zhiwei_liu, shan.gavin

Set mc->valid_cpu_types so that the user specified CPU type can
be validated in machine_run_board_init(). We needn't to do it
by ourselves.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/riscv/shakti_c.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/riscv/shakti_c.c b/hw/riscv/shakti_c.c
index 12ea74b032..3888034c2b 100644
--- a/hw/riscv/shakti_c.c
+++ b/hw/riscv/shakti_c.c
@@ -28,7 +28,6 @@
 #include "exec/address-spaces.h"
 #include "hw/riscv/boot.h"
 
-
 static const struct MemmapEntry {
     hwaddr base;
     hwaddr size;
@@ -47,12 +46,6 @@ static void shakti_c_machine_state_init(MachineState *mstate)
     ShaktiCMachineState *sms = RISCV_SHAKTI_MACHINE(mstate);
     MemoryRegion *system_memory = get_system_memory();
 
-    /* Allow only Shakti C CPU for this platform */
-    if (strcmp(mstate->cpu_type, TYPE_RISCV_CPU_SHAKTI_C) != 0) {
-        error_report("This board can only be used with Shakti C CPU");
-        exit(1);
-    }
-
     /* Initialize SoC */
     object_initialize_child(OBJECT(mstate), "soc", &sms->soc,
                             TYPE_RISCV_SHAKTI_SOC);
@@ -82,9 +75,15 @@ static void shakti_c_machine_instance_init(Object *obj)
 static void shakti_c_machine_class_init(ObjectClass *klass, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(klass);
+    static const char * const valid_cpu_types[] = {
+        RISCV_CPU_TYPE_NAME("shakti-c"),
+        NULL
+    };
+
     mc->desc = "RISC-V Board compatible with Shakti SDK";
     mc->init = shakti_c_machine_state_init;
     mc->default_cpu_type = TYPE_RISCV_CPU_SHAKTI_C;
+    mc->valid_cpu_types = valid_cpu_types;
     mc->default_ram_id = "riscv.shakti.c.ram";
 }
 
-- 
2.42.0



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v9 0/9] Unified CPU type check
  2023-12-04  0:47 [PATCH v9 0/9] Unified CPU type check Gavin Shan
                   ` (8 preceding siblings ...)
  2023-12-04  0:47 ` [PATCH v9 9/9] hw/riscv/shakti_c: " Gavin Shan
@ 2023-12-12  4:55 ` Gavin Shan
  2023-12-13 10:08   ` Philippe Mathieu-Daudé
  9 siblings, 1 reply; 19+ messages in thread
From: Gavin Shan @ 2023-12-12  4:55 UTC (permalink / raw)
  To: qemu-arm
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	philmd, armbru, wangyanan55, vijai, palmer, alistair.francis,
	bin.meng, liwei1518, dbarboza, zhiwei_liu, shan.gavin

Hi Phil,

On 12/4/23 10:47, Gavin Shan wrote:
> This series bases on Phil's repository because the prepatory commits
> have been queued to the branch.
> 
>    https://gitlab.com/philmd/qemu.git (branch: cpus-next)
> 
> There are two places where the user specified CPU type is checked to see
> if it's supported or allowed by the board: machine_run_board_init() and
> mc->init(). We don't have to maintain two duplicate sets of logic. This
> series intends to move the check to machine_run_board_init() so that we
> have unified CPU type check.
> 
> This series can be checked out from:
> 
>    git@github.com:gwshan/qemu.git (branch: kvm/cpu-type)
> 
> PATCH[1-4] refactors and improves the logic to validate CPU type in
>             machine_run_board_init()
> PATCH[5-9] validates the CPU type in machine_run_board_init() for the
>             individual boards
> 
> v6: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00768.html
> v7: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01045.html
> v8: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01168.html
> 

Ping to see if there is a chance to queue it up before the Chrismas? :)

Thanks,
Gavin



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v9 0/9] Unified CPU type check
  2023-12-12  4:55 ` [PATCH v9 0/9] Unified CPU type check Gavin Shan
@ 2023-12-13 10:08   ` Philippe Mathieu-Daudé
  2023-12-13 10:54     ` Gavin Shan
  0 siblings, 1 reply; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-12-13 10:08 UTC (permalink / raw)
  To: Gavin Shan, qemu-arm
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	armbru, wangyanan55, vijai, palmer, alistair.francis, bin.meng,
	liwei1518, dbarboza, zhiwei_liu, shan.gavin

On 12/12/23 05:55, Gavin Shan wrote:
> Hi Phil,
> 
> On 12/4/23 10:47, Gavin Shan wrote:
>> This series bases on Phil's repository because the prepatory commits
>> have been queued to the branch.
>>
>>    https://gitlab.com/philmd/qemu.git (branch: cpus-next)
>>
>> There are two places where the user specified CPU type is checked to see
>> if it's supported or allowed by the board: machine_run_board_init() and
>> mc->init(). We don't have to maintain two duplicate sets of logic. This
>> series intends to move the check to machine_run_board_init() so that we
>> have unified CPU type check.
>>
>> This series can be checked out from:
>>
>>    git@github.com:gwshan/qemu.git (branch: kvm/cpu-type)
>>
>> PATCH[1-4] refactors and improves the logic to validate CPU type in
>>             machine_run_board_init()
>> PATCH[5-9] validates the CPU type in machine_run_board_init() for the
>>             individual boards
>>
>> v6: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00768.html
>> v7: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01045.html
>> v8: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01168.html
>>
> 
> Ping to see if there is a chance to queue it up before the Chrismas? :)

Series queued. "Before" Christmas will depend on the final release tag.

Thanks for the various iterations,

Phil.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v9 0/9] Unified CPU type check
  2023-12-13 10:08   ` Philippe Mathieu-Daudé
@ 2023-12-13 10:54     ` Gavin Shan
  2024-01-05 22:12       ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 19+ messages in thread
From: Gavin Shan @ 2023-12-13 10:54 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-arm
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	armbru, wangyanan55, vijai, palmer, alistair.francis, bin.meng,
	liwei1518, dbarboza, zhiwei_liu, shan.gavin


On 12/13/23 20:08, Philippe Mathieu-Daudé wrote:
> On 12/12/23 05:55, Gavin Shan wrote:
>> On 12/4/23 10:47, Gavin Shan wrote:
>>> This series bases on Phil's repository because the prepatory commits
>>> have been queued to the branch.
>>>
>>>    https://gitlab.com/philmd/qemu.git (branch: cpus-next)
>>>
>>> There are two places where the user specified CPU type is checked to see
>>> if it's supported or allowed by the board: machine_run_board_init() and
>>> mc->init(). We don't have to maintain two duplicate sets of logic. This
>>> series intends to move the check to machine_run_board_init() so that we
>>> have unified CPU type check.
>>>
>>> This series can be checked out from:
>>>
>>>    git@github.com:gwshan/qemu.git (branch: kvm/cpu-type)
>>>
>>> PATCH[1-4] refactors and improves the logic to validate CPU type in
>>>             machine_run_board_init()
>>> PATCH[5-9] validates the CPU type in machine_run_board_init() for the
>>>             individual boards
>>>
>>> v6: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00768.html
>>> v7: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01045.html
>>> v8: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01168.html
>>>
>>
>> Ping to see if there is a chance to queue it up before the Chrismas? :)
> 
> Series queued. "Before" Christmas will depend on the final release tag.
> 
> Thanks for the various iterations,
> 

Phil, thank you for you continuous reviews and valuable comments.

Yes, the final merge to master branch depends on the release plan.
'queue' meant to merge the series to your 'cpus-next' branch ;-)

Thanks,
Gavin



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v9 1/9] machine: Use error handling when CPU type is checked
  2023-12-04  0:47 ` [PATCH v9 1/9] machine: Use error handling when CPU type is checked Gavin Shan
@ 2024-01-05 11:00   ` Philippe Mathieu-Daudé
  2024-01-08  1:11     ` Gavin Shan
  0 siblings, 1 reply; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-01-05 11:00 UTC (permalink / raw)
  To: Gavin Shan, armbru
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	wangyanan55, vijai, palmer, alistair.francis, bin.meng, liwei1518,
	dbarboza, zhiwei_liu, shan.gavin, qemu-arm

On 4/12/23 01:47, Gavin Shan wrote:
> Functions that use an Error **errp parameter to return errors should
> not also report them to the user, because reporting is the caller's
> job. The principle is violated by machine_run_board_init() because
> it calls error_report(), error_printf(), and exit(1) when the machine
> doesn't support the requested CPU type.
> 
> Clean this up by using error_setg() and error_append_hint() instead.
> No functional change, as the only caller passes &error_fatal.
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
> v9: Improved change log                                  (Markus)
> ---
>   hw/core/machine.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 0c17398141..bde7f4af6d 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1466,15 +1466,16 @@ void machine_run_board_init(MachineState *machine, const char *mem_path, Error *
>   
>           if (!machine_class->valid_cpu_types[i]) {
>               /* 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]);
> +            error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
> +            error_append_hint(errp, "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_append_hint(errp, ", %s",
> +                                  machine_class->valid_cpu_types[i]);
>               }
> -            error_printf("\n");
>   
> -            exit(1);
> +            error_append_hint(&errp, "\n");

This doesn't build:

hw/core/machine.c:1488:31: error: incompatible pointer types passing 
'Error ***' (aka 'struct Error ***') to parameter of type 'Error *const 
*' (aka 'struct Error *const *'); remove & 
[-Werror,-Wincompatible-pointer-types]
             error_append_hint(&errp, "\n");
                               ^~~~~

> +            return;
>           }
>       }
>   

Squashing:
-- >8 --
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 021044aaaf..1898d1d1d7 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1487,3 +1487,3 @@ void machine_run_board_init(MachineState *machine, 
const char *mem_path, Error *

-            error_append_hint(&errp, "\n");
+            error_append_hint(errp, "\n");
              return;
---


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v9 3/9] machine: Improve is_cpu_type_supported()
  2023-12-04  0:47 ` [PATCH v9 3/9] machine: Improve is_cpu_type_supported() Gavin Shan
@ 2024-01-05 22:09   ` Philippe Mathieu-Daudé
  2024-01-08  1:21     ` Gavin Shan
  0 siblings, 1 reply; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-01-05 22:09 UTC (permalink / raw)
  To: Gavin Shan, qemu-arm
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	armbru, wangyanan55, vijai, palmer, alistair.francis, bin.meng,
	liwei1518, dbarboza, zhiwei_liu, shan.gavin

On 4/12/23 01:47, Gavin Shan wrote:
> It's no sense to check the CPU type when mc->valid_cpu_types[0] is
> NULL, which is a program error. Raise an assert on this.
> 
> A precise hint for the error message is given when mc->valid_cpu_types[0]
> is the only valid entry. Besides, enumeration on mc->valid_cpu_types[0]
> when we have mutiple valid entries there is avoided to increase the code
> readability, as suggested by Philippe Mathieu-Daudé.
> 
> Besides, @cc comes from machine->cpu_type or mc->default_cpu_type. For
> the later case, it can be NULL and it's also a program error. We should
> use assert() in this case.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> v9: assert(mc->valid_cpu_types[0] != NULL)                   (Phil)
>      assert(cc != NULL);                                      (Phil)
> ---
>   hw/core/machine.c | 20 ++++++++++++++------
>   1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 1797e002f9..4ae9aaee8e 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1400,6 +1400,7 @@ static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
>        * type is provided through '-cpu' option.
>        */
>       if (mc->valid_cpu_types && machine->cpu_type) {
> +        assert(mc->valid_cpu_types[0] != NULL);
>           for (i = 0; mc->valid_cpu_types[i]; i++) {
>               if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
>                   break;
> @@ -1409,20 +1410,27 @@ static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
>           /* The user specified CPU type isn't valid */
>           if (!mc->valid_cpu_types[i]) {
>               error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
> -            error_append_hint(errp, "The valid types are: %s",
> -                              mc->valid_cpu_types[0]);
> -            for (i = 1; mc->valid_cpu_types[i]; i++) {
> -                error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
> +            if (!mc->valid_cpu_types[1]) {
> +                error_append_hint(errp, "The only valid type is: %s\n",
> +                                  mc->valid_cpu_types[0]);
> +            } else {
> +                error_append_hint(errp, "The valid types are: ");
> +                for (i = 0; mc->valid_cpu_types[i]; i++) {
> +                    error_append_hint(errp, "%s%s",
> +                                      mc->valid_cpu_types[i],
> +                                      mc->valid_cpu_types[i + 1] ? ", " : "");
> +                }
> +                error_append_hint(errp, "\n");
>               }
>   
> -            error_append_hint(errp, "\n");
>               return false;
>           }
>       }
>   
>       /* Check if CPU type is deprecated and warn if so */
>       cc = CPU_CLASS(oc);
> -    if (cc && cc->deprecation_note) {
> +    assert(cc != NULL);
> +    if (cc->deprecation_note) {
>           warn_report("CPU model %s is deprecated -- %s",
>                       machine->cpu_type, cc->deprecation_note);
>       }

Since we were getting:

$ qemu-system-s390x -M none
QEMU 8.2.50 monitor - type 'help' for more information
qemu-system-s390x: ../../hw/core/machine.c:1444: _Bool 
is_cpu_type_supported(const MachineState *, Error **): Assertion `cc != 
NULL' failed.
Aborted (core dumped)

I added a check on mc->valid_cpu_types before calling
is_cpu_type_supported() in the previous patch. See commit acbadc5a29.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v9 0/9] Unified CPU type check
  2023-12-13 10:54     ` Gavin Shan
@ 2024-01-05 22:12       ` Philippe Mathieu-Daudé
  2024-01-08  1:24         ` Gavin Shan
  0 siblings, 1 reply; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-01-05 22:12 UTC (permalink / raw)
  To: Gavin Shan, qemu-arm
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	armbru, wangyanan55, vijai, palmer, alistair.francis, bin.meng,
	liwei1518, dbarboza, zhiwei_liu, shan.gavin

Hi Gavin,

On 13/12/23 11:54, Gavin Shan wrote:
> On 12/13/23 20:08, Philippe Mathieu-Daudé wrote:
>> On 12/12/23 05:55, Gavin Shan wrote:
>>> On 12/4/23 10:47, Gavin Shan wrote:
>>>> This series bases on Phil's repository because the prepatory commits
>>>> have been queued to the branch.
>>>>
>>>>    https://gitlab.com/philmd/qemu.git (branch: cpus-next)
>>>>
>>>> There are two places where the user specified CPU type is checked to 
>>>> see
>>>> if it's supported or allowed by the board: machine_run_board_init() and
>>>> mc->init(). We don't have to maintain two duplicate sets of logic. This
>>>> series intends to move the check to machine_run_board_init() so that we
>>>> have unified CPU type check.
>>>>
>>>> This series can be checked out from:
>>>>
>>>>    git@github.com:gwshan/qemu.git (branch: kvm/cpu-type)
>>>>
>>>> PATCH[1-4] refactors and improves the logic to validate CPU type in
>>>>             machine_run_board_init()
>>>> PATCH[5-9] validates the CPU type in machine_run_board_init() for the
>>>>             individual boards
>>>>
>>>> v6: 
>>>> https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00768.html
>>>> v7: 
>>>> https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01045.html
>>>> v8: 
>>>> https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01168.html
>>>>
>>>
>>> Ping to see if there is a chance to queue it up before the Chrismas? :)
>>
>> Series queued. "Before" Christmas will depend on the final release tag.
>>
>> Thanks for the various iterations,
>>
> 
> Phil, thank you for you continuous reviews and valuable comments.
> 
> Yes, the final merge to master branch depends on the release plan.
> 'queue' meant to merge the series to your 'cpus-next' branch ;-)

I had to fix 3 different issues caught by our CI. Next time please
run your series on GitLab CI, you just have to push your branch and
wait for the result :)

Now merged as 445946f4dd..cd75cc6337.

Happy new year!


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v9 1/9] machine: Use error handling when CPU type is checked
  2024-01-05 11:00   ` Philippe Mathieu-Daudé
@ 2024-01-08  1:11     ` Gavin Shan
  0 siblings, 0 replies; 19+ messages in thread
From: Gavin Shan @ 2024-01-08  1:11 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, armbru
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	wangyanan55, vijai, palmer, alistair.francis, bin.meng, liwei1518,
	dbarboza, zhiwei_liu, shan.gavin, qemu-arm

On 1/5/24 21:00, Philippe Mathieu-Daudé wrote:
> On 4/12/23 01:47, Gavin Shan wrote:
>> Functions that use an Error **errp parameter to return errors should
>> not also report them to the user, because reporting is the caller's
>> job. The principle is violated by machine_run_board_init() because
>> it calls error_report(), error_printf(), and exit(1) when the machine
>> doesn't support the requested CPU type.
>>
>> Clean this up by using error_setg() and error_append_hint() instead.
>> No functional change, as the only caller passes &error_fatal.
>>
>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> v9: Improved change log                                  (Markus)
>> ---
>>   hw/core/machine.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 0c17398141..bde7f4af6d 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -1466,15 +1466,16 @@ void machine_run_board_init(MachineState *machine, const char *mem_path, Error *
>>           if (!machine_class->valid_cpu_types[i]) {
>>               /* 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]);
>> +            error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
>> +            error_append_hint(errp, "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_append_hint(errp, ", %s",
>> +                                  machine_class->valid_cpu_types[i]);
>>               }
>> -            error_printf("\n");
>> -            exit(1);
>> +            error_append_hint(&errp, "\n");
> 
> This doesn't build:
> 
> hw/core/machine.c:1488:31: error: incompatible pointer types passing 'Error ***' (aka 'struct Error ***') to parameter of type 'Error *const *' (aka 'struct Error *const *'); remove & [-Werror,-Wincompatible-pointer-types]
>              error_append_hint(&errp, "\n");
>                                ^~~~~
> 

Yes, &errp should have been errp. The problematic code was carried from
previous revisions and has been corrected by PATCH[2/9]. It's how I missed
the building error. Thanks for fixing it up!

Thanks,
Gavin

>> +            return;
>>           }
>>       }
> 
> Squashing:
> -- >8 --
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 021044aaaf..1898d1d1d7 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1487,3 +1487,3 @@ void machine_run_board_init(MachineState *machine, const char *mem_path, Error *
> 
> -            error_append_hint(&errp, "\n");
> +            error_append_hint(errp, "\n");
>               return;
> ---
> 



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v9 3/9] machine: Improve is_cpu_type_supported()
  2024-01-05 22:09   ` Philippe Mathieu-Daudé
@ 2024-01-08  1:21     ` Gavin Shan
  0 siblings, 0 replies; 19+ messages in thread
From: Gavin Shan @ 2024-01-08  1:21 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-arm
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	armbru, wangyanan55, vijai, palmer, alistair.francis, bin.meng,
	liwei1518, dbarboza, zhiwei_liu, shan.gavin

On 1/6/24 08:09, Philippe Mathieu-Daudé wrote:
> On 4/12/23 01:47, Gavin Shan wrote:
>> It's no sense to check the CPU type when mc->valid_cpu_types[0] is
>> NULL, which is a program error. Raise an assert on this.
>>
>> A precise hint for the error message is given when mc->valid_cpu_types[0]
>> is the only valid entry. Besides, enumeration on mc->valid_cpu_types[0]
>> when we have mutiple valid entries there is avoided to increase the code
>> readability, as suggested by Philippe Mathieu-Daudé.
>>
>> Besides, @cc comes from machine->cpu_type or mc->default_cpu_type. For
>> the later case, it can be NULL and it's also a program error. We should
>> use assert() in this case.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> v9: assert(mc->valid_cpu_types[0] != NULL)                   (Phil)
>>      assert(cc != NULL);                                      (Phil)
>> ---
>>   hw/core/machine.c | 20 ++++++++++++++------
>>   1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 1797e002f9..4ae9aaee8e 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -1400,6 +1400,7 @@ static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
>>        * type is provided through '-cpu' option.
>>        */
>>       if (mc->valid_cpu_types && machine->cpu_type) {
>> +        assert(mc->valid_cpu_types[0] != NULL);
>>           for (i = 0; mc->valid_cpu_types[i]; i++) {
>>               if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
>>                   break;
>> @@ -1409,20 +1410,27 @@ static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
>>           /* The user specified CPU type isn't valid */
>>           if (!mc->valid_cpu_types[i]) {
>>               error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
>> -            error_append_hint(errp, "The valid types are: %s",
>> -                              mc->valid_cpu_types[0]);
>> -            for (i = 1; mc->valid_cpu_types[i]; i++) {
>> -                error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
>> +            if (!mc->valid_cpu_types[1]) {
>> +                error_append_hint(errp, "The only valid type is: %s\n",
>> +                                  mc->valid_cpu_types[0]);
>> +            } else {
>> +                error_append_hint(errp, "The valid types are: ");
>> +                for (i = 0; mc->valid_cpu_types[i]; i++) {
>> +                    error_append_hint(errp, "%s%s",
>> +                                      mc->valid_cpu_types[i],
>> +                                      mc->valid_cpu_types[i + 1] ? ", " : "");
>> +                }
>> +                error_append_hint(errp, "\n");
>>               }
>> -            error_append_hint(errp, "\n");
>>               return false;
>>           }
>>       }
>>       /* Check if CPU type is deprecated and warn if so */
>>       cc = CPU_CLASS(oc);
>> -    if (cc && cc->deprecation_note) {
>> +    assert(cc != NULL);
>> +    if (cc->deprecation_note) {
>>           warn_report("CPU model %s is deprecated -- %s",
>>                       machine->cpu_type, cc->deprecation_note);
>>       }
> 
> Since we were getting:
> 
> $ qemu-system-s390x -M none
> QEMU 8.2.50 monitor - type 'help' for more information
> qemu-system-s390x: ../../hw/core/machine.c:1444: _Bool is_cpu_type_supported(const MachineState *, Error **): Assertion `cc != NULL' failed.
> Aborted (core dumped)
> 
> I added a check on mc->valid_cpu_types before calling
> is_cpu_type_supported() in the previous patch. See commit acbadc5a29.
> 

Phil, thanks for fixing it up and it looks good to me.

Thanks,
Gavin



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v9 0/9] Unified CPU type check
  2024-01-05 22:12       ` Philippe Mathieu-Daudé
@ 2024-01-08  1:24         ` Gavin Shan
  0 siblings, 0 replies; 19+ messages in thread
From: Gavin Shan @ 2024-01-08  1:24 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-arm
  Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
	strahinja.p.jankovic, imammedo, kfting, wuhaotsh, nieklinnenbank,
	rad, quic_llindhol, marcin.juszkiewicz, eduardo, marcel.apfelbaum,
	armbru, wangyanan55, vijai, palmer, alistair.francis, bin.meng,
	liwei1518, dbarboza, zhiwei_liu, shan.gavin

Hi Phil,

On 1/6/24 08:12, Philippe Mathieu-Daudé wrote:
> On 13/12/23 11:54, Gavin Shan wrote:
>> On 12/13/23 20:08, Philippe Mathieu-Daudé wrote:
>>> On 12/12/23 05:55, Gavin Shan wrote:
>>>> On 12/4/23 10:47, Gavin Shan wrote:
>>>>> This series bases on Phil's repository because the prepatory commits
>>>>> have been queued to the branch.
>>>>>
>>>>>    https://gitlab.com/philmd/qemu.git (branch: cpus-next)
>>>>>
>>>>> There are two places where the user specified CPU type is checked to see
>>>>> if it's supported or allowed by the board: machine_run_board_init() and
>>>>> mc->init(). We don't have to maintain two duplicate sets of logic. This
>>>>> series intends to move the check to machine_run_board_init() so that we
>>>>> have unified CPU type check.
>>>>>
>>>>> This series can be checked out from:
>>>>>
>>>>>    git@github.com:gwshan/qemu.git (branch: kvm/cpu-type)
>>>>>
>>>>> PATCH[1-4] refactors and improves the logic to validate CPU type in
>>>>>             machine_run_board_init()
>>>>> PATCH[5-9] validates the CPU type in machine_run_board_init() for the
>>>>>             individual boards
>>>>>
>>>>> v6: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00768.html
>>>>> v7: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01045.html
>>>>> v8: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01168.html
>>>>>
>>>>
>>>> Ping to see if there is a chance to queue it up before the Chrismas? :)
>>>
>>> Series queued. "Before" Christmas will depend on the final release tag.
>>>
>>> Thanks for the various iterations,
>>>
>>
>> Phil, thank you for you continuous reviews and valuable comments.
>>
>> Yes, the final merge to master branch depends on the release plan.
>> 'queue' meant to merge the series to your 'cpus-next' branch ;-)
> 
> I had to fix 3 different issues caught by our CI. Next time please
> run your series on GitLab CI, you just have to push your branch and
> wait for the result :)
> 
> Now merged as 445946f4dd..cd75cc6337.
> 
> Happy new year!
> 

Happy new year. I just came back from holiday.

Thanks for the reminder for GitLab CI, which I don't know how to run yet.
I will learn how to run it from gitlab :)

Thanks,
Gavin



^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2024-01-08  1:25 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-04  0:47 [PATCH v9 0/9] Unified CPU type check Gavin Shan
2023-12-04  0:47 ` [PATCH v9 1/9] machine: Use error handling when CPU type is checked Gavin Shan
2024-01-05 11:00   ` Philippe Mathieu-Daudé
2024-01-08  1:11     ` Gavin Shan
2023-12-04  0:47 ` [PATCH v9 2/9] machine: Introduce helper is_cpu_type_supported() Gavin Shan
2023-12-04  0:47 ` [PATCH v9 3/9] machine: Improve is_cpu_type_supported() Gavin Shan
2024-01-05 22:09   ` Philippe Mathieu-Daudé
2024-01-08  1:21     ` Gavin Shan
2023-12-04  0:47 ` [PATCH v9 4/9] machine: Print CPU model name instead of CPU type Gavin Shan
2023-12-04  0:47 ` [PATCH v9 5/9] hw/arm/virt: Hide host CPU model for tcg Gavin Shan
2023-12-04  0:47 ` [PATCH v9 6/9] hw/arm/virt: Check CPU type in machine_run_board_init() Gavin Shan
2023-12-04  0:47 ` [PATCH v9 7/9] hw/arm/sbsa-ref: " Gavin Shan
2023-12-04  0:47 ` [PATCH v9 8/9] hw/arm: " Gavin Shan
2023-12-04  0:47 ` [PATCH v9 9/9] hw/riscv/shakti_c: " Gavin Shan
2023-12-12  4:55 ` [PATCH v9 0/9] Unified CPU type check Gavin Shan
2023-12-13 10:08   ` Philippe Mathieu-Daudé
2023-12-13 10:54     ` Gavin Shan
2024-01-05 22:12       ` Philippe Mathieu-Daudé
2024-01-08  1:24         ` Gavin Shan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).