Hi Peter and Marcin,
On 7/13/23 21:52, Marcin Juszkiewicz wrote:
W dniu 13.07.2023 o 13:44, Peter Maydell pisze:
I see this isn't a change in this patch, but given that
what the user specifies is not "cortex-a8-arm-cpu" but
"cortex-a8", why do we include the "-arm-cpu" suffix in
the error messages? It's not valid syntax to say
"-cpu cortex-a8-arm-cpu", so it's a bit misleading...
Internally those cpu names are "max-{TYPE_ARM_CPU}" and similar for other
architectures.
I like the change but it (IMHO) needs to cut "-{TYPE_*_CPU}" string from names:
13:37 marcin@applejack:qemu$ ./build/aarch64-softmmu/qemu-system-aarch64 -M
virt -cpu cortex-r5
qemu-system-aarch64: Invalid CPU type: cortex-r5-arm-cpu
The valid types are: cortex-a7-arm-cpu, cortex-a15-arm-cpu, cortex-a35-arm-cpu,
cortex-a55-arm-cpu, cortex-a72-arm-cpu, cortex-a76-arm-cpu, a64fx-arm-cpu,
neoverse-n1-arm-cpu, neoverse-v1-arm-cpu, cortex-a53-arm-cpu,
cortex-a57-arm-cpu, host-arm-cpu, max-arm-cpu
13:37 marcin@applejack:qemu$ ./build/aarch64-softmmu/qemu-system-aarch64 -M
virt -cpu cortex-a57-arm-cpu
qemu-system-aarch64: unable to find CPU model 'cortex-a57-arm-cpu'
The suffix of CPU types are provided in hw/arm/virt.c::valid_cpu_types in
PATCH[2].
In the generic validation, the complete CPU type is used. The error message also
have complete CPU type there.
Peter and Marcin, how about to split the CPU types to two fields, as below? In
this
way, the complete CPU type will be used for validation and the 'internal' names
will
be used for the error messages.
struct MachineClass {
const char *valid_cpu_type_suffix;
const char **valid_cpu_types;
};
hw/arm/virt.c
-------------
static const char *valid_cpu_types[] = {
#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 virt_machine_class_init(ObjectClass *oc, void *data)
{
mc->valid_cpu_type_suffix = TYPE_ARM_CPU;
mc->valid_cpu_types = valid_cpu_types;
}
hw/core/machine.c
-----------------
static void validate_cpu_type(MachineState *machine)
{
ObjectClass *oc = object_class_by_name(machine->cpu_type), *ret = NULL;
CPUClass *cc = CPU_CLASS(oc);
char *cpu_type;
int i;
if (!machine->cpu_type || !machine_class->valid_cpu_types) {
goto out_no_check;
}
for (i = 0; machine_class->valid_cpu_types[i]; i++) {
/* The class name is the combination of prefix + suffix */
if (machine_class->valid_cpu_type_suffix) {
cpu_type = g_strdup_printf("%s-%s", machine_class->valid_cpu_types[i],
machine_class->valid_cpu_type_suffix);
} else {
cpu_type = g_strdup(machine_class->valid_cpu_types[i]);
}
ret = object_class_dynamic_cast(oc, cpu_type));
g_free(cpu_type);
if (ret) {
break;
}
}
if (!machine_class->valid_cpu_types[i]) {
/* The user-specified CPU type is invalid */
/**** the prefix is used in the error messages ********/
error_report("Invalid CPU type: %s", machine->cpu_type);
error_printf("The valid types are: %s",
machine_class->valid_cpu_types[0]);
for (i = 1; machine_class->valid_cpu_types[i]; i++) {
error_printf(", %s", machine_class->valid_cpu_types[i]);
}
error_printf("\n");
exit(1);
}
/* Check if CPU type is deprecated and warn if so */
out_no_check:
if (cc && cc->deprecation_note) {
warn_report("CPU model %s is deprecated -- %s",
machine->cpu_type, cc->deprecation_note);
}
}
I can change sbsa-ref to follow that change but let it be userfriendly.
Yes, sbsa-ref needs an extra patch to use the generic invalidation. I can add
one patch on the top for that in next revision if you agree, Marcin.
Thanks,
Gavin
