* [PATCH v2 0/8] machine: Unified CPU type check
@ 2023-07-26 0:31 Gavin Shan
2023-07-26 0:31 ` [PATCH v2 1/8] machine: Use error handling when CPU type is checked Gavin Shan
` (7 more replies)
0 siblings, 8 replies; 21+ messages in thread
From: Gavin Shan @ 2023-07-26 0:31 UTC (permalink / raw)
To: qemu-arm
Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
strahinja.p.jankovic, sundeep.lkml, kfting, wuhaotsh,
nieklinnenbank, rad, quic_llindhol, marcin.juszkiewicz, eduardo,
marcel.apfelbaum, philmd, wangyanan55, laurent, vijai, palmer,
alistair.francis, bin.meng, liweiwei, dbarboza, zhiwei_liu,
imammedo, cohuck, richard.henderson, pbonzini, shan.gavin
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().
PATCH[1-3] Improves the check in machine_run_board_init()
PATCH[4-8] Move the check mc->init() to machine_run_board_init()
v1: https://lists.nongnu.org/archive/html/qemu-arm/2023-07/msg00302.html
Testing
=======
With the following command lines, the output messages are varied before
and after the series is applied.
/home/gshan/sandbox/src/qemu/main/build/qemu-system-aarch64 \
-accel tcg -machine virt,gic-version=3,nvdimm=on \
-cpu cortex-a8 -smp maxcpus=2,cpus=1 \
:
Before the series is applied:
qemu-system-aarch64: mach-virt: CPU type cortex-a8-arm-cpu not supported
After the series is applied:
qemu-system-aarch64: Invalid CPU type: cortex-a8-arm-cpu
The valid models are: cortex-a7, cortex-a15, cortex-a35, cortex-a55,
cortex-a72, cortex-a76, a64fx, neoverse-n1,
neoverse-v1, cortex-a53, cortex-a57, max
Changelog
=========
v2:
* Constify mc->valid_cpu_types (Richard)
* Print the supported CPU models, instead of typenames (Peter)
* Misc improvements for the hleper to do the check (Igor)
* More patches to move the check (Marcin)
Gavin Shan (8):
machine: Use error handling when CPU type is checked
machine: Introduce helper is_cpu_type_supported()
machine: Print supported CPU models instead of typenames
hw/arm/virt: Check CPU type in machine_run_board_init()
hw/arm/virt: Unsupported host CPU model on TCG
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 | 18 ++++++---
hw/arm/cubieboard.c | 18 ++++++---
hw/arm/mps2-tz.c | 34 ++++++++++++++---
hw/arm/mps2.c | 44 ++++++++++++++++++---
hw/arm/msf2-som.c | 18 ++++++---
hw/arm/musca.c | 19 ++++++----
hw/arm/npcm7xx_boards.c | 19 ++++++----
hw/arm/orangepi.c | 18 ++++++---
hw/arm/sbsa-ref.c | 29 ++++++--------
hw/arm/virt.c | 43 ++++++++++++---------
hw/core/machine.c | 84 +++++++++++++++++++++++------------------
hw/m68k/q800.c | 8 +++-
hw/riscv/shakti_c.c | 17 ++++++---
include/hw/boards.h | 3 +-
14 files changed, 243 insertions(+), 129 deletions(-)
--
2.41.0
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 1/8] machine: Use error handling when CPU type is checked
2023-07-26 0:31 [PATCH v2 0/8] machine: Unified CPU type check Gavin Shan
@ 2023-07-26 0:31 ` Gavin Shan
2023-07-26 0:31 ` [PATCH v2 2/8] machine: Introduce helper is_cpu_type_supported() Gavin Shan
` (6 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Gavin Shan @ 2023-07-26 0:31 UTC (permalink / raw)
To: qemu-arm
Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
strahinja.p.jankovic, sundeep.lkml, kfting, wuhaotsh,
nieklinnenbank, rad, quic_llindhol, marcin.juszkiewicz, eduardo,
marcel.apfelbaum, philmd, wangyanan55, laurent, vijai, palmer,
alistair.francis, bin.meng, liweiwei, dbarboza, zhiwei_liu,
imammedo, cohuck, richard.henderson, pbonzini, shan.gavin
QEMU will be terminated if the specified CPU type isn't supported
in machine_run_board_init(). The list of supported CPU type is
maintained in mc->valid_cpu_types. The error handling can be used
to propagate error messages, to be consistent how the errors are
handled for other situations in the same function.
No functional change intended.
Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
hw/core/machine.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index f0d35c6401..d7e7f8f120 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1355,6 +1355,7 @@ void machine_run_board_init(MachineState *machine, const char *mem_path, Error *
MachineClass *machine_class = MACHINE_GET_CLASS(machine);
ObjectClass *oc = object_class_by_name(machine->cpu_type);
CPUClass *cc;
+ Error *local_err = NULL;
/* This checkpoint is required by replay to separate prior clock
reading from the other reads, because timer polling functions query
@@ -1423,15 +1424,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(&local_err, "Invalid CPU type: %s", machine->cpu_type);
+ error_append_hint(&local_err, "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(&local_err, ", %s",
+ machine_class->valid_cpu_types[i]);
}
- error_printf("\n");
+ error_append_hint(&local_err, "\n");
- exit(1);
+ error_propagate(errp, local_err);
}
}
--
2.41.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 2/8] machine: Introduce helper is_cpu_type_supported()
2023-07-26 0:31 [PATCH v2 0/8] machine: Unified CPU type check Gavin Shan
2023-07-26 0:31 ` [PATCH v2 1/8] machine: Use error handling when CPU type is checked Gavin Shan
@ 2023-07-26 0:31 ` Gavin Shan
2023-07-26 23:03 ` Richard Henderson
2023-07-26 0:32 ` [PATCH v2 3/8] machine: Print supported CPU models instead of typenames Gavin Shan
` (5 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Gavin Shan @ 2023-07-26 0:31 UTC (permalink / raw)
To: qemu-arm
Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
strahinja.p.jankovic, sundeep.lkml, kfting, wuhaotsh,
nieklinnenbank, rad, quic_llindhol, marcin.juszkiewicz, eduardo,
marcel.apfelbaum, philmd, wangyanan55, laurent, vijai, palmer,
alistair.francis, bin.meng, liweiwei, dbarboza, zhiwei_liu,
imammedo, cohuck, richard.henderson, pbonzini, shan.gavin
The logic of checking if the specified CPU type is supported in
machine_run_board_init() is independent enough. Factor it out into
helper is_cpu_type_supported(). With this, machine_run_board_init()
looks a bit clean. 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>
---
hw/core/machine.c | 82 +++++++++++++++++++++++++----------------------
1 file changed, 44 insertions(+), 38 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index d7e7f8f120..fe110e9b0a 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1349,12 +1349,50 @@ out:
return r;
}
+static void is_cpu_type_supported(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;
+ }
+ }
+
+ /* 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);
+ }
+}
void machine_run_board_init(MachineState *machine, const char *mem_path, Error **errp)
{
MachineClass *machine_class = MACHINE_GET_CLASS(machine);
- ObjectClass *oc = object_class_by_name(machine->cpu_type);
- CPUClass *cc;
Error *local_err = NULL;
/* This checkpoint is required by replay to separate prior clock
@@ -1406,42 +1444,10 @@ 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 specificed 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(&local_err, "Invalid CPU type: %s", machine->cpu_type);
- error_append_hint(&local_err, "The valid types are: %s",
- machine_class->valid_cpu_types[0]);
- for (i = 1; machine_class->valid_cpu_types[i]; i++) {
- error_append_hint(&local_err, ", %s",
- machine_class->valid_cpu_types[i]);
- }
- error_append_hint(&local_err, "\n");
-
- error_propagate(errp, local_err);
- }
- }
-
- /* 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 */
+ is_cpu_type_supported(machine, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
}
if (machine->cgs) {
--
2.41.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 3/8] machine: Print supported CPU models instead of typenames
2023-07-26 0:31 [PATCH v2 0/8] machine: Unified CPU type check Gavin Shan
2023-07-26 0:31 ` [PATCH v2 1/8] machine: Use error handling when CPU type is checked Gavin Shan
2023-07-26 0:31 ` [PATCH v2 2/8] machine: Introduce helper is_cpu_type_supported() Gavin Shan
@ 2023-07-26 0:32 ` Gavin Shan
2023-07-26 23:08 ` Richard Henderson
2023-07-26 0:32 ` [PATCH v2 4/8] hw/arm/virt: Check CPU type in machine_run_board_init() Gavin Shan
` (4 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Gavin Shan @ 2023-07-26 0:32 UTC (permalink / raw)
To: qemu-arm
Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
strahinja.p.jankovic, sundeep.lkml, kfting, wuhaotsh,
nieklinnenbank, rad, quic_llindhol, marcin.juszkiewicz, eduardo,
marcel.apfelbaum, philmd, wangyanan55, laurent, vijai, palmer,
alistair.francis, bin.meng, liweiwei, dbarboza, zhiwei_liu,
imammedo, cohuck, richard.henderson, pbonzini, shan.gavin
The supported CPU models instead of typenames should be printed when
the user specified CPU type isn't supported in is_cpu_type_supported(),
to be consistent with the CPU model specified by user through '-cpu
<model>' option.
Correct the error messages to print CPU models, maintained in the newly
added mc->valid_cpu_models because there is no fixed pattern for the
conversion between CPU model and typename. Besides, mc->valid_cpu_types
and mc->valid_cpu_models are further constified since we're here.
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
hw/core/machine.c | 10 ++++++----
hw/m68k/q800.c | 8 +++++++-
include/hw/boards.h | 3 ++-
3 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index fe110e9b0a..858f8ede89 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1362,6 +1362,8 @@ static void is_cpu_type_supported(MachineState *machine, Error **errp)
* type is provided through '-cpu' option.
*/
if (mc->valid_cpu_types && machine->cpu_type) {
+ assert(mc->valid_cpu_models && mc->valid_cpu_models[0]);
+
for (i = 0; mc->valid_cpu_types[i]; i++) {
if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
break;
@@ -1371,10 +1373,10 @@ static void is_cpu_type_supported(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]);
+ error_append_hint(errp, "The valid models are: %s",
+ mc->valid_cpu_models[0]);
+ for (i = 1; mc->valid_cpu_models[i]; i++) {
+ error_append_hint(errp, ", %s", mc->valid_cpu_models[i]);
}
error_append_hint(errp, "\n");
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index b770b71d54..1e360674a7 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -596,11 +596,16 @@ static GlobalProperty hw_compat_q800[] = {
};
static const size_t hw_compat_q800_len = G_N_ELEMENTS(hw_compat_q800);
-static const char *q800_machine_valid_cpu_types[] = {
+static const char * const q800_machine_valid_cpu_types[] = {
M68K_CPU_TYPE_NAME("m68040"),
NULL
};
+static const char * const q800_machine_valid_cpu_models[] = {
+ "m68040",
+ NULL
+};
+
static void q800_machine_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
@@ -609,6 +614,7 @@ static void q800_machine_class_init(ObjectClass *oc, void *data)
mc->init = q800_machine_init;
mc->default_cpu_type = M68K_CPU_TYPE_NAME("m68040");
mc->valid_cpu_types = q800_machine_valid_cpu_types;
+ mc->valid_cpu_models = q800_machine_valid_cpu_models;
mc->max_cpus = 1;
mc->block_default_type = IF_SCSI;
mc->default_ram_id = "m68k_mac.ram";
diff --git a/include/hw/boards.h b/include/hw/boards.h
index ed83360198..81747b0788 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -268,7 +268,8 @@ struct MachineClass {
bool has_hotpluggable_cpus;
bool ignore_memory_transaction_failures;
int numa_mem_align_shift;
- const char **valid_cpu_types;
+ const char * const *valid_cpu_types;
+ const char * const *valid_cpu_models;
strList *allowed_dynamic_sysbus_devices;
bool auto_enable_numa_with_memhp;
bool auto_enable_numa_with_memdev;
--
2.41.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 4/8] hw/arm/virt: Check CPU type in machine_run_board_init()
2023-07-26 0:31 [PATCH v2 0/8] machine: Unified CPU type check Gavin Shan
` (2 preceding siblings ...)
2023-07-26 0:32 ` [PATCH v2 3/8] machine: Print supported CPU models instead of typenames Gavin Shan
@ 2023-07-26 0:32 ` Gavin Shan
2023-07-26 0:32 ` [PATCH v2 5/8] hw/arm/virt: Unsupported host CPU model on TCG Gavin Shan
` (3 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Gavin Shan @ 2023-07-26 0:32 UTC (permalink / raw)
To: qemu-arm
Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
strahinja.p.jankovic, sundeep.lkml, kfting, wuhaotsh,
nieklinnenbank, rad, quic_llindhol, marcin.juszkiewicz, eduardo,
marcel.apfelbaum, philmd, wangyanan55, laurent, vijai, palmer,
alistair.francis, bin.meng, liweiwei, dbarboza, zhiwei_liu,
imammedo, cohuck, richard.henderson, pbonzini, shan.gavin
Set mc->valid_cpu_{types, models} so that the specified CPU type
can be checked in machine_run_board_init(). We needn't to do the
check by ourselves.
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
hw/arm/virt.c | 39 ++++++++++++++++++++++-----------------
1 file changed, 22 insertions(+), 17 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7d9dbc2663..debd85614e 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -203,7 +203,7 @@ static const int a15irqmap[] = {
[VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */
};
-static const char *valid_cpus[] = {
+static const char * const valid_cpu_types[] = {
#ifdef CONFIG_TCG
ARM_CPU_TYPE_NAME("cortex-a7"),
ARM_CPU_TYPE_NAME("cortex-a15"),
@@ -219,19 +219,27 @@ static const char *valid_cpus[] = {
ARM_CPU_TYPE_NAME("cortex-a57"),
ARM_CPU_TYPE_NAME("host"),
ARM_CPU_TYPE_NAME("max"),
+ NULL
};
-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 const char * const valid_cpu_models[] = {
+#ifdef CONFIG_TCG
+ "cortex-a7",
+ "cortex-a15",
+ "cortex-a35",
+ "cortex-a55",
+ "cortex-a72",
+ "cortex-a76",
+ "a64fx",
+ "neoverse-n1",
+ "neoverse-v1",
+#endif
+ "cortex-a53",
+ "cortex-a57",
+ "host",
+ "max",
+ NULL
+};
static void create_randomness(MachineState *ms, const char *node)
{
@@ -2030,11 +2038,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);
/*
@@ -2953,6 +2956,8 @@ 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->valid_cpu_models = valid_cpu_models;
mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
mc->kvm_type = virt_kvm_type;
assert(!mc->get_hotplug_handler);
--
2.41.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 5/8] hw/arm/virt: Unsupported host CPU model on TCG
2023-07-26 0:31 [PATCH v2 0/8] machine: Unified CPU type check Gavin Shan
` (3 preceding siblings ...)
2023-07-26 0:32 ` [PATCH v2 4/8] hw/arm/virt: Check CPU type in machine_run_board_init() Gavin Shan
@ 2023-07-26 0:32 ` Gavin Shan
2023-07-26 0:32 ` [PATCH v2 6/8] hw/arm/sbsa-ref: Check CPU type in machine_run_board_init() Gavin Shan
` (2 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Gavin Shan @ 2023-07-26 0:32 UTC (permalink / raw)
To: qemu-arm
Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
strahinja.p.jankovic, sundeep.lkml, kfting, wuhaotsh,
nieklinnenbank, rad, quic_llindhol, marcin.juszkiewicz, eduardo,
marcel.apfelbaum, philmd, wangyanan55, laurent, vijai, palmer,
alistair.francis, bin.meng, liweiwei, dbarboza, zhiwei_liu,
imammedo, cohuck, richard.henderson, pbonzini, shan.gavin
The 'host' CPU model isn't supported until KVM or HVF is enabled.
For example, the following error messages are seen when the guest
is started with option '-cpu cortex-a8'.
qemu-system-aarch64: Invalid CPU type: cortex-a8-arm-cpu
The valid models are: cortex-a7, cortex-a15, cortex-a35,
cortex-a55, cortex-a72, cortex-a76, a64fx, neoverse-n1,
neoverse-v1, cortex-a53, cortex-a57, host, max
Hide 'host' CPU model until KVM or HVF is enabled.
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
hw/arm/virt.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index debd85614e..2562ca0c1e 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -217,7 +217,9 @@ static const char * const valid_cpu_types[] = {
#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
};
@@ -236,7 +238,9 @@ static const char * const valid_cpu_models[] = {
#endif
"cortex-a53",
"cortex-a57",
+#if defined(CONFIG_KVM) || defined(CONFIG_HVF)
"host",
+#endif
"max",
NULL
};
--
2.41.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 6/8] hw/arm/sbsa-ref: Check CPU type in machine_run_board_init()
2023-07-26 0:31 [PATCH v2 0/8] machine: Unified CPU type check Gavin Shan
` (4 preceding siblings ...)
2023-07-26 0:32 ` [PATCH v2 5/8] hw/arm/virt: Unsupported host CPU model on TCG Gavin Shan
@ 2023-07-26 0:32 ` Gavin Shan
2023-07-26 0:32 ` [PATCH v2 7/8] hw/arm: " Gavin Shan
2023-07-26 0:32 ` [PATCH v2 8/8] hw/riscv/shakti_c: " Gavin Shan
7 siblings, 0 replies; 21+ messages in thread
From: Gavin Shan @ 2023-07-26 0:32 UTC (permalink / raw)
To: qemu-arm
Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
strahinja.p.jankovic, sundeep.lkml, kfting, wuhaotsh,
nieklinnenbank, rad, quic_llindhol, marcin.juszkiewicz, eduardo,
marcel.apfelbaum, philmd, wangyanan55, laurent, vijai, palmer,
alistair.francis, bin.meng, liweiwei, dbarboza, zhiwei_liu,
imammedo, cohuck, richard.henderson, pbonzini, shan.gavin
Set mc->valid_cpu_{types, models} so that the specified CPU type
can be checked in machine_run_board_init(). We needn't to do the
check by ourselves.
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
hw/arm/sbsa-ref.c | 29 ++++++++++++-----------------
1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index bc89eb4806..66d171b745 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -149,25 +149,23 @@ static const int sbsa_ref_irqmap[] = {
[SBSA_GWDT_WS0] = 16,
};
-static const char * const valid_cpus[] = {
+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("max"),
+ NULL,
};
-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 const char * const valid_cpu_models[] = {
+ "cortex-a57",
+ "cortex-a72",
+ "neoverse-n1",
+ "neoverse-v1",
+ "max",
+ NULL,
+};
static uint64_t sbsa_ref_cpu_mp_affinity(SBSAMachineState *sms, int idx)
{
@@ -730,11 +728,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);
@@ -899,6 +892,8 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data)
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->valid_cpu_models = valid_cpu_models;
mc->max_cpus = 512;
mc->pci_allow_0_address = true;
mc->minimum_page_bits = 12;
--
2.41.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 7/8] hw/arm: Check CPU type in machine_run_board_init()
2023-07-26 0:31 [PATCH v2 0/8] machine: Unified CPU type check Gavin Shan
` (5 preceding siblings ...)
2023-07-26 0:32 ` [PATCH v2 6/8] hw/arm/sbsa-ref: Check CPU type in machine_run_board_init() Gavin Shan
@ 2023-07-26 0:32 ` Gavin Shan
2023-07-26 0:32 ` [PATCH v2 8/8] hw/riscv/shakti_c: " Gavin Shan
7 siblings, 0 replies; 21+ messages in thread
From: Gavin Shan @ 2023-07-26 0:32 UTC (permalink / raw)
To: qemu-arm
Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
strahinja.p.jankovic, sundeep.lkml, kfting, wuhaotsh,
nieklinnenbank, rad, quic_llindhol, marcin.juszkiewicz, eduardo,
marcel.apfelbaum, philmd, wangyanan55, laurent, vijai, palmer,
alistair.francis, bin.meng, liweiwei, dbarboza, zhiwei_liu,
imammedo, cohuck, richard.henderson, pbonzini, shan.gavin
Set mc->valid_cpu_{types, models} so that the specified CPU type
can be checked in machine_run_board_init(). We needn't to do the
check by ourselves.
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
hw/arm/bananapi_m2u.c | 18 +++++++++++------
hw/arm/cubieboard.c | 18 +++++++++++------
hw/arm/mps2-tz.c | 34 +++++++++++++++++++++++++------
hw/arm/mps2.c | 44 +++++++++++++++++++++++++++++++++++------
hw/arm/msf2-som.c | 18 +++++++++++------
hw/arm/musca.c | 19 +++++++++++-------
hw/arm/npcm7xx_boards.c | 19 +++++++++++-------
hw/arm/orangepi.c | 18 +++++++++++------
8 files changed, 138 insertions(+), 50 deletions(-)
diff --git a/hw/arm/bananapi_m2u.c b/hw/arm/bananapi_m2u.c
index 74121d8966..d6c9b90370 100644
--- a/hw/arm/bananapi_m2u.c
+++ b/hw/arm/bananapi_m2u.c
@@ -29,6 +29,16 @@
static struct arm_boot_info bpim2u_binfo;
+static const char * const valid_cpu_types[] = {
+ ARM_CPU_TYPE_NAME("cortex-a7"),
+ NULL
+};
+
+static const char * const valid_cpu_models[] = {
+ "cortex-a7",
+ NULL
+};
+
/*
* R40 can boot from mmc0 and mmc2, and bpim2u has two mmc interface, one is
* connected to sdcard and another mount an emmc media.
@@ -70,12 +80,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));
@@ -138,6 +142,8 @@ static void bpim2u_machine_init(MachineClass *mc)
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->valid_cpu_models = valid_cpu_models;
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 8c7fa91529..4a66a781a4 100644
--- a/hw/arm/cubieboard.c
+++ b/hw/arm/cubieboard.c
@@ -28,6 +28,16 @@ static struct arm_boot_info cubieboard_binfo = {
.board_id = 0x1008,
};
+static const char * const valid_cpu_types[] = {
+ ARM_CPU_TYPE_NAME("cortex-a8"),
+ NULL
+};
+
+static const char * const valid_cpu_models[] = {
+ "cortex-a8",
+ NULL
+};
+
static void cubieboard_init(MachineState *machine)
{
AwA10State *a10;
@@ -51,12 +61,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));
@@ -115,6 +119,8 @@ static void cubieboard_machine_init(MachineClass *mc)
{
mc->desc = "cubietech cubieboard (Cortex-A8)";
mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a8");
+ mc->valid_cpu_types = valid_cpu_types;
+ mc->valid_cpu_models = valid_cpu_models;
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 5873107302..7eba212659 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -183,6 +183,26 @@ OBJECT_DECLARE_TYPE(MPS2TZMachineState, MPS2TZMachineClass, MPS2TZ_MACHINE)
#define MPS3_DDR_SIZE (2 * GiB)
#endif
+static const char * const valid_cpu_types[] = {
+ ARM_CPU_TYPE_NAME("cortex-m33"),
+ NULL
+};
+
+static const char * const mps3tz_an547_valid_cpu_types[] = {
+ ARM_CPU_TYPE_NAME("cortex-m55"),
+ NULL
+};
+
+static const char * const valid_cpu_models[] = {
+ "cortex-m33",
+ NULL
+};
+
+static const char * const mps3tz_an547_valid_cpu_models[] = {
+ "cortex-m55",
+ NULL
+};
+
static const uint32_t an505_oscclk[] = {
40000000,
24580000,
@@ -802,12 +822,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);
@@ -1293,6 +1307,8 @@ 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;
+ mc->valid_cpu_models = valid_cpu_models;
mmc->scc_id = 0x41045050;
mmc->sysclk_frq = 20 * 1000 * 1000; /* 20MHz */
mmc->apb_periph_frq = mmc->sysclk_frq;
@@ -1322,6 +1338,8 @@ 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;
+ mc->valid_cpu_models = valid_cpu_models;
mmc->scc_id = 0x41045210;
mmc->sysclk_frq = 20 * 1000 * 1000; /* 20MHz */
mmc->apb_periph_frq = mmc->sysclk_frq;
@@ -1351,6 +1369,8 @@ 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;
+ mc->valid_cpu_models = valid_cpu_models;
mmc->scc_id = 0x41045240;
mmc->sysclk_frq = 32 * 1000 * 1000; /* 32MHz */
mmc->apb_periph_frq = mmc->sysclk_frq;
@@ -1385,6 +1405,8 @@ 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 = mps3tz_an547_valid_cpu_types;
+ mc->valid_cpu_models = mps3tz_an547_valid_cpu_models;
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 d92fd60684..b833029088 100644
--- a/hw/arm/mps2.c
+++ b/hw/arm/mps2.c
@@ -110,6 +110,36 @@ OBJECT_DECLARE_TYPE(MPS2MachineState, MPS2MachineClass, MPS2_MACHINE)
*/
#define REFCLK_FRQ (1 * 1000 * 1000)
+static const char * const valid_cpu_types[] = {
+ ARM_CPU_TYPE_NAME("cortex-m3"),
+ NULL
+};
+
+static const char * const mps2_an386_valid_cpu_types[] = {
+ ARM_CPU_TYPE_NAME("cortex-m4"),
+ NULL
+};
+
+static const char * const mps2_an500_valid_cpu_types[] = {
+ ARM_CPU_TYPE_NAME("cortex-m7"),
+ NULL
+};
+
+static const char * const valid_cpu_models[] = {
+ "cortex-m3",
+ NULL
+};
+
+static const char * const mps2_an386_valid_cpu_models[] = {
+ "cortex-m4",
+ NULL
+};
+
+static const char * const mps2_an500_valid_cpu_models[] = {
+ "cortex-m7",
+ NULL
+};
+
/* Initialize the auxiliary RAM region @mr and map it into
* the memory map at @base.
*/
@@ -140,12 +170,6 @@ static void mps2_common_init(MachineState *machine)
DeviceState *armv7m, *sccdev;
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,6 +508,8 @@ static void mps2_an385_class_init(ObjectClass *oc, void *data)
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;
+ mc->valid_cpu_models = valid_cpu_models;
mmc->scc_id = 0x41043850;
mmc->psram_base = 0x21000000;
mmc->ethernet_base = 0x40200000;
@@ -498,6 +524,8 @@ static void mps2_an386_class_init(ObjectClass *oc, void *data)
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 = mps2_an386_valid_cpu_types;
+ mc->valid_cpu_models = mps2_an386_valid_cpu_models;
mmc->scc_id = 0x41043860;
mmc->psram_base = 0x21000000;
mmc->ethernet_base = 0x40200000;
@@ -512,6 +540,8 @@ static void mps2_an500_class_init(ObjectClass *oc, void *data)
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 = mps2_an500_valid_cpu_types;
+ mc->valid_cpu_models = mps2_an500_valid_cpu_models;
mmc->scc_id = 0x41045000;
mmc->psram_base = 0x60000000;
mmc->ethernet_base = 0xa0000000;
@@ -526,6 +556,8 @@ static void mps2_an511_class_init(ObjectClass *oc, void *data)
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;
+ mc->valid_cpu_models = valid_cpu_models;
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..725309514b 100644
--- a/hw/arm/msf2-som.c
+++ b/hw/arm/msf2-som.c
@@ -42,6 +42,16 @@
#define M2S010_ENVM_SIZE (256 * KiB)
#define M2S010_ESRAM_SIZE (64 * KiB)
+static const char * const valid_cpu_types[] = {
+ ARM_CPU_TYPE_NAME("cortex-m3"),
+ NULL
+};
+
+static const char * const valid_cpu_models[] = {
+ "cortex-m3",
+ NULL
+};
+
static void emcraft_sf2_s2s010_init(MachineState *machine)
{
DeviceState *dev;
@@ -55,12 +65,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);
@@ -109,6 +113,8 @@ static void emcraft_sf2_machine_init(MachineClass *mc)
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;
+ mc->valid_cpu_models = valid_cpu_models;
}
DEFINE_MACHINE("emcraft-sf2", emcraft_sf2_machine_init)
diff --git a/hw/arm/musca.c b/hw/arm/musca.c
index 6eeee57c9d..69d6735f27 100644
--- a/hw/arm/musca.c
+++ b/hw/arm/musca.c
@@ -102,6 +102,16 @@ OBJECT_DECLARE_TYPE(MuscaMachineState, MuscaMachineClass, MUSCA_MACHINE)
/* Slow 32Khz S32KCLK frequency in Hz */
#define S32KCLK_FRQ (32 * 1000)
+static const char * const valid_cpu_types[] = {
+ ARM_CPU_TYPE_NAME("cortex-m33"),
+ NULL
+};
+
+static const char * const valid_cpu_models[] = {
+ "cortex-m33",
+ NULL
+};
+
static qemu_irq get_sse_irq_in(MuscaMachineState *mms, int irqno)
{
/* Return a qemu_irq which will signal IRQ n to all CPUs in the SSE. */
@@ -355,7 +365,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 +375,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");
@@ -609,6 +612,8 @@ static void musca_class_init(ObjectClass *oc, void *data)
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->valid_cpu_models = valid_cpu_models;
mc->init = musca_init;
}
diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
index 2aef579aac..23b5ab0ccc 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -55,6 +55,16 @@
static const char npcm7xx_default_bootrom[] = "npcm7xx_bootrom.bin";
+static const char * const valid_cpu_types[] = {
+ ARM_CPU_TYPE_NAME("cortex-a9"),
+ NULL
+};
+
+static const char * const valid_cpu_models[] = {
+ "cortex-a9",
+ NULL
+};
+
static void npcm7xx_load_bootrom(MachineState *machine, NPCM7xxState *soc)
{
const char *bios_name = machine->firmware ?: npcm7xx_default_bootrom;
@@ -121,15 +131,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);
@@ -469,6 +472,8 @@ static void npcm7xx_machine_class_init(ObjectClass *oc, void *data)
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;
+ mc->valid_cpu_models = valid_cpu_models;
}
/*
diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
index 10653361ed..d13987deed 100644
--- a/hw/arm/orangepi.c
+++ b/hw/arm/orangepi.c
@@ -28,6 +28,16 @@
static struct arm_boot_info orangepi_binfo;
+static const char * const valid_cpu_types[] = {
+ ARM_CPU_TYPE_NAME("cortex-a7"),
+ NULL
+};
+
+static const char * const valid_cpu_models[] = {
+ "cortex-a7",
+ NULL
+};
+
static void orangepi_init(MachineState *machine)
{
AwH3State *h3;
@@ -48,12 +58,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));
@@ -118,6 +122,8 @@ 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->valid_cpu_models = valid_cpu_models;
mc->default_ram_size = 1 * GiB;
mc->default_ram_id = "orangepi.ram";
}
--
2.41.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 8/8] hw/riscv/shakti_c: Check CPU type in machine_run_board_init()
2023-07-26 0:31 [PATCH v2 0/8] machine: Unified CPU type check Gavin Shan
` (6 preceding siblings ...)
2023-07-26 0:32 ` [PATCH v2 7/8] hw/arm: " Gavin Shan
@ 2023-07-26 0:32 ` Gavin Shan
7 siblings, 0 replies; 21+ messages in thread
From: Gavin Shan @ 2023-07-26 0:32 UTC (permalink / raw)
To: qemu-arm
Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
strahinja.p.jankovic, sundeep.lkml, kfting, wuhaotsh,
nieklinnenbank, rad, quic_llindhol, marcin.juszkiewicz, eduardo,
marcel.apfelbaum, philmd, wangyanan55, laurent, vijai, palmer,
alistair.francis, bin.meng, liweiwei, dbarboza, zhiwei_liu,
imammedo, cohuck, richard.henderson, pbonzini, shan.gavin
Set mc->valid_cpu_{types, models} so that the specified CPU type
can be checked in machine_run_board_init(). We needn't to do the
check by ourselves.
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
hw/riscv/shakti_c.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/hw/riscv/shakti_c.c b/hw/riscv/shakti_c.c
index 12ea74b032..0bd59d47cd 100644
--- a/hw/riscv/shakti_c.c
+++ b/hw/riscv/shakti_c.c
@@ -28,6 +28,15 @@
#include "exec/address-spaces.h"
#include "hw/riscv/boot.h"
+static const char * const valid_cpu_types[] = {
+ RISCV_CPU_TYPE_NAME("shakti-c"),
+ NULL
+};
+
+static const char * const valid_cpu_models[] = {
+ "shakti-c",
+ NULL
+};
static const struct MemmapEntry {
hwaddr base;
@@ -47,12 +56,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);
@@ -85,6 +88,8 @@ static void shakti_c_machine_class_init(ObjectClass *klass, void *data)
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->valid_cpu_models = valid_cpu_models;
mc->default_ram_id = "riscv.shakti.c.ram";
}
--
2.41.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/8] machine: Introduce helper is_cpu_type_supported()
2023-07-26 0:31 ` [PATCH v2 2/8] machine: Introduce helper is_cpu_type_supported() Gavin Shan
@ 2023-07-26 23:03 ` Richard Henderson
0 siblings, 0 replies; 21+ messages in thread
From: Richard Henderson @ 2023-07-26 23:03 UTC (permalink / raw)
To: Gavin Shan; +Cc: qemu-devel
On 7/25/23 17:31, Gavin Shan wrote:
> The logic of checking if the specified CPU type is supported in
> machine_run_board_init() is independent enough. Factor it out into
> helper is_cpu_type_supported(). With this, machine_run_board_init()
> looks a bit clean. 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>
> ---
> hw/core/machine.c | 82 +++++++++++++++++++++++++----------------------
> 1 file changed, 44 insertions(+), 38 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index d7e7f8f120..fe110e9b0a 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1349,12 +1349,50 @@ out:
> return r;
> }
>
> +static void is_cpu_type_supported(MachineState *machine, Error **errp)
Return type bool, false on failure.
> + /* Check if the CPU type is supported */
> + is_cpu_type_supported(machine, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
Fold call into if, and no need for local_error.
r~
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/8] machine: Print supported CPU models instead of typenames
2023-07-26 0:32 ` [PATCH v2 3/8] machine: Print supported CPU models instead of typenames Gavin Shan
@ 2023-07-26 23:08 ` Richard Henderson
2023-07-27 5:16 ` Gavin Shan
0 siblings, 1 reply; 21+ messages in thread
From: Richard Henderson @ 2023-07-26 23:08 UTC (permalink / raw)
To: Gavin Shan, qemu-arm
Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
strahinja.p.jankovic, sundeep.lkml, kfting, wuhaotsh,
nieklinnenbank, rad, quic_llindhol, marcin.juszkiewicz, eduardo,
marcel.apfelbaum, philmd, wangyanan55, laurent, vijai, palmer,
alistair.francis, bin.meng, liweiwei, dbarboza, zhiwei_liu,
imammedo, cohuck, pbonzini, shan.gavin
On 7/25/23 17:32, Gavin Shan wrote:
> -static const char *q800_machine_valid_cpu_types[] = {
> +static const char * const q800_machine_valid_cpu_types[] = {
> M68K_CPU_TYPE_NAME("m68040"),
> NULL
> };
>
> +static const char * const q800_machine_valid_cpu_models[] = {
> + "m68040",
> + NULL
> +};
I really don't like this replication.
r~
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/8] machine: Print supported CPU models instead of typenames
2023-07-26 23:08 ` Richard Henderson
@ 2023-07-27 5:16 ` Gavin Shan
2023-07-27 9:00 ` Igor Mammedov
2023-07-27 14:27 ` Richard Henderson
0 siblings, 2 replies; 21+ messages in thread
From: Gavin Shan @ 2023-07-27 5:16 UTC (permalink / raw)
To: Richard Henderson, qemu-arm
Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
strahinja.p.jankovic, sundeep.lkml, kfting, wuhaotsh,
nieklinnenbank, rad, quic_llindhol, marcin.juszkiewicz, eduardo,
marcel.apfelbaum, philmd, wangyanan55, laurent, vijai, palmer,
alistair.francis, bin.meng, liweiwei, dbarboza, zhiwei_liu,
imammedo, cohuck, pbonzini, shan.gavin
On 7/27/23 09:08, Richard Henderson wrote:
> On 7/25/23 17:32, Gavin Shan wrote:
>> -static const char *q800_machine_valid_cpu_types[] = {
>> +static const char * const q800_machine_valid_cpu_types[] = {
>> M68K_CPU_TYPE_NAME("m68040"),
>> NULL
>> };
>> +static const char * const q800_machine_valid_cpu_models[] = {
>> + "m68040",
>> + NULL
>> +};
>
> I really don't like this replication.
>
Right, it's going to be lots of replications, but gives much flexibility.
There are 21 targets and we don't have fixed pattern for the mapping between
CPU model name and CPU typename. I'm summarizing the used patterns like below.
1 All CPU model names are mappinged to fixed CPU typename;
2 CPU model name is same to CPU typename;
3 CPU model name is alias to CPU typename;
4 CPU model name is prefix of CPU typename;
Target Categories suffix-of-CPU-typename
-------------------------------------------------------
alpha -234 -alpha-cpu
arm ---4 -arm-cpu
avr -2--
cris --34 -cris-cpu
hexagon ---4 -hexagon-cpu
hppa 1---
i386 ---4 -i386-cpu
loongarch -2-4 -loongarch-cpu
m68k ---4 -m68k-cpu
microblaze 1---
mips ---4 -mips64-cpu -mips-cpu
nios2 1---
openrisc ---4 -or1k-cpu
ppc --34 -powerpc64-cpu -powerpc-cpu
riscv ---4 -riscv-cpu
rx -2-4 -rx-cpu
s390x ---4 -s390x-cpu
sh4 --34 -superh-cpu
sparc -2--
tricore ---4 -tricore-cpu
xtensa ---4 -xtensa-cpu
There are several options as below. Please let me know which one or something
else is the best.
(a) Keep what we have and use mc->valid_{cpu_types, cpu_models}[] to track
the valid CPU typenames and CPU model names.
(b) Introduce CPUClass::model_name_by_typename(). Every target has their own
implementation to convert CPU typename to CPU model name. The CPU model name
is parsed from mc->valid_cpu_types[i].
char *CPUClass::model_by_typename(const char *typename);
(c) As we discussed before, use mc->valid_cpu_type_suffix and mc->valid_cpu_models
because the CPU type check is currently needed by target arm/m68k/riscv where we
do have fixed pattern to convert CPU model names to CPU typenames. The CPU typename
is comprised of CPU model name and suffix. However, it won't be working when the CPU
type check is required by other target where we have patterns other than this.
Thanks,
Gavin
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/8] machine: Print supported CPU models instead of typenames
2023-07-27 5:16 ` Gavin Shan
@ 2023-07-27 9:00 ` Igor Mammedov
2023-07-31 5:07 ` Gavin Shan
2023-07-27 14:27 ` Richard Henderson
1 sibling, 1 reply; 21+ messages in thread
From: Igor Mammedov @ 2023-07-27 9:00 UTC (permalink / raw)
To: Gavin Shan
Cc: Richard Henderson, qemu-arm, qemu-devel, qemu-riscv,
peter.maydell, b.galvani, strahinja.p.jankovic, sundeep.lkml,
kfting, wuhaotsh, nieklinnenbank, rad, quic_llindhol,
marcin.juszkiewicz, eduardo, marcel.apfelbaum, philmd,
wangyanan55, laurent, vijai, palmer, alistair.francis, bin.meng,
liweiwei, dbarboza, zhiwei_liu, cohuck, pbonzini, shan.gavin,
qemu-ppc
On Thu, 27 Jul 2023 15:16:18 +1000
Gavin Shan <gshan@redhat.com> wrote:
> On 7/27/23 09:08, Richard Henderson wrote:
> > On 7/25/23 17:32, Gavin Shan wrote:
> >> -static const char *q800_machine_valid_cpu_types[] = {
> >> +static const char * const q800_machine_valid_cpu_types[] = {
> >> M68K_CPU_TYPE_NAME("m68040"),
> >> NULL
> >> };
> >> +static const char * const q800_machine_valid_cpu_models[] = {
> >> + "m68040",
> >> + NULL
> >> +};
> >
> > I really don't like this replication.
> >
>
> Right, it's going to be lots of replications, but gives much flexibility.
> There are 21 targets and we don't have fixed pattern for the mapping between
> CPU model name and CPU typename. I'm summarizing the used patterns like below.
>
> 1 All CPU model names are mappinged to fixed CPU typename;
plainly spelled it would be: cpu_model name ignored and
a cpu type is returned anyways.
I'd make this hard error right away, as "junk in => error out"
it's clearly user error. I think we don't even have to follow
deprecation process for that.
> 2 CPU model name is same to CPU typename;
> 3 CPU model name is alias to CPU typename;
> 4 CPU model name is prefix of CPU typename;
and some more:
5. cpu model names aren't names at all sometimes, and some other
CPU property is used. (ppc)
This one I'd prefer to get rid of and ppc handling more consistent
with other targets, which would need PPC folks to persuaded to drop
PVR lookup.
>
> Target Categories suffix-of-CPU-typename
> -------------------------------------------------------
> alpha -234 -alpha-cpu
> arm ---4 -arm-cpu
> avr -2--
> cris --34 -cris-cpu
> hexagon ---4 -hexagon-cpu
> hppa 1---
> i386 ---4 -i386-cpu
> loongarch -2-4 -loongarch-cpu
> m68k ---4 -m68k-cpu
> microblaze 1---
> mips ---4 -mips64-cpu -mips-cpu
> nios2 1---
> openrisc ---4 -or1k-cpu
> ppc --34 -powerpc64-cpu -powerpc-cpu
> riscv ---4 -riscv-cpu
> rx -2-4 -rx-cpu
> s390x ---4 -s390x-cpu
> sh4 --34 -superh-cpu
> sparc -2--
> tricore ---4 -tricore-cpu
> xtensa ---4 -xtensa-cpu
>
> There are several options as below. Please let me know which one or something
> else is the best.
>
> (a) Keep what we have and use mc->valid_{cpu_types, cpu_models}[] to track
> the valid CPU typenames and CPU model names.
>
> (b) Introduce CPUClass::model_name_by_typename(). Every target has their own
> implementation to convert CPU typename to CPU model name. The CPU model name
> is parsed from mc->valid_cpu_types[i].
>
> char *CPUClass::model_by_typename(const char *typename);
>
> (c) As we discussed before, use mc->valid_cpu_type_suffix and mc->valid_cpu_models
> because the CPU type check is currently needed by target arm/m68k/riscv where we
> do have fixed pattern to convert CPU model names to CPU typenames. The CPU typename
> is comprised of CPU model name and suffix. However, it won't be working when the CPU
> type check is required by other target where we have patterns other than this.
none of above is really good, that's why I was objecting to introducing
reverse type->name mapping. That ends up with increased amount junk,
and it's not because your patches are bad, but because you are trying
to deal with cpu model names (which is a historically evolved mess).
The best from engineering POV would be replacing CPU models with
type names.
Even though it's a bit radical, I very much prefer replacing
cpu_model names with '-cpu type'usage directly. Making it
consistent with -device/other interfaces and coincidentally that
obsoletes need in reverse name mapping.
It's painful for end users who will need to change configs/scripts,
but it's one time job. Additionally from QEMU pov, codebase
will be less messy => more maintainable which benefits not only
developers but end-users in the end.
[rant:
It's the same story repeating over and over, when it comes to
changing QEMU CLI, which hits resistance wall. But with QEMU
deprecation process we've changed CLI behavior before,
despite of that world didn't cease to exist and users
have adapted to new QEMU and arguably QEMU became a tiny
bit more maintainable since we don't have to deal some
legacy behavior.
]
Another idea back in the days was (as a compromise),
1. keep using keep valid_cpu_types
2. instead of introducing yet another way to do reverse mapping,
clean/generalize/make it work everywhere list_cpus (which
already does that mapping) and then use that to do your thing.
It will have drawbacks you've listed above, but hopefully
that will clean up and reuse existing list_cpus.
(only this time, I'd build it around query-cpu-model-expansion,
which output is used by generic list_cpus)
[and here I'm asking to rewrite directly unrelated QEMU part yet again]
> Thanks,
> Gavin
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/8] machine: Print supported CPU models instead of typenames
2023-07-27 5:16 ` Gavin Shan
2023-07-27 9:00 ` Igor Mammedov
@ 2023-07-27 14:27 ` Richard Henderson
2023-07-31 5:33 ` Gavin Shan
1 sibling, 1 reply; 21+ messages in thread
From: Richard Henderson @ 2023-07-27 14:27 UTC (permalink / raw)
To: Gavin Shan, qemu-arm
Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
strahinja.p.jankovic, sundeep.lkml, kfting, wuhaotsh,
nieklinnenbank, rad, quic_llindhol, marcin.juszkiewicz, eduardo,
marcel.apfelbaum, philmd, wangyanan55, laurent, vijai, palmer,
alistair.francis, bin.meng, liweiwei, dbarboza, zhiwei_liu,
imammedo, cohuck, pbonzini, shan.gavin
On 7/26/23 22:16, Gavin Shan wrote:
>
> On 7/27/23 09:08, Richard Henderson wrote:
>> On 7/25/23 17:32, Gavin Shan wrote:
>>> -static const char *q800_machine_valid_cpu_types[] = {
>>> +static const char * const q800_machine_valid_cpu_types[] = {
>>> M68K_CPU_TYPE_NAME("m68040"),
>>> NULL
>>> };
>>> +static const char * const q800_machine_valid_cpu_models[] = {
>>> + "m68040",
>>> + NULL
>>> +};
>>
>> I really don't like this replication.
>>
>
> Right, it's going to be lots of replications, but gives much flexibility.
> There are 21 targets and we don't have fixed pattern for the mapping between
> CPU model name and CPU typename. I'm summarizing the used patterns like below.
>
> 1 All CPU model names are mappinged to fixed CPU typename;
> 2 CPU model name is same to CPU typename;
> 3 CPU model name is alias to CPU typename;
> 4 CPU model name is prefix of CPU typename;
>
> Target Categories suffix-of-CPU-typename
> -------------------------------------------------------
> alpha -234 -alpha-cpu
> arm ---4 -arm-cpu
> avr -2--
> cris --34 -cris-cpu
> hexagon ---4 -hexagon-cpu
> hppa 1---
> i386 ---4 -i386-cpu
> loongarch -2-4 -loongarch-cpu
> m68k ---4 -m68k-cpu
> microblaze 1---
> mips ---4 -mips64-cpu -mips-cpu
> nios2 1---
> openrisc ---4 -or1k-cpu
> ppc --34 -powerpc64-cpu -powerpc-cpu
> riscv ---4 -riscv-cpu
> rx -2-4 -rx-cpu
> s390x ---4 -s390x-cpu
> sh4 --34 -superh-cpu
> sparc -2--
> tricore ---4 -tricore-cpu
> xtensa ---4 -xtensa-cpu
That is unfortunate, however...
> There are several options as below. Please let me know which one or something
> else is the best.
>
> (a) Keep what we have and use mc->valid_{cpu_types, cpu_models}[] to track
> the valid CPU typenames and CPU model names.
>
> (b) Introduce CPUClass::model_name_by_typename(). Every target has their own
> implementation to convert CPU typename to CPU model name. The CPU model name
> is parsed from mc->valid_cpu_types[i].
>
> char *CPUClass::model_by_typename(const char *typename);
>
> (c) As we discussed before, use mc->valid_cpu_type_suffix and mc->valid_cpu_models
> because the CPU type check is currently needed by target arm/m68k/riscv where we
> do have fixed pattern to convert CPU model names to CPU typenames. The CPU typename
> is comprised of CPU model name and suffix. However, it won't be working when the CPU
> type check is required by other target where we have patterns other than this.
(d) Merge the two arrays together and use macro expansion, e.g.
typedef struct {
const char *name;
const char *type;
} Something;
#define ARM_SOMETHING(x) { x, ARM_CPU_TYPE_NAME(x) }
static const Something valid[] = {
ARM_SOMETHING("cortex-a53"),
{ NULL, NULL }
};
where Something ought to be better named.
r~
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/8] machine: Print supported CPU models instead of typenames
2023-07-27 9:00 ` Igor Mammedov
@ 2023-07-31 5:07 ` Gavin Shan
2023-08-28 14:46 ` Igor Mammedov
0 siblings, 1 reply; 21+ messages in thread
From: Gavin Shan @ 2023-07-31 5:07 UTC (permalink / raw)
To: Igor Mammedov
Cc: Richard Henderson, qemu-arm, qemu-devel, qemu-riscv,
peter.maydell, b.galvani, strahinja.p.jankovic, sundeep.lkml,
kfting, wuhaotsh, nieklinnenbank, rad, quic_llindhol,
marcin.juszkiewicz, eduardo, marcel.apfelbaum, philmd,
wangyanan55, laurent, vijai, palmer, alistair.francis, bin.meng,
liweiwei, dbarboza, zhiwei_liu, cohuck, pbonzini, shan.gavin,
qemu-ppc
On 7/27/23 19:00, Igor Mammedov wrote:
> On Thu, 27 Jul 2023 15:16:18 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>
>> On 7/27/23 09:08, Richard Henderson wrote:
>>> On 7/25/23 17:32, Gavin Shan wrote:
>>>> -static const char *q800_machine_valid_cpu_types[] = {
>>>> +static const char * const q800_machine_valid_cpu_types[] = {
>>>> M68K_CPU_TYPE_NAME("m68040"),
>>>> NULL
>>>> };
>>>> +static const char * const q800_machine_valid_cpu_models[] = {
>>>> + "m68040",
>>>> + NULL
>>>> +};
>>>
>>> I really don't like this replication.
>>>
>>
>> Right, it's going to be lots of replications, but gives much flexibility.
>> There are 21 targets and we don't have fixed pattern for the mapping between
>> CPU model name and CPU typename. I'm summarizing the used patterns like below.
>>
>> 1 All CPU model names are mappinged to fixed CPU typename;
>
> plainly spelled it would be: cpu_model name ignored and
> a cpu type is returned anyways.
>
> I'd make this hard error right away, as "junk in => error out"
> it's clearly user error. I think we don't even have to follow
> deprecation process for that.
>
Right, It's not expected behavior to map ambiguous CPU model names to
the fixed CPU typename.
>> 2 CPU model name is same to CPU typename;
>> 3 CPU model name is alias to CPU typename;
>> 4 CPU model name is prefix of CPU typename;
>
> and some more:
> 5. cpu model names aren't names at all sometimes, and some other
> CPU property is used. (ppc)
> This one I'd prefer to get rid of and ppc handling more consistent
> with other targets, which would need PPC folks to persuaded to drop
> PVR lookup.
>
I put this into class 3, meaning the PVRs are regarded as aliases to CPU
typenames.
>>
>> Target Categories suffix-of-CPU-typename
>> -------------------------------------------------------
>> alpha -234 -alpha-cpu
>> arm ---4 -arm-cpu
>> avr -2--
>> cris --34 -cris-cpu
>> hexagon ---4 -hexagon-cpu
>> hppa 1---
>> i386 ---4 -i386-cpu
>> loongarch -2-4 -loongarch-cpu
>> m68k ---4 -m68k-cpu
>> microblaze 1---
>> mips ---4 -mips64-cpu -mips-cpu
>> nios2 1---
>> openrisc ---4 -or1k-cpu
>> ppc --34 -powerpc64-cpu -powerpc-cpu
>> riscv ---4 -riscv-cpu
>> rx -2-4 -rx-cpu
>> s390x ---4 -s390x-cpu
>> sh4 --34 -superh-cpu
>> sparc -2--
>> tricore ---4 -tricore-cpu
>> xtensa ---4 -xtensa-cpu
>>
>> There are several options as below. Please let me know which one or something
>> else is the best.
>>
>> (a) Keep what we have and use mc->valid_{cpu_types, cpu_models}[] to track
>> the valid CPU typenames and CPU model names.
>>
>> (b) Introduce CPUClass::model_name_by_typename(). Every target has their own
>> implementation to convert CPU typename to CPU model name. The CPU model name
>> is parsed from mc->valid_cpu_types[i].
>>
>> char *CPUClass::model_by_typename(const char *typename);
>>
>> (c) As we discussed before, use mc->valid_cpu_type_suffix and mc->valid_cpu_models
>> because the CPU type check is currently needed by target arm/m68k/riscv where we
>> do have fixed pattern to convert CPU model names to CPU typenames. The CPU typename
>> is comprised of CPU model name and suffix. However, it won't be working when the CPU
>> type check is required by other target where we have patterns other than this.
>
> none of above is really good, that's why I was objecting to introducing
> reverse type->name mapping. That ends up with increased amount junk,
> and it's not because your patches are bad, but because you are trying
> to deal with cpu model names (which is a historically evolved mess).
> The best from engineering POV would be replacing CPU models with
> type names.
>
> Even though it's a bit radical, I very much prefer replacing
> cpu_model names with '-cpu type'usage directly. Making it
> consistent with -device/other interfaces and coincidentally that
> obsoletes need in reverse name mapping.
>
> It's painful for end users who will need to change configs/scripts,
> but it's one time job. Additionally from QEMU pov, codebase
> will be less messy => more maintainable which benefits not only
> developers but end-users in the end.
>
I have to clarify the type->model mapping has been existing since the
model->type mapping was introduced with the help of CPU_RESOLVING_TYPE.
I mean the logic has been existing since the existence of CPU_RESOLVING_TYPE,
even the code wasn't there.
I'm not sure about the idea to switch to '-cpu <cpu-type-name>' since
it was rejected by Peter Maydell before. Hope Peter can double confirm
for this. For me, the shorter name is beneficial. For example, users
needn't to have '-cpu host-arm-cpu' for '-cpu host'.
> [rant:
> It's the same story repeating over and over, when it comes to
> changing QEMU CLI, which hits resistance wall. But with QEMU
> deprecation process we've changed CLI behavior before,
> despite of that world didn't cease to exist and users
> have adapted to new QEMU and arguably QEMU became a tiny
> bit more maintainable since we don't have to deal some
> legacy behavior.
> ]
>
I need more context about 'deprecation process' here. My understanding
is both CPU typename and model name will be accepted for a fixed period
of time. However, a warning message will be given to indicate that the
model name will be obsoleted soon. Eventually, we switch to CPU typename
completely. Please correct me if there are anything wrong.
> Another idea back in the days was (as a compromise),
> 1. keep using keep valid_cpu_types
> 2. instead of introducing yet another way to do reverse mapping,
> clean/generalize/make it work everywhere list_cpus (which
> already does that mapping) and then use that to do your thing.
> It will have drawbacks you've listed above, but hopefully
> that will clean up and reuse existing list_cpus.
> (only this time, I'd build it around query-cpu-model-expansion,
> which output is used by generic list_cpus)
> [and here I'm asking to rewrite directly unrelated QEMU part yet again]
>
I'm afraid that list_cpus() is hard to be reused. All available CPU model names
are listed by list_cpus(). mc->valid_cpu_types[] are just part of them and variable
on basis of boards. Generally speaking, we need a function to do reverse things
as to CPUClass::class_by_name(). So I would suggest to introduce CPUClass::model_from_type(),
as below. Could you please suggest if it sounds reasonable to you?
- CPUClass::class_by_name() is modified to
char *CPUClass::model_to_type(const char *model)
- char *CPUClass::type_to_model(const char *type)
- CPUClass::type_to_model() is used in cpu_list() for every target when CPU
model name, fetched from CPU type name, is printed in xxx_cpu_list_entry()
- CPUClass::type_to_model() is reused in hw/core/machine.c to get the CPU
model name from CPU type names in mc->valid_cpu_types[].
Thanks,
Gavin
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/8] machine: Print supported CPU models instead of typenames
2023-07-27 14:27 ` Richard Henderson
@ 2023-07-31 5:33 ` Gavin Shan
0 siblings, 0 replies; 21+ messages in thread
From: Gavin Shan @ 2023-07-31 5:33 UTC (permalink / raw)
To: Richard Henderson, qemu-arm
Cc: qemu-devel, qemu-riscv, peter.maydell, b.galvani,
strahinja.p.jankovic, sundeep.lkml, kfting, wuhaotsh,
nieklinnenbank, rad, quic_llindhol, marcin.juszkiewicz, eduardo,
marcel.apfelbaum, philmd, wangyanan55, laurent, vijai, palmer,
alistair.francis, bin.meng, liweiwei, dbarboza, zhiwei_liu,
imammedo, cohuck, pbonzini, shan.gavin
On 7/28/23 00:27, Richard Henderson wrote:
> On 7/26/23 22:16, Gavin Shan wrote:
>>
>> On 7/27/23 09:08, Richard Henderson wrote:
>>> On 7/25/23 17:32, Gavin Shan wrote:
>>>> -static const char *q800_machine_valid_cpu_types[] = {
>>>> +static const char * const q800_machine_valid_cpu_types[] = {
>>>> M68K_CPU_TYPE_NAME("m68040"),
>>>> NULL
>>>> };
>>>> +static const char * const q800_machine_valid_cpu_models[] = {
>>>> + "m68040",
>>>> + NULL
>>>> +};
>>>
>>> I really don't like this replication.
>>>
>>
>> Right, it's going to be lots of replications, but gives much flexibility.
>> There are 21 targets and we don't have fixed pattern for the mapping between
>> CPU model name and CPU typename. I'm summarizing the used patterns like below.
>>
>> 1 All CPU model names are mappinged to fixed CPU typename;
>> 2 CPU model name is same to CPU typename;
>> 3 CPU model name is alias to CPU typename;
>> 4 CPU model name is prefix of CPU typename;
>>
>> Target Categories suffix-of-CPU-typename
>> -------------------------------------------------------
>> alpha -234 -alpha-cpu
>> arm ---4 -arm-cpu
>> avr -2--
>> cris --34 -cris-cpu
>> hexagon ---4 -hexagon-cpu
>> hppa 1---
>> i386 ---4 -i386-cpu
>> loongarch -2-4 -loongarch-cpu
>> m68k ---4 -m68k-cpu
>> microblaze 1---
>> mips ---4 -mips64-cpu -mips-cpu
>> nios2 1---
>> openrisc ---4 -or1k-cpu
>> ppc --34 -powerpc64-cpu -powerpc-cpu
>> riscv ---4 -riscv-cpu
>> rx -2-4 -rx-cpu
>> s390x ---4 -s390x-cpu
>> sh4 --34 -superh-cpu
>> sparc -2--
>> tricore ---4 -tricore-cpu
>> xtensa ---4 -xtensa-cpu
>
> That is unfortunate, however...
>
>
>> There are several options as below. Please let me know which one or something
>> else is the best.
>>
>> (a) Keep what we have and use mc->valid_{cpu_types, cpu_models}[] to track
>> the valid CPU typenames and CPU model names.
>>
>> (b) Introduce CPUClass::model_name_by_typename(). Every target has their own
>> implementation to convert CPU typename to CPU model name. The CPU model name
>> is parsed from mc->valid_cpu_types[i].
>>
>> char *CPUClass::model_by_typename(const char *typename);
>>
>> (c) As we discussed before, use mc->valid_cpu_type_suffix and mc->valid_cpu_models
>> because the CPU type check is currently needed by target arm/m68k/riscv where we
>> do have fixed pattern to convert CPU model names to CPU typenames. The CPU typename
>> is comprised of CPU model name and suffix. However, it won't be working when the CPU
>> type check is required by other target where we have patterns other than this.
>
> (d) Merge the two arrays together and use macro expansion, e.g.
>
> typedef struct {
> const char *name;
> const char *type;
> } Something;
>
> #define ARM_SOMETHING(x) { x, ARM_CPU_TYPE_NAME(x) }
>
> static const Something valid[] = {
> ARM_SOMETHING("cortex-a53"),
> { NULL, NULL }
> };
>
> where Something ought to be better named.
>
Thanks, Richard. It's a nice idea, but not generalized enough. Igor suggested
to reuse the existing list_cpus() in another reply, and I suggested to add
CPUClass::type_to_model() to convert CPU type name to model name for every
target. Please take look and comment when you get a chance.
https://lists.nongnu.org/archive/html/qemu-arm/2023-07/msg00659.html
Thanks,
Gavin
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/8] machine: Print supported CPU models instead of typenames
2023-07-31 5:07 ` Gavin Shan
@ 2023-08-28 14:46 ` Igor Mammedov
2023-08-29 6:28 ` Gavin Shan
0 siblings, 1 reply; 21+ messages in thread
From: Igor Mammedov @ 2023-08-28 14:46 UTC (permalink / raw)
To: Gavin Shan
Cc: Richard Henderson, qemu-arm, qemu-devel, qemu-riscv,
peter.maydell, b.galvani, strahinja.p.jankovic, sundeep.lkml,
kfting, wuhaotsh, nieklinnenbank, rad, quic_llindhol,
marcin.juszkiewicz, eduardo, marcel.apfelbaum, philmd,
wangyanan55, laurent, vijai, palmer, alistair.francis, bin.meng,
liweiwei, dbarboza, zhiwei_liu, cohuck, pbonzini, shan.gavin,
qemu-ppc
On Mon, 31 Jul 2023 15:07:30 +1000
Gavin Shan <gshan@redhat.com> wrote:
> On 7/27/23 19:00, Igor Mammedov wrote:
> > On Thu, 27 Jul 2023 15:16:18 +1000
> > Gavin Shan <gshan@redhat.com> wrote:
> >
> >> On 7/27/23 09:08, Richard Henderson wrote:
> >>> On 7/25/23 17:32, Gavin Shan wrote:
> >>>> -static const char *q800_machine_valid_cpu_types[] = {
> >>>> +static const char * const q800_machine_valid_cpu_types[] = {
> >>>> M68K_CPU_TYPE_NAME("m68040"),
> >>>> NULL
> >>>> };
> >>>> +static const char * const q800_machine_valid_cpu_models[] = {
> >>>> + "m68040",
> >>>> + NULL
> >>>> +};
> >>>
> >>> I really don't like this replication.
> >>>
> >>
> >> Right, it's going to be lots of replications, but gives much flexibility.
> >> There are 21 targets and we don't have fixed pattern for the mapping between
> >> CPU model name and CPU typename. I'm summarizing the used patterns like below.
> >>
> >> 1 All CPU model names are mappinged to fixed CPU typename;
> >
> > plainly spelled it would be: cpu_model name ignored and
> > a cpu type is returned anyways.
> >
> > I'd make this hard error right away, as "junk in => error out"
> > it's clearly user error. I think we don't even have to follow
> > deprecation process for that.
> >
>
> Right, It's not expected behavior to map ambiguous CPU model names to
> the fixed CPU typename.
to be nice we can deprecate those and then later remove.
(while deprecating make those targets accept typenames)
>
> >> 2 CPU model name is same to CPU typename;
> >> 3 CPU model name is alias to CPU typename;
> >> 4 CPU model name is prefix of CPU typename;
> >
> > and some more:
> > 5. cpu model names aren't names at all sometimes, and some other
> > CPU property is used. (ppc)
> > This one I'd prefer to get rid of and ppc handling more consistent
> > with other targets, which would need PPC folks to persuaded to drop
> > PVR lookup.
> >
>
> I put this into class 3, meaning the PVRs are regarded as aliases to CPU
> typenames.
with PPC using 'true' aliases, -cpu input is lost after it's translated into typename.
(same for alpha)
it also adds an extra fun with 'max' cpu model but that boils down to above statement.
(same for
* sh4
* cris(in user mode only, but you are making sysemu extension, so it doesn't count)
)
For this class of aliases reverse translation won't yield the same
result as used -cpu. The only option you have is to store -cpu cpu_model
somewhere (use qemu_opts??, and then fetch it later to print in error message)
x86 has 'aliases' as well, but in reality it creates distinct cpu types
for each 'alias', so it's possible to do reverse translation.
> >>
> >> Target Categories suffix-of-CPU-typename
> >> -------------------------------------------------------
> >> alpha -234 -alpha-cpu
> >> arm ---4 -arm-cpu
> >> avr -2--
> >> cris --34 -cris-cpu
> >> hexagon ---4 -hexagon-cpu
> >> hppa 1---
> >> i386 ---4 -i386-cpu
> >> loongarch -2-4 -loongarch-cpu
> >> m68k ---4 -m68k-cpu
> >> microblaze 1---
> >> mips ---4 -mips64-cpu -mips-cpu
> >> nios2 1---
> >> openrisc ---4 -or1k-cpu
> >> ppc --34 -powerpc64-cpu -powerpc-cpu
> >> riscv ---4 -riscv-cpu
> >> rx -2-4 -rx-cpu
> >> s390x ---4 -s390x-cpu
> >> sh4 --34 -superh-cpu
> >> sparc -2--
it's case 4
> >> tricore ---4 -tricore-cpu
> >> xtensa ---4 -xtensa-cpu
> >>
> >> There are several options as below. Please let me know which one or something
> >> else is the best.
> >>
> >> (a) Keep what we have and use mc->valid_{cpu_types, cpu_models}[] to track
> >> the valid CPU typenames and CPU model names.
> >>
> >> (b) Introduce CPUClass::model_name_by_typename(). Every target has their own
> >> implementation to convert CPU typename to CPU model name. The CPU model name
> >> is parsed from mc->valid_cpu_types[i].
> >>
> >> char *CPUClass::model_by_typename(const char *typename);
> >>
> >> (c) As we discussed before, use mc->valid_cpu_type_suffix and mc->valid_cpu_models
> >> because the CPU type check is currently needed by target arm/m68k/riscv where we
> >> do have fixed pattern to convert CPU model names to CPU typenames. The CPU typename
> >> is comprised of CPU model name and suffix. However, it won't be working when the CPU
> >> type check is required by other target where we have patterns other than this.
> >
> > none of above is really good, that's why I was objecting to introducing
> > reverse type->name mapping. That ends up with increased amount junk,
> > and it's not because your patches are bad, but because you are trying
> > to deal with cpu model names (which is a historically evolved mess).
> > The best from engineering POV would be replacing CPU models with
> > type names.
> >
> > Even though it's a bit radical, I very much prefer replacing
> > cpu_model names with '-cpu type'usage directly. Making it
> > consistent with -device/other interfaces and coincidentally that
> > obsoletes need in reverse name mapping.
> >
> > It's painful for end users who will need to change configs/scripts,
> > but it's one time job. Additionally from QEMU pov, codebase
> > will be less messy => more maintainable which benefits not only
> > developers but end-users in the end.
> >
>
> I have to clarify the type->model mapping has been existing since the
> model->type mapping was introduced with the help of CPU_RESOLVING_TYPE.
> I mean the logic has been existing since the existence of CPU_RESOLVING_TYPE,
> even the code wasn't there.
>
> I'm not sure about the idea to switch to '-cpu <cpu-type-name>' since
> it was rejected by Peter Maydell before. Hope Peter can double confirm
> for this. For me, the shorter name is beneficial. For example, users
> needn't to have '-cpu host-arm-cpu' for '-cpu host'.
>
>
> > [rant:
> > It's the same story repeating over and over, when it comes to
> > changing QEMU CLI, which hits resistance wall. But with QEMU
> > deprecation process we've changed CLI behavior before,
> > despite of that world didn't cease to exist and users
> > have adapted to new QEMU and arguably QEMU became a tiny
> > bit more maintainable since we don't have to deal some
> > legacy behavior.
> > ]
> >
>
> I need more context about 'deprecation process' here. My understanding
> is both CPU typename and model name will be accepted for a fixed period
> of time. However, a warning message will be given to indicate that the
> model name will be obsoleted soon. Eventually, we switch to CPU typename
> completely. Please correct me if there are anything wrong.
yep, that's the gist of deprecation in this case.
> > Another idea back in the days was (as a compromise),
> > 1. keep using keep valid_cpu_types
> > 2. instead of introducing yet another way to do reverse mapping,
> > clean/generalize/make it work everywhere list_cpus (which
> > already does that mapping) and then use that to do your thing.
> > It will have drawbacks you've listed above, but hopefully
> > that will clean up and reuse existing list_cpus.
> > (only this time, I'd build it around query-cpu-model-expansion,
> > which output is used by generic list_cpus)
> > [and here I'm asking to rewrite directly unrelated QEMU part yet again]
> >
>
> I'm afraid that list_cpus() is hard to be reused. All available CPU model names
> are listed by list_cpus(). mc->valid_cpu_types[] are just part of them and variable
> on basis of boards. Generally speaking, we need a function to do reverse things
> as to CPUClass::class_by_name(). So I would suggest to introduce CPUClass::model_from_type(),
> as below. Could you please suggest if it sounds reasonable to you?
>
> - CPUClass::class_by_name() is modified to
> char *CPUClass::model_to_type(const char *model)
>
> - char *CPUClass::type_to_model(const char *type)
>
> - CPUClass::type_to_model() is used in cpu_list() for every target when CPU
> model name, fetched from CPU type name, is printed in xxx_cpu_list_entry()
>
> - CPUClass::type_to_model() is reused in hw/core/machine.c to get the CPU
> model name from CPU type names in mc->valid_cpu_types[].
instead of per target hooks (which are atm mostly open-coded in several places)
how about adding generic handler for cases 2,4:
cpu_type_to_model(typename)
cpu_suffix = re'-*-cpu'
if (class_exists(typename - cpu_suffix))
return typename - cpu_suffix
else if (class_exists(typename))
return typename
explode
that should work for translating valid_cpus typenames to cpumodel names
and once that in place cleanup all open-coded translations with it tree-wide
you can find those easily by:
git grep _CPU_TYPE_SUFFIX
git grep query_cpu_definitions
>
> Thanks,
> Gavin
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/8] machine: Print supported CPU models instead of typenames
2023-08-28 14:46 ` Igor Mammedov
@ 2023-08-29 6:28 ` Gavin Shan
2023-08-29 9:03 ` Igor Mammedov
0 siblings, 1 reply; 21+ messages in thread
From: Gavin Shan @ 2023-08-29 6:28 UTC (permalink / raw)
To: Igor Mammedov
Cc: Richard Henderson, qemu-arm, qemu-devel, qemu-riscv,
peter.maydell, b.galvani, strahinja.p.jankovic, sundeep.lkml,
kfting, wuhaotsh, nieklinnenbank, rad, quic_llindhol,
marcin.juszkiewicz, eduardo, marcel.apfelbaum, philmd,
wangyanan55, laurent, vijai, palmer, alistair.francis, bin.meng,
liweiwei, dbarboza, zhiwei_liu, cohuck, pbonzini, shan.gavin,
qemu-ppc
Hi Igor,
On 8/29/23 00:46, Igor Mammedov wrote:
> On Mon, 31 Jul 2023 15:07:30 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>> On 7/27/23 19:00, Igor Mammedov wrote:
>>> On Thu, 27 Jul 2023 15:16:18 +1000
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>
>>>> On 7/27/23 09:08, Richard Henderson wrote:
>>>>> On 7/25/23 17:32, Gavin Shan wrote:
>>>>>> -static const char *q800_machine_valid_cpu_types[] = {
>>>>>> +static const char * const q800_machine_valid_cpu_types[] = {
>>>>>> M68K_CPU_TYPE_NAME("m68040"),
>>>>>> NULL
>>>>>> };
>>>>>> +static const char * const q800_machine_valid_cpu_models[] = {
>>>>>> + "m68040",
>>>>>> + NULL
>>>>>> +};
>>>>>
>>>>> I really don't like this replication.
>>>>>
>>>>
>>>> Right, it's going to be lots of replications, but gives much flexibility.
>>>> There are 21 targets and we don't have fixed pattern for the mapping between
>>>> CPU model name and CPU typename. I'm summarizing the used patterns like below.
>>>>
>>>> 1 All CPU model names are mappinged to fixed CPU typename;
>>>
>>> plainly spelled it would be: cpu_model name ignored and
>>> a cpu type is returned anyways.
>>>
>>> I'd make this hard error right away, as "junk in => error out"
>>> it's clearly user error. I think we don't even have to follow
>>> deprecation process for that.
>>>
>>
>> Right, It's not expected behavior to map ambiguous CPU model names to
>> the fixed CPU typename.
>
> to be nice we can deprecate those and then later remove.
> (while deprecating make those targets accept typenames)
>
Lets put it aside for now and revisit it later, so that we can focus on
the conversion from the CPU type name to the CPU model name for now.
>>
>>>> 2 CPU model name is same to CPU typename;
>>>> 3 CPU model name is alias to CPU typename;
>>>> 4 CPU model name is prefix of CPU typename;
>>>
>>> and some more:
>>> 5. cpu model names aren't names at all sometimes, and some other
>>> CPU property is used. (ppc)
>>> This one I'd prefer to get rid of and ppc handling more consistent
>>> with other targets, which would need PPC folks to persuaded to drop
>>> PVR lookup.
>>>
>>
>> I put this into class 3, meaning the PVRs are regarded as aliases to CPU
>> typenames.
>
> with PPC using 'true' aliases, -cpu input is lost after it's translated into typename.
> (same for alpha)
>
> it also adds an extra fun with 'max' cpu model but that boils down to above statement.
> (same for
> * sh4
> * cris(in user mode only, but you are making sysemu extension, so it doesn't count)
> )
> For this class of aliases reverse translation won't yield the same
> result as used -cpu. The only option you have is to store -cpu cpu_model
> somewhere (use qemu_opts??, and then fetch it later to print in error message)
>
> x86 has 'aliases' as well, but in reality it creates distinct cpu types
> for each 'alias', so it's possible to do reverse translation.
>
It's true that '-cpu input' gets lost in these cases. However, the CPU type
name instead of the CPU model name is printed in the error message when the
CPU type is validated in hw/core/machine.c::machine_run_board_init(). It looks
good to me to print the CPU type name instead of the model name there.
Another error message is printed when the CPU model specified in '-cpu input'
isn't valid. The CPU model has been printed and it looks good either.
# qemu-system-aarch64 -M virt -cpu aaa
qemu-system-aarch64: unable to find CPU model 'aaa'
Are there other cases I missed where we need to print the CPU model name, which
is specified by user through '-cpu input'?
>>>>
>>>> Target Categories suffix-of-CPU-typename
>>>> -------------------------------------------------------
>>>> alpha -234 -alpha-cpu
>>>> arm ---4 -arm-cpu
>>>> avr -2--
>>>> cris --34 -cris-cpu
>>>> hexagon ---4 -hexagon-cpu
>>>> hppa 1---
>>>> i386 ---4 -i386-cpu
>>>> loongarch -2-4 -loongarch-cpu
>>>> m68k ---4 -m68k-cpu
>>>> microblaze 1---
>>>> mips ---4 -mips64-cpu -mips-cpu
>>>> nios2 1---
>>>> openrisc ---4 -or1k-cpu
>>>> ppc --34 -powerpc64-cpu -powerpc-cpu
>>>> riscv ---4 -riscv-cpu
>>>> rx -2-4 -rx-cpu
>>>> s390x ---4 -s390x-cpu
>>>> sh4 --34 -superh-cpu
>>>> sparc -2--
>
> it's case 4
>
Yes.
>>>> tricore ---4 -tricore-cpu
>>>> xtensa ---4 -xtensa-cpu
>>>>
>>>> There are several options as below. Please let me know which one or something
>>>> else is the best.
>>>>
>>>> (a) Keep what we have and use mc->valid_{cpu_types, cpu_models}[] to track
>>>> the valid CPU typenames and CPU model names.
>>>>
>>>> (b) Introduce CPUClass::model_name_by_typename(). Every target has their own
>>>> implementation to convert CPU typename to CPU model name. The CPU model name
>>>> is parsed from mc->valid_cpu_types[i].
>>>>
>>>> char *CPUClass::model_by_typename(const char *typename);
>>>>
>>>> (c) As we discussed before, use mc->valid_cpu_type_suffix and mc->valid_cpu_models
>>>> because the CPU type check is currently needed by target arm/m68k/riscv where we
>>>> do have fixed pattern to convert CPU model names to CPU typenames. The CPU typename
>>>> is comprised of CPU model name and suffix. However, it won't be working when the CPU
>>>> type check is required by other target where we have patterns other than this.
>>>
>>> none of above is really good, that's why I was objecting to introducing
>>> reverse type->name mapping. That ends up with increased amount junk,
>>> and it's not because your patches are bad, but because you are trying
>>> to deal with cpu model names (which is a historically evolved mess).
>>> The best from engineering POV would be replacing CPU models with
>>> type names.
>>>
>>> Even though it's a bit radical, I very much prefer replacing
>>> cpu_model names with '-cpu type'usage directly. Making it
>>> consistent with -device/other interfaces and coincidentally that
>>> obsoletes need in reverse name mapping.
>>>
>>> It's painful for end users who will need to change configs/scripts,
>>> but it's one time job. Additionally from QEMU pov, codebase
>>> will be less messy => more maintainable which benefits not only
>>> developers but end-users in the end.
>>>
>>
>> I have to clarify the type->model mapping has been existing since the
>> model->type mapping was introduced with the help of CPU_RESOLVING_TYPE.
>> I mean the logic has been existing since the existence of CPU_RESOLVING_TYPE,
>> even the code wasn't there.
>>
>> I'm not sure about the idea to switch to '-cpu <cpu-type-name>' since
>> it was rejected by Peter Maydell before. Hope Peter can double confirm
>> for this. For me, the shorter name is beneficial. For example, users
>> needn't to have '-cpu host-arm-cpu' for '-cpu host'.
>>
>>
>>> [rant:
>>> It's the same story repeating over and over, when it comes to
>>> changing QEMU CLI, which hits resistance wall. But with QEMU
>>> deprecation process we've changed CLI behavior before,
>>> despite of that world didn't cease to exist and users
>>> have adapted to new QEMU and arguably QEMU became a tiny
>>> bit more maintainable since we don't have to deal some
>>> legacy behavior.
>>> ]
>>>
>>
>> I need more context about 'deprecation process' here. My understanding
>> is both CPU typename and model name will be accepted for a fixed period
>> of time. However, a warning message will be given to indicate that the
>> model name will be obsoleted soon. Eventually, we switch to CPU typename
>> completely. Please correct me if there are anything wrong.
>
> yep, that's the gist of deprecation in this case.
>
Ok. Thanks for your confirm.
>>> Another idea back in the days was (as a compromise),
>>> 1. keep using keep valid_cpu_types
>>> 2. instead of introducing yet another way to do reverse mapping,
>>> clean/generalize/make it work everywhere list_cpus (which
>>> already does that mapping) and then use that to do your thing.
>>> It will have drawbacks you've listed above, but hopefully
>>> that will clean up and reuse existing list_cpus.
>>> (only this time, I'd build it around query-cpu-model-expansion,
>>> which output is used by generic list_cpus)
>>> [and here I'm asking to rewrite directly unrelated QEMU part yet again]
>>>
>>
>> I'm afraid that list_cpus() is hard to be reused. All available CPU model names
>> are listed by list_cpus(). mc->valid_cpu_types[] are just part of them and variable
>> on basis of boards. Generally speaking, we need a function to do reverse things
>> as to CPUClass::class_by_name(). So I would suggest to introduce CPUClass::model_from_type(),
>> as below. Could you please suggest if it sounds reasonable to you?
>>
>> - CPUClass::class_by_name() is modified to
>> char *CPUClass::model_to_type(const char *model)
>>
>> - char *CPUClass::type_to_model(const char *type)
>>
>> - CPUClass::type_to_model() is used in cpu_list() for every target when CPU
>> model name, fetched from CPU type name, is printed in xxx_cpu_list_entry()
>>
>> - CPUClass::type_to_model() is reused in hw/core/machine.c to get the CPU
>> model name from CPU type names in mc->valid_cpu_types[].
>
> instead of per target hooks (which are atm mostly open-coded in several places)
> how about adding generic handler for cases 2,4:
> cpu_type_to_model(typename)
> cpu_suffix = re'-*-cpu'
> if (class_exists(typename - cpu_suffix))
> return typename - cpu_suffix
> else if (class_exists(typename))
> return typename
> explode
>
> that should work for translating valid_cpus typenames to cpumodel names
> and once that in place cleanup all open-coded translations with it tree-wide
>
> you can find those easily by:
> git grep _CPU_TYPE_SUFFIX
> git grep query_cpu_definitions
>
Thanks for the advice. I think it's enough for now since the CPU type
invalidation is currently done for arm/mips/riscv for now. On these
targets, the CPU type name is always the combination of the CPU model
name and suffix. I will add helper qemu/cpu.c::cpu_model_by_name()
as you suggested. Note that, the suffix can be gained by ("-" CPU_RESOLVING_TYPE)
Yes, the newly added helper cpu_model_by_name() needs to be applied
to targets where query_cpu_definitions and cpu_list are defined.
Thanks,
Gavin
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/8] machine: Print supported CPU models instead of typenames
2023-08-29 6:28 ` Gavin Shan
@ 2023-08-29 9:03 ` Igor Mammedov
2023-08-30 7:34 ` Gavin Shan
0 siblings, 1 reply; 21+ messages in thread
From: Igor Mammedov @ 2023-08-29 9:03 UTC (permalink / raw)
To: Gavin Shan
Cc: Richard Henderson, qemu-arm, qemu-devel, qemu-riscv,
peter.maydell, b.galvani, strahinja.p.jankovic, sundeep.lkml,
kfting, wuhaotsh, nieklinnenbank, rad, quic_llindhol,
marcin.juszkiewicz, eduardo, marcel.apfelbaum, philmd,
wangyanan55, laurent, vijai, palmer, alistair.francis, bin.meng,
liweiwei, dbarboza, zhiwei_liu, cohuck, pbonzini, shan.gavin,
qemu-ppc
On Tue, 29 Aug 2023 16:28:45 +1000
Gavin Shan <gshan@redhat.com> wrote:
> Hi Igor,
>
> On 8/29/23 00:46, Igor Mammedov wrote:
> > On Mon, 31 Jul 2023 15:07:30 +1000
> > Gavin Shan <gshan@redhat.com> wrote:
> >> On 7/27/23 19:00, Igor Mammedov wrote:
> >>> On Thu, 27 Jul 2023 15:16:18 +1000
> >>> Gavin Shan <gshan@redhat.com> wrote:
> >>>
> >>>> On 7/27/23 09:08, Richard Henderson wrote:
> >>>>> On 7/25/23 17:32, Gavin Shan wrote:
> >>>>>> -static const char *q800_machine_valid_cpu_types[] = {
> >>>>>> +static const char * const q800_machine_valid_cpu_types[] = {
> >>>>>> M68K_CPU_TYPE_NAME("m68040"),
> >>>>>> NULL
> >>>>>> };
> >>>>>> +static const char * const q800_machine_valid_cpu_models[] = {
> >>>>>> + "m68040",
> >>>>>> + NULL
> >>>>>> +};
> >>>>>
> >>>>> I really don't like this replication.
> >>>>>
> >>>>
> >>>> Right, it's going to be lots of replications, but gives much flexibility.
> >>>> There are 21 targets and we don't have fixed pattern for the mapping between
> >>>> CPU model name and CPU typename. I'm summarizing the used patterns like below.
> >>>>
> >>>> 1 All CPU model names are mappinged to fixed CPU typename;
> >>>
> >>> plainly spelled it would be: cpu_model name ignored and
> >>> a cpu type is returned anyways.
> >>>
> >>> I'd make this hard error right away, as "junk in => error out"
> >>> it's clearly user error. I think we don't even have to follow
> >>> deprecation process for that.
> >>>
> >>
> >> Right, It's not expected behavior to map ambiguous CPU model names to
> >> the fixed CPU typename.
> >
> > to be nice we can deprecate those and then later remove.
> > (while deprecating make those targets accept typenames)
> >
>
> Lets put it aside for now and revisit it later, so that we can focus on
> the conversion from the CPU type name to the CPU model name for now.
>
> >>
> >>>> 2 CPU model name is same to CPU typename;
> >>>> 3 CPU model name is alias to CPU typename;
> >>>> 4 CPU model name is prefix of CPU typename;
> >>>
> >>> and some more:
> >>> 5. cpu model names aren't names at all sometimes, and some other
> >>> CPU property is used. (ppc)
> >>> This one I'd prefer to get rid of and ppc handling more consistent
> >>> with other targets, which would need PPC folks to persuaded to drop
> >>> PVR lookup.
> >>>
> >>
> >> I put this into class 3, meaning the PVRs are regarded as aliases to CPU
> >> typenames.
> >
> > with PPC using 'true' aliases, -cpu input is lost after it's translated into typename.
> > (same for alpha)
> >
> > it also adds an extra fun with 'max' cpu model but that boils down to above statement.
> > (same for
> > * sh4
> > * cris(in user mode only, but you are making sysemu extension, so it doesn't count)
> > )
> > For this class of aliases reverse translation won't yield the same
> > result as used -cpu. The only option you have is to store -cpu cpu_model
> > somewhere (use qemu_opts??, and then fetch it later to print in error message)
> >
> > x86 has 'aliases' as well, but in reality it creates distinct cpu types
> > for each 'alias', so it's possible to do reverse translation.
> >
>
> It's true that '-cpu input' gets lost in these cases. However, the CPU type
> name instead of the CPU model name is printed in the error message when the
> CPU type is validated in hw/core/machine.c::machine_run_board_init(). It looks
> good to me to print the CPU type name instead of the model name there.
It's the same confusing whether it's type or cpumodel it it doesn't match
user provided value.
> Another error message is printed when the CPU model specified in '-cpu input'
> isn't valid. The CPU model has been printed and it looks good either.
>
> # qemu-system-aarch64 -M virt -cpu aaa
> qemu-system-aarch64: unable to find CPU model 'aaa'
>
> Are there other cases I missed where we need to print the CPU model name, which
> is specified by user through '-cpu input'?
>
> >>>>
> >>>> Target Categories suffix-of-CPU-typename
> >>>> -------------------------------------------------------
> >>>> alpha -234 -alpha-cpu
> >>>> arm ---4 -arm-cpu
> >>>> avr -2--
> >>>> cris --34 -cris-cpu
> >>>> hexagon ---4 -hexagon-cpu
> >>>> hppa 1---
> >>>> i386 ---4 -i386-cpu
> >>>> loongarch -2-4 -loongarch-cpu
> >>>> m68k ---4 -m68k-cpu
> >>>> microblaze 1---
> >>>> mips ---4 -mips64-cpu -mips-cpu
> >>>> nios2 1---
> >>>> openrisc ---4 -or1k-cpu
> >>>> ppc --34 -powerpc64-cpu -powerpc-cpu
> >>>> riscv ---4 -riscv-cpu
> >>>> rx -2-4 -rx-cpu
> >>>> s390x ---4 -s390x-cpu
> >>>> sh4 --34 -superh-cpu
> >>>> sparc -2--
> >
> > it's case 4
> >
>
> Yes.
>
> >>>> tricore ---4 -tricore-cpu
> >>>> xtensa ---4 -xtensa-cpu
> >>>>
> >>>> There are several options as below. Please let me know which one or something
> >>>> else is the best.
> >>>>
> >>>> (a) Keep what we have and use mc->valid_{cpu_types, cpu_models}[] to track
> >>>> the valid CPU typenames and CPU model names.
> >>>>
> >>>> (b) Introduce CPUClass::model_name_by_typename(). Every target has their own
> >>>> implementation to convert CPU typename to CPU model name. The CPU model name
> >>>> is parsed from mc->valid_cpu_types[i].
> >>>>
> >>>> char *CPUClass::model_by_typename(const char *typename);
> >>>>
> >>>> (c) As we discussed before, use mc->valid_cpu_type_suffix and mc->valid_cpu_models
> >>>> because the CPU type check is currently needed by target arm/m68k/riscv where we
> >>>> do have fixed pattern to convert CPU model names to CPU typenames. The CPU typename
> >>>> is comprised of CPU model name and suffix. However, it won't be working when the CPU
> >>>> type check is required by other target where we have patterns other than this.
> >>>
> >>> none of above is really good, that's why I was objecting to introducing
> >>> reverse type->name mapping. That ends up with increased amount junk,
> >>> and it's not because your patches are bad, but because you are trying
> >>> to deal with cpu model names (which is a historically evolved mess).
> >>> The best from engineering POV would be replacing CPU models with
> >>> type names.
> >>>
> >>> Even though it's a bit radical, I very much prefer replacing
> >>> cpu_model names with '-cpu type'usage directly. Making it
> >>> consistent with -device/other interfaces and coincidentally that
> >>> obsoletes need in reverse name mapping.
> >>>
> >>> It's painful for end users who will need to change configs/scripts,
> >>> but it's one time job. Additionally from QEMU pov, codebase
> >>> will be less messy => more maintainable which benefits not only
> >>> developers but end-users in the end.
> >>>
> >>
> >> I have to clarify the type->model mapping has been existing since the
> >> model->type mapping was introduced with the help of CPU_RESOLVING_TYPE.
> >> I mean the logic has been existing since the existence of CPU_RESOLVING_TYPE,
> >> even the code wasn't there.
> >>
> >> I'm not sure about the idea to switch to '-cpu <cpu-type-name>' since
> >> it was rejected by Peter Maydell before. Hope Peter can double confirm
> >> for this. For me, the shorter name is beneficial. For example, users
> >> needn't to have '-cpu host-arm-cpu' for '-cpu host'.
> >>
> >>
> >>> [rant:
> >>> It's the same story repeating over and over, when it comes to
> >>> changing QEMU CLI, which hits resistance wall. But with QEMU
> >>> deprecation process we've changed CLI behavior before,
> >>> despite of that world didn't cease to exist and users
> >>> have adapted to new QEMU and arguably QEMU became a tiny
> >>> bit more maintainable since we don't have to deal some
> >>> legacy behavior.
> >>> ]
> >>>
> >>
> >> I need more context about 'deprecation process' here. My understanding
> >> is both CPU typename and model name will be accepted for a fixed period
> >> of time. However, a warning message will be given to indicate that the
> >> model name will be obsoleted soon. Eventually, we switch to CPU typename
> >> completely. Please correct me if there are anything wrong.
> >
> > yep, that's the gist of deprecation in this case.
> >
>
> Ok. Thanks for your confirm.
>
> >>> Another idea back in the days was (as a compromise),
> >>> 1. keep using keep valid_cpu_types
> >>> 2. instead of introducing yet another way to do reverse mapping,
> >>> clean/generalize/make it work everywhere list_cpus (which
> >>> already does that mapping) and then use that to do your thing.
> >>> It will have drawbacks you've listed above, but hopefully
> >>> that will clean up and reuse existing list_cpus.
> >>> (only this time, I'd build it around query-cpu-model-expansion,
> >>> which output is used by generic list_cpus)
> >>> [and here I'm asking to rewrite directly unrelated QEMU part yet again]
> >>>
> >>
> >> I'm afraid that list_cpus() is hard to be reused. All available CPU model names
> >> are listed by list_cpus(). mc->valid_cpu_types[] are just part of them and variable
> >> on basis of boards. Generally speaking, we need a function to do reverse things
> >> as to CPUClass::class_by_name(). So I would suggest to introduce CPUClass::model_from_type(),
> >> as below. Could you please suggest if it sounds reasonable to you?
> >>
> >> - CPUClass::class_by_name() is modified to
> >> char *CPUClass::model_to_type(const char *model)
> >>
> >> - char *CPUClass::type_to_model(const char *type)
> >>
> >> - CPUClass::type_to_model() is used in cpu_list() for every target when CPU
> >> model name, fetched from CPU type name, is printed in xxx_cpu_list_entry()
> >>
> >> - CPUClass::type_to_model() is reused in hw/core/machine.c to get the CPU
> >> model name from CPU type names in mc->valid_cpu_types[].
> >
> > instead of per target hooks (which are atm mostly open-coded in several places)
> > how about adding generic handler for cases 2,4:
> > cpu_type_to_model(typename)
> > cpu_suffix = re'-*-cpu'
> > if (class_exists(typename - cpu_suffix))
> > return typename - cpu_suffix
> > else if (class_exists(typename))
> > return typename
> > explode
> >
> > that should work for translating valid_cpus typenames to cpumodel names
> > and once that in place cleanup all open-coded translations with it tree-wide
> >
> > you can find those easily by:
> > git grep _CPU_TYPE_SUFFIX
> > git grep query_cpu_definitions
> >
>
> Thanks for the advice. I think it's enough for now since the CPU type
> invalidation is currently done for arm/mips/riscv for now. On these
> targets, the CPU type name is always the combination of the CPU model
> name and suffix. I will add helper qemu/cpu.c::cpu_model_by_name()
cpu_model_from_type() would be describe what function does better.
> as you suggested. Note that, the suffix can be gained by ("-" CPU_RESOLVING_TYPE)
>
> Yes, the newly added helper cpu_model_by_name() needs to be applied
> to targets where query_cpu_definitions and cpu_list are defined.
>
> Thanks,
> Gavin
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/8] machine: Print supported CPU models instead of typenames
2023-08-29 9:03 ` Igor Mammedov
@ 2023-08-30 7:34 ` Gavin Shan
2023-08-31 9:02 ` Igor Mammedov
0 siblings, 1 reply; 21+ messages in thread
From: Gavin Shan @ 2023-08-30 7:34 UTC (permalink / raw)
To: Igor Mammedov
Cc: Richard Henderson, qemu-arm, qemu-devel, qemu-riscv,
peter.maydell, b.galvani, strahinja.p.jankovic, sundeep.lkml,
kfting, wuhaotsh, nieklinnenbank, rad, quic_llindhol,
marcin.juszkiewicz, eduardo, marcel.apfelbaum, philmd,
wangyanan55, laurent, vijai, palmer, alistair.francis, bin.meng,
liweiwei, dbarboza, zhiwei_liu, cohuck, pbonzini, shan.gavin,
qemu-ppc
Hi Igor,
On 8/29/23 19:03, Igor Mammedov wrote:
> On Tue, 29 Aug 2023 16:28:45 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>> On 8/29/23 00:46, Igor Mammedov wrote:
>>> On Mon, 31 Jul 2023 15:07:30 +1000
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>> On 7/27/23 19:00, Igor Mammedov wrote:
>>>>> On Thu, 27 Jul 2023 15:16:18 +1000
>>>>> Gavin Shan <gshan@redhat.com> wrote:
>>>>>
>>>>>> On 7/27/23 09:08, Richard Henderson wrote:
>>>>>>> On 7/25/23 17:32, Gavin Shan wrote:
>>>>>>>> -static const char *q800_machine_valid_cpu_types[] = {
>>>>>>>> +static const char * const q800_machine_valid_cpu_types[] = {
>>>>>>>> M68K_CPU_TYPE_NAME("m68040"),
>>>>>>>> NULL
>>>>>>>> };
>>>>>>>> +static const char * const q800_machine_valid_cpu_models[] = {
>>>>>>>> + "m68040",
>>>>>>>> + NULL
>>>>>>>> +};
>>>>>>>
>>>>>>> I really don't like this replication.
>>>>>>>
>>>>>>
>>>>>> Right, it's going to be lots of replications, but gives much flexibility.
>>>>>> There are 21 targets and we don't have fixed pattern for the mapping between
>>>>>> CPU model name and CPU typename. I'm summarizing the used patterns like below.
>>>>>>
>>>>>> 1 All CPU model names are mappinged to fixed CPU typename;
>>>>>
>>>>> plainly spelled it would be: cpu_model name ignored and
>>>>> a cpu type is returned anyways.
>>>>>
>>>>> I'd make this hard error right away, as "junk in => error out"
>>>>> it's clearly user error. I think we don't even have to follow
>>>>> deprecation process for that.
>>>>>
>>>>
>>>> Right, It's not expected behavior to map ambiguous CPU model names to
>>>> the fixed CPU typename.
>>>
>>> to be nice we can deprecate those and then later remove.
>>> (while deprecating make those targets accept typenames)
>>>
>>
>> Lets put it aside for now and revisit it later, so that we can focus on
>> the conversion from the CPU type name to the CPU model name for now.
>>
>>>>
>>>>>> 2 CPU model name is same to CPU typename;
>>>>>> 3 CPU model name is alias to CPU typename;
>>>>>> 4 CPU model name is prefix of CPU typename;
>>>>>
>>>>> and some more:
>>>>> 5. cpu model names aren't names at all sometimes, and some other
>>>>> CPU property is used. (ppc)
>>>>> This one I'd prefer to get rid of and ppc handling more consistent
>>>>> with other targets, which would need PPC folks to persuaded to drop
>>>>> PVR lookup.
>>>>>
>>>>
>>>> I put this into class 3, meaning the PVRs are regarded as aliases to CPU
>>>> typenames.
>>>
>>> with PPC using 'true' aliases, -cpu input is lost after it's translated into typename.
>>> (same for alpha)
>>>
>>> it also adds an extra fun with 'max' cpu model but that boils down to above statement.
>>> (same for
>>> * sh4
>>> * cris(in user mode only, but you are making sysemu extension, so it doesn't count)
>>> )
>>> For this class of aliases reverse translation won't yield the same
>>> result as used -cpu. The only option you have is to store -cpu cpu_model
>>> somewhere (use qemu_opts??, and then fetch it later to print in error message)
>>>
>>> x86 has 'aliases' as well, but in reality it creates distinct cpu types
>>> for each 'alias', so it's possible to do reverse translation.
>>>
>>
>> It's true that '-cpu input' gets lost in these cases. However, the CPU type
>> name instead of the CPU model name is printed in the error message when the
>> CPU type is validated in hw/core/machine.c::machine_run_board_init(). It looks
>> good to me to print the CPU type name instead of the model name there.
>
> It's the same confusing whether it's type or cpumodel it it doesn't match
> user provided value.
>
I tend to agree that it's misleading to print the CPU type name in the
error message in hw/core/machine.c::machine_run_board_init(), where the CPU
type is validated. qemu_opts may be too heavy for this. It eventually turns
to a machine's property if @machine_opts_dict is the best place to store
'-cpu input'. Besides, it doesn't fit for another case very well, where
current_machine->cpu_type = machine_class->default_cpu_type if '-cpu input'
isn't provided by user.
For simplicity, how about to add MachineState::cpu_model? It's initialized to
cpu_model_from_type(machine_class->default_cpu_type) in qemu_init(), or
g_strdump(model_pieces[0) in parse_cpu_option().
>> Another error message is printed when the CPU model specified in '-cpu input'
>> isn't valid. The CPU model has been printed and it looks good either.
>>
>> # qemu-system-aarch64 -M virt -cpu aaa
>> qemu-system-aarch64: unable to find CPU model 'aaa'
>>
>> Are there other cases I missed where we need to print the CPU model name, which
>> is specified by user through '-cpu input'?
>>
>>>>>>
>>>>>> Target Categories suffix-of-CPU-typename
>>>>>> -------------------------------------------------------
>>>>>> alpha -234 -alpha-cpu
>>>>>> arm ---4 -arm-cpu
>>>>>> avr -2--
>>>>>> cris --34 -cris-cpu
>>>>>> hexagon ---4 -hexagon-cpu
>>>>>> hppa 1---
>>>>>> i386 ---4 -i386-cpu
>>>>>> loongarch -2-4 -loongarch-cpu
>>>>>> m68k ---4 -m68k-cpu
>>>>>> microblaze 1---
>>>>>> mips ---4 -mips64-cpu -mips-cpu
>>>>>> nios2 1---
>>>>>> openrisc ---4 -or1k-cpu
>>>>>> ppc --34 -powerpc64-cpu -powerpc-cpu
>>>>>> riscv ---4 -riscv-cpu
>>>>>> rx -2-4 -rx-cpu
>>>>>> s390x ---4 -s390x-cpu
>>>>>> sh4 --34 -superh-cpu
>>>>>> sparc -2--
>>>
>>> it's case 4
>>>
>>
>> Yes.
>>
>>>>>> tricore ---4 -tricore-cpu
>>>>>> xtensa ---4 -xtensa-cpu
>>>>>>
>>>>>> There are several options as below. Please let me know which one or something
>>>>>> else is the best.
>>>>>>
>>>>>> (a) Keep what we have and use mc->valid_{cpu_types, cpu_models}[] to track
>>>>>> the valid CPU typenames and CPU model names.
>>>>>>
>>>>>> (b) Introduce CPUClass::model_name_by_typename(). Every target has their own
>>>>>> implementation to convert CPU typename to CPU model name. The CPU model name
>>>>>> is parsed from mc->valid_cpu_types[i].
>>>>>>
>>>>>> char *CPUClass::model_by_typename(const char *typename);
>>>>>>
>>>>>> (c) As we discussed before, use mc->valid_cpu_type_suffix and mc->valid_cpu_models
>>>>>> because the CPU type check is currently needed by target arm/m68k/riscv where we
>>>>>> do have fixed pattern to convert CPU model names to CPU typenames. The CPU typename
>>>>>> is comprised of CPU model name and suffix. However, it won't be working when the CPU
>>>>>> type check is required by other target where we have patterns other than this.
>>>>>
>>>>> none of above is really good, that's why I was objecting to introducing
>>>>> reverse type->name mapping. That ends up with increased amount junk,
>>>>> and it's not because your patches are bad, but because you are trying
>>>>> to deal with cpu model names (which is a historically evolved mess).
>>>>> The best from engineering POV would be replacing CPU models with
>>>>> type names.
>>>>>
>>>>> Even though it's a bit radical, I very much prefer replacing
>>>>> cpu_model names with '-cpu type'usage directly. Making it
>>>>> consistent with -device/other interfaces and coincidentally that
>>>>> obsoletes need in reverse name mapping.
>>>>>
>>>>> It's painful for end users who will need to change configs/scripts,
>>>>> but it's one time job. Additionally from QEMU pov, codebase
>>>>> will be less messy => more maintainable which benefits not only
>>>>> developers but end-users in the end.
>>>>>
>>>>
>>>> I have to clarify the type->model mapping has been existing since the
>>>> model->type mapping was introduced with the help of CPU_RESOLVING_TYPE.
>>>> I mean the logic has been existing since the existence of CPU_RESOLVING_TYPE,
>>>> even the code wasn't there.
>>>>
>>>> I'm not sure about the idea to switch to '-cpu <cpu-type-name>' since
>>>> it was rejected by Peter Maydell before. Hope Peter can double confirm
>>>> for this. For me, the shorter name is beneficial. For example, users
>>>> needn't to have '-cpu host-arm-cpu' for '-cpu host'.
>>>>
>>>>
>>>>> [rant:
>>>>> It's the same story repeating over and over, when it comes to
>>>>> changing QEMU CLI, which hits resistance wall. But with QEMU
>>>>> deprecation process we've changed CLI behavior before,
>>>>> despite of that world didn't cease to exist and users
>>>>> have adapted to new QEMU and arguably QEMU became a tiny
>>>>> bit more maintainable since we don't have to deal some
>>>>> legacy behavior.
>>>>> ]
>>>>>
>>>>
>>>> I need more context about 'deprecation process' here. My understanding
>>>> is both CPU typename and model name will be accepted for a fixed period
>>>> of time. However, a warning message will be given to indicate that the
>>>> model name will be obsoleted soon. Eventually, we switch to CPU typename
>>>> completely. Please correct me if there are anything wrong.
>>>
>>> yep, that's the gist of deprecation in this case.
>>>
>>
>> Ok. Thanks for your confirm.
>>
>>>>> Another idea back in the days was (as a compromise),
>>>>> 1. keep using keep valid_cpu_types
>>>>> 2. instead of introducing yet another way to do reverse mapping,
>>>>> clean/generalize/make it work everywhere list_cpus (which
>>>>> already does that mapping) and then use that to do your thing.
>>>>> It will have drawbacks you've listed above, but hopefully
>>>>> that will clean up and reuse existing list_cpus.
>>>>> (only this time, I'd build it around query-cpu-model-expansion,
>>>>> which output is used by generic list_cpus)
>>>>> [and here I'm asking to rewrite directly unrelated QEMU part yet again]
>>>>>
>>>>
>>>> I'm afraid that list_cpus() is hard to be reused. All available CPU model names
>>>> are listed by list_cpus(). mc->valid_cpu_types[] are just part of them and variable
>>>> on basis of boards. Generally speaking, we need a function to do reverse things
>>>> as to CPUClass::class_by_name(). So I would suggest to introduce CPUClass::model_from_type(),
>>>> as below. Could you please suggest if it sounds reasonable to you?
>>>>
>>>> - CPUClass::class_by_name() is modified to
>>>> char *CPUClass::model_to_type(const char *model)
>>>>
>>>> - char *CPUClass::type_to_model(const char *type)
>>>>
>>>> - CPUClass::type_to_model() is used in cpu_list() for every target when CPU
>>>> model name, fetched from CPU type name, is printed in xxx_cpu_list_entry()
>>>>
>>>> - CPUClass::type_to_model() is reused in hw/core/machine.c to get the CPU
>>>> model name from CPU type names in mc->valid_cpu_types[].
>>>
>>> instead of per target hooks (which are atm mostly open-coded in several places)
>>> how about adding generic handler for cases 2,4:
>>> cpu_type_to_model(typename)
>>> cpu_suffix = re'-*-cpu'
>>> if (class_exists(typename - cpu_suffix))
>>> return typename - cpu_suffix
>>> else if (class_exists(typename))
>>> return typename
>>> explode
>>>
>>> that should work for translating valid_cpus typenames to cpumodel names
>>> and once that in place cleanup all open-coded translations with it tree-wide
>>>
>>> you can find those easily by:
>>> git grep _CPU_TYPE_SUFFIX
>>> git grep query_cpu_definitions
>>>
>>
>> Thanks for the advice. I think it's enough for now since the CPU type
>> invalidation is currently done for arm/mips/riscv for now. On these
>> targets, the CPU type name is always the combination of the CPU model
>> name and suffix. I will add helper qemu/cpu.c::cpu_model_by_name()
>
> cpu_model_from_type() would be describe what function does better.
>
Agreed, thanks.
>> as you suggested. Note that, the suffix can be gained by ("-" CPU_RESOLVING_TYPE)
>>
>> Yes, the newly added helper cpu_model_by_name() needs to be applied
>> to targets where query_cpu_definitions and cpu_list are defined.
Thanks,
Gavin
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/8] machine: Print supported CPU models instead of typenames
2023-08-30 7:34 ` Gavin Shan
@ 2023-08-31 9:02 ` Igor Mammedov
0 siblings, 0 replies; 21+ messages in thread
From: Igor Mammedov @ 2023-08-31 9:02 UTC (permalink / raw)
To: Gavin Shan
Cc: Richard Henderson, qemu-arm, qemu-devel, qemu-riscv,
peter.maydell, b.galvani, strahinja.p.jankovic, sundeep.lkml,
kfting, wuhaotsh, nieklinnenbank, rad, quic_llindhol,
marcin.juszkiewicz, eduardo, marcel.apfelbaum, philmd,
wangyanan55, laurent, vijai, palmer, alistair.francis, bin.meng,
liweiwei, dbarboza, zhiwei_liu, cohuck, pbonzini, shan.gavin,
qemu-ppc
On Wed, 30 Aug 2023 17:34:12 +1000
Gavin Shan <gshan@redhat.com> wrote:
> Hi Igor,
>
> On 8/29/23 19:03, Igor Mammedov wrote:
> > On Tue, 29 Aug 2023 16:28:45 +1000
> > Gavin Shan <gshan@redhat.com> wrote:
> >> On 8/29/23 00:46, Igor Mammedov wrote:
> >>> On Mon, 31 Jul 2023 15:07:30 +1000
> >>> Gavin Shan <gshan@redhat.com> wrote:
> >>>> On 7/27/23 19:00, Igor Mammedov wrote:
> >>>>> On Thu, 27 Jul 2023 15:16:18 +1000
> >>>>> Gavin Shan <gshan@redhat.com> wrote:
> >>>>>
> >>>>>> On 7/27/23 09:08, Richard Henderson wrote:
> >>>>>>> On 7/25/23 17:32, Gavin Shan wrote:
> >>>>>>>> -static const char *q800_machine_valid_cpu_types[] = {
> >>>>>>>> +static const char * const q800_machine_valid_cpu_types[] = {
> >>>>>>>> M68K_CPU_TYPE_NAME("m68040"),
> >>>>>>>> NULL
> >>>>>>>> };
> >>>>>>>> +static const char * const q800_machine_valid_cpu_models[] = {
> >>>>>>>> + "m68040",
> >>>>>>>> + NULL
> >>>>>>>> +};
> >>>>>>>
> >>>>>>> I really don't like this replication.
> >>>>>>>
> >>>>>>
> >>>>>> Right, it's going to be lots of replications, but gives much flexibility.
> >>>>>> There are 21 targets and we don't have fixed pattern for the mapping between
> >>>>>> CPU model name and CPU typename. I'm summarizing the used patterns like below.
> >>>>>>
> >>>>>> 1 All CPU model names are mappinged to fixed CPU typename;
> >>>>>
> >>>>> plainly spelled it would be: cpu_model name ignored and
> >>>>> a cpu type is returned anyways.
> >>>>>
> >>>>> I'd make this hard error right away, as "junk in => error out"
> >>>>> it's clearly user error. I think we don't even have to follow
> >>>>> deprecation process for that.
> >>>>>
> >>>>
> >>>> Right, It's not expected behavior to map ambiguous CPU model names to
> >>>> the fixed CPU typename.
> >>>
> >>> to be nice we can deprecate those and then later remove.
> >>> (while deprecating make those targets accept typenames)
> >>>
> >>
> >> Lets put it aside for now and revisit it later, so that we can focus on
> >> the conversion from the CPU type name to the CPU model name for now.
> >>
> >>>>
> >>>>>> 2 CPU model name is same to CPU typename;
> >>>>>> 3 CPU model name is alias to CPU typename;
> >>>>>> 4 CPU model name is prefix of CPU typename;
> >>>>>
> >>>>> and some more:
> >>>>> 5. cpu model names aren't names at all sometimes, and some other
> >>>>> CPU property is used. (ppc)
> >>>>> This one I'd prefer to get rid of and ppc handling more consistent
> >>>>> with other targets, which would need PPC folks to persuaded to drop
> >>>>> PVR lookup.
> >>>>>
> >>>>
> >>>> I put this into class 3, meaning the PVRs are regarded as aliases to CPU
> >>>> typenames.
> >>>
> >>> with PPC using 'true' aliases, -cpu input is lost after it's translated into typename.
> >>> (same for alpha)
> >>>
> >>> it also adds an extra fun with 'max' cpu model but that boils down to above statement.
> >>> (same for
> >>> * sh4
> >>> * cris(in user mode only, but you are making sysemu extension, so it doesn't count)
> >>> )
> >>> For this class of aliases reverse translation won't yield the same
> >>> result as used -cpu. The only option you have is to store -cpu cpu_model
> >>> somewhere (use qemu_opts??, and then fetch it later to print in error message)
> >>>
> >>> x86 has 'aliases' as well, but in reality it creates distinct cpu types
> >>> for each 'alias', so it's possible to do reverse translation.
> >>>
> >>
> >> It's true that '-cpu input' gets lost in these cases. However, the CPU type
> >> name instead of the CPU model name is printed in the error message when the
> >> CPU type is validated in hw/core/machine.c::machine_run_board_init(). It looks
> >> good to me to print the CPU type name instead of the model name there.
> >
> > It's the same confusing whether it's type or cpumodel it it doesn't match
> > user provided value.
> >
>
> I tend to agree that it's misleading to print the CPU type name in the
> error message in hw/core/machine.c::machine_run_board_init(), where the CPU
> type is validated. qemu_opts may be too heavy for this. It eventually turns
> to a machine's property if @machine_opts_dict is the best place to store
> '-cpu input'. Besides, it doesn't fit for another case very well, where
> current_machine->cpu_type = machine_class->default_cpu_type if '-cpu input'
> isn't provided by user.
>
> For simplicity, how about to add MachineState::cpu_model? It's initialized to
> cpu_model_from_type(machine_class->default_cpu_type) in qemu_init(), or
> g_strdump(model_pieces[0) in parse_cpu_option().
I'd prefer not bringing cpu_model back to device models
(Machine in this case) after getting rid of it.
> >> Another error message is printed when the CPU model specified in '-cpu input'
> >> isn't valid. The CPU model has been printed and it looks good either.
> >>
> >> # qemu-system-aarch64 -M virt -cpu aaa
> >> qemu-system-aarch64: unable to find CPU model 'aaa'
> >>
> >> Are there other cases I missed where we need to print the CPU model name, which
> >> is specified by user through '-cpu input'?
> >>
> >>>>>>
> >>>>>> Target Categories suffix-of-CPU-typename
> >>>>>> -------------------------------------------------------
> >>>>>> alpha -234 -alpha-cpu
> >>>>>> arm ---4 -arm-cpu
> >>>>>> avr -2--
> >>>>>> cris --34 -cris-cpu
> >>>>>> hexagon ---4 -hexagon-cpu
> >>>>>> hppa 1---
> >>>>>> i386 ---4 -i386-cpu
> >>>>>> loongarch -2-4 -loongarch-cpu
> >>>>>> m68k ---4 -m68k-cpu
> >>>>>> microblaze 1---
> >>>>>> mips ---4 -mips64-cpu -mips-cpu
> >>>>>> nios2 1---
> >>>>>> openrisc ---4 -or1k-cpu
> >>>>>> ppc --34 -powerpc64-cpu -powerpc-cpu
> >>>>>> riscv ---4 -riscv-cpu
> >>>>>> rx -2-4 -rx-cpu
> >>>>>> s390x ---4 -s390x-cpu
> >>>>>> sh4 --34 -superh-cpu
> >>>>>> sparc -2--
> >>>
> >>> it's case 4
> >>>
> >>
> >> Yes.
> >>
> >>>>>> tricore ---4 -tricore-cpu
> >>>>>> xtensa ---4 -xtensa-cpu
> >>>>>>
> >>>>>> There are several options as below. Please let me know which one or something
> >>>>>> else is the best.
> >>>>>>
> >>>>>> (a) Keep what we have and use mc->valid_{cpu_types, cpu_models}[] to track
> >>>>>> the valid CPU typenames and CPU model names.
> >>>>>>
> >>>>>> (b) Introduce CPUClass::model_name_by_typename(). Every target has their own
> >>>>>> implementation to convert CPU typename to CPU model name. The CPU model name
> >>>>>> is parsed from mc->valid_cpu_types[i].
> >>>>>>
> >>>>>> char *CPUClass::model_by_typename(const char *typename);
> >>>>>>
> >>>>>> (c) As we discussed before, use mc->valid_cpu_type_suffix and mc->valid_cpu_models
> >>>>>> because the CPU type check is currently needed by target arm/m68k/riscv where we
> >>>>>> do have fixed pattern to convert CPU model names to CPU typenames. The CPU typename
> >>>>>> is comprised of CPU model name and suffix. However, it won't be working when the CPU
> >>>>>> type check is required by other target where we have patterns other than this.
> >>>>>
> >>>>> none of above is really good, that's why I was objecting to introducing
> >>>>> reverse type->name mapping. That ends up with increased amount junk,
> >>>>> and it's not because your patches are bad, but because you are trying
> >>>>> to deal with cpu model names (which is a historically evolved mess).
> >>>>> The best from engineering POV would be replacing CPU models with
> >>>>> type names.
> >>>>>
> >>>>> Even though it's a bit radical, I very much prefer replacing
> >>>>> cpu_model names with '-cpu type'usage directly. Making it
> >>>>> consistent with -device/other interfaces and coincidentally that
> >>>>> obsoletes need in reverse name mapping.
> >>>>>
> >>>>> It's painful for end users who will need to change configs/scripts,
> >>>>> but it's one time job. Additionally from QEMU pov, codebase
> >>>>> will be less messy => more maintainable which benefits not only
> >>>>> developers but end-users in the end.
> >>>>>
> >>>>
> >>>> I have to clarify the type->model mapping has been existing since the
> >>>> model->type mapping was introduced with the help of CPU_RESOLVING_TYPE.
> >>>> I mean the logic has been existing since the existence of CPU_RESOLVING_TYPE,
> >>>> even the code wasn't there.
> >>>>
> >>>> I'm not sure about the idea to switch to '-cpu <cpu-type-name>' since
> >>>> it was rejected by Peter Maydell before. Hope Peter can double confirm
> >>>> for this. For me, the shorter name is beneficial. For example, users
> >>>> needn't to have '-cpu host-arm-cpu' for '-cpu host'.
> >>>>
> >>>>
> >>>>> [rant:
> >>>>> It's the same story repeating over and over, when it comes to
> >>>>> changing QEMU CLI, which hits resistance wall. But with QEMU
> >>>>> deprecation process we've changed CLI behavior before,
> >>>>> despite of that world didn't cease to exist and users
> >>>>> have adapted to new QEMU and arguably QEMU became a tiny
> >>>>> bit more maintainable since we don't have to deal some
> >>>>> legacy behavior.
> >>>>> ]
> >>>>>
> >>>>
> >>>> I need more context about 'deprecation process' here. My understanding
> >>>> is both CPU typename and model name will be accepted for a fixed period
> >>>> of time. However, a warning message will be given to indicate that the
> >>>> model name will be obsoleted soon. Eventually, we switch to CPU typename
> >>>> completely. Please correct me if there are anything wrong.
> >>>
> >>> yep, that's the gist of deprecation in this case.
> >>>
> >>
> >> Ok. Thanks for your confirm.
> >>
> >>>>> Another idea back in the days was (as a compromise),
> >>>>> 1. keep using keep valid_cpu_types
> >>>>> 2. instead of introducing yet another way to do reverse mapping,
> >>>>> clean/generalize/make it work everywhere list_cpus (which
> >>>>> already does that mapping) and then use that to do your thing.
> >>>>> It will have drawbacks you've listed above, but hopefully
> >>>>> that will clean up and reuse existing list_cpus.
> >>>>> (only this time, I'd build it around query-cpu-model-expansion,
> >>>>> which output is used by generic list_cpus)
> >>>>> [and here I'm asking to rewrite directly unrelated QEMU part yet again]
> >>>>>
> >>>>
> >>>> I'm afraid that list_cpus() is hard to be reused. All available CPU model names
> >>>> are listed by list_cpus(). mc->valid_cpu_types[] are just part of them and variable
> >>>> on basis of boards. Generally speaking, we need a function to do reverse things
> >>>> as to CPUClass::class_by_name(). So I would suggest to introduce CPUClass::model_from_type(),
> >>>> as below. Could you please suggest if it sounds reasonable to you?
> >>>>
> >>>> - CPUClass::class_by_name() is modified to
> >>>> char *CPUClass::model_to_type(const char *model)
> >>>>
> >>>> - char *CPUClass::type_to_model(const char *type)
> >>>>
> >>>> - CPUClass::type_to_model() is used in cpu_list() for every target when CPU
> >>>> model name, fetched from CPU type name, is printed in xxx_cpu_list_entry()
> >>>>
> >>>> - CPUClass::type_to_model() is reused in hw/core/machine.c to get the CPU
> >>>> model name from CPU type names in mc->valid_cpu_types[].
> >>>
> >>> instead of per target hooks (which are atm mostly open-coded in several places)
> >>> how about adding generic handler for cases 2,4:
> >>> cpu_type_to_model(typename)
> >>> cpu_suffix = re'-*-cpu'
> >>> if (class_exists(typename - cpu_suffix))
> >>> return typename - cpu_suffix
> >>> else if (class_exists(typename))
> >>> return typename
> >>> explode
> >>>
> >>> that should work for translating valid_cpus typenames to cpumodel names
> >>> and once that in place cleanup all open-coded translations with it tree-wide
> >>>
> >>> you can find those easily by:
> >>> git grep _CPU_TYPE_SUFFIX
> >>> git grep query_cpu_definitions
> >>>
> >>
> >> Thanks for the advice. I think it's enough for now since the CPU type
> >> invalidation is currently done for arm/mips/riscv for now. On these
> >> targets, the CPU type name is always the combination of the CPU model
> >> name and suffix. I will add helper qemu/cpu.c::cpu_model_by_name()
> >
> > cpu_model_from_type() would be describe what function does better.
> >
>
> Agreed, thanks.
>
> >> as you suggested. Note that, the suffix can be gained by ("-" CPU_RESOLVING_TYPE)
> >>
> >> Yes, the newly added helper cpu_model_by_name() needs to be applied
> >> to targets where query_cpu_definitions and cpu_list are defined.
>
> Thanks,
> Gavin
>
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2023-08-31 9:03 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-26 0:31 [PATCH v2 0/8] machine: Unified CPU type check Gavin Shan
2023-07-26 0:31 ` [PATCH v2 1/8] machine: Use error handling when CPU type is checked Gavin Shan
2023-07-26 0:31 ` [PATCH v2 2/8] machine: Introduce helper is_cpu_type_supported() Gavin Shan
2023-07-26 23:03 ` Richard Henderson
2023-07-26 0:32 ` [PATCH v2 3/8] machine: Print supported CPU models instead of typenames Gavin Shan
2023-07-26 23:08 ` Richard Henderson
2023-07-27 5:16 ` Gavin Shan
2023-07-27 9:00 ` Igor Mammedov
2023-07-31 5:07 ` Gavin Shan
2023-08-28 14:46 ` Igor Mammedov
2023-08-29 6:28 ` Gavin Shan
2023-08-29 9:03 ` Igor Mammedov
2023-08-30 7:34 ` Gavin Shan
2023-08-31 9:02 ` Igor Mammedov
2023-07-27 14:27 ` Richard Henderson
2023-07-31 5:33 ` Gavin Shan
2023-07-26 0:32 ` [PATCH v2 4/8] hw/arm/virt: Check CPU type in machine_run_board_init() Gavin Shan
2023-07-26 0:32 ` [PATCH v2 5/8] hw/arm/virt: Unsupported host CPU model on TCG Gavin Shan
2023-07-26 0:32 ` [PATCH v2 6/8] hw/arm/sbsa-ref: Check CPU type in machine_run_board_init() Gavin Shan
2023-07-26 0:32 ` [PATCH v2 7/8] hw/arm: " Gavin Shan
2023-07-26 0:32 ` [PATCH v2 8/8] hw/riscv/shakti_c: " 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).