hw/arm/virt.c | 23 +++----------- hw/core/machine.c | 81 +++++++++++++++++++++++++---------------------- 2 files changed, 48 insertions(+), 56 deletions(-)
There is a generic CPU type invalidation in machine_run_board_init()
and we needn't a same and private invalidation for hw/arm/virt machines.
This series intends to use the generic CPU type invalidation on the
hw/arm/virt machines.
PATCH[1] factors the CPU type invalidation logic in machine_run_board_init()
to a helper validate_cpu_type().
PATCH[2] uses the generic CPU type invalidation for hw/arm/virt machines
PATCH[3] support "host-arm-cpu" CPU type only when KVM or HVF is visible
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 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, \
max-arm-cpu
Gavin Shan (3):
machine: Factor CPU type invalidation out into helper
hw/arm/virt: Use generic CPU type invalidation
hw/arm/virt: Support host CPU type only when KVM or HVF is configured
hw/arm/virt.c | 23 +++-----------
hw/core/machine.c | 81 +++++++++++++++++++++++++----------------------
2 files changed, 48 insertions(+), 56 deletions(-)
--
2.41.0
On Thu, 13 Jul 2023 at 06:45, Gavin Shan <gshan@redhat.com> wrote: > > There is a generic CPU type invalidation in machine_run_board_init() > and we needn't a same and private invalidation for hw/arm/virt machines. > This series intends to use the generic CPU type invalidation on the > hw/arm/virt machines. > > PATCH[1] factors the CPU type invalidation logic in machine_run_board_init() > to a helper validate_cpu_type(). > PATCH[2] uses the generic CPU type invalidation for hw/arm/virt machines > PATCH[3] support "host-arm-cpu" CPU type only when KVM or HVF is visible > > 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 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, \ > max-arm-cpu 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... -- PMM
Hi Peter, On 7/13/23 21:44, Peter Maydell wrote: > On Thu, 13 Jul 2023 at 06:45, Gavin Shan <gshan@redhat.com> wrote: >> >> There is a generic CPU type invalidation in machine_run_board_init() >> and we needn't a same and private invalidation for hw/arm/virt machines. >> This series intends to use the generic CPU type invalidation on the >> hw/arm/virt machines. >> >> PATCH[1] factors the CPU type invalidation logic in machine_run_board_init() >> to a helper validate_cpu_type(). >> PATCH[2] uses the generic CPU type invalidation for hw/arm/virt machines >> PATCH[3] support "host-arm-cpu" CPU type only when KVM or HVF is visible >> >> 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 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, \ >> max-arm-cpu > > 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... > Good point. Right, the complete CPU type names are provided by board (hw/arm/virt). The compelte CPU type names are used in hw/core/machine.c to search the object class. In the error messages in the same source file, the complete CPU type names are also used. Actually, we need 'internal' names like 'cortex-a8' to be shown in the error messages. For the solution, I've suggested to split the MachineClass::valid_cpu_types to two fields (valid_cpu_types and valid_cpu_type_suffix). Their combination is the complete CPU type name and 'valid_cpu_types[i]' corresponds to the 'internal' name, to be used in the error messages. Please take a look on that thread and reply to it. Thanks, Gavin
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'
I can change sbsa-ref to follow that change but let it be userfriendly.
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
On 7/13/23 13:34, Gavin Shan wrote:
> 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;
While you're changing this:
const char * const *valid_cpu_types;
> };
>
> hw/arm/virt.c
> -------------
>
> static const char *valid_cpu_types[] = {
So that you can then do
static const char * const valid_cpu_types[]
r~
Hi Richard,
On 7/14/23 05:27, Richard Henderson wrote:
> On 7/13/23 13:34, Gavin Shan wrote:
>> 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;
>
> While you're changing this:
>
> const char * const *valid_cpu_types;
>
yes, will do.
>> };
>>
>> hw/arm/virt.c
>> -------------
>>
>> static const char *valid_cpu_types[] = {
>
> So that you can then do
>
> static const char * const valid_cpu_types[]
>
yes, will do.
Thanks,
Gavin
On 13/7/23 14:34, Gavin Shan wrote:
> 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.
In some places (arm_cpu_list_entry, arm_cpu_add_definition) we use:
g_strndup(typename, strlen(typename) - strlen("-" TYPE_ARM_CPU))
Maybe extract as a helper? cpu_typename_name()? :)
Hi Philippe,
On 7/14/23 02:29, Philippe Mathieu-Daudé wrote:
> On 13/7/23 14:34, Gavin Shan wrote:
>> 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.
>
> In some places (arm_cpu_list_entry, arm_cpu_add_definition) we use:
>
> g_strndup(typename, strlen(typename) - strlen("-" TYPE_ARM_CPU))
>
> Maybe extract as a helper? cpu_typename_name()? :)
>
Yeah, it's definitely a good idea. The helper is needed by all architectures,
not ARM alone. The following CPU types don't have explicit definition of
XXXX_CPU_TYPE_SUFFIX. We need take "-" TYPE_CPU as the suffix.
target/microblaze/cpu.c TYPE_MICROBLAZE_CPU
target/hppa/cpu.c TYPE_HPPA_CPU
target/nios2/cpu.c TYPE_NIOS2_CPU
target/microblaze/cpu-qom.h:#define TYPE_MICROBLAZE_CPU "microblaze-cpu"
target/hppa/cpu-qom.h: #define TYPE_HPPA_CPU "hppa-cpu"
target/nios2/cpu.h: #define TYPE_NIOS2_CPU "nios2-cpu"
I think the function name can be cpu_model_name() since we have called it
as 'model' in cpu.c::parse_cpu_option(). Something like below. Please let
me know if you have more comments.
target/xxxx/cpu.h
-----------------
static inline char *cpu_model_name(const char *typename)
{
return g_strndup(typename, strlen(typename) - strlen(TYPE_XXX_CPU_SUFFIX));
}
Thanks,
Gavin
On 7/14/23 10:51, Gavin Shan wrote:
> On 7/14/23 02:29, Philippe Mathieu-Daudé wrote:
>> On 13/7/23 14:34, Gavin Shan wrote:
>>> 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.
>>
>> In some places (arm_cpu_list_entry, arm_cpu_add_definition) we use:
>>
>> g_strndup(typename, strlen(typename) - strlen("-" TYPE_ARM_CPU))
>>
>> Maybe extract as a helper? cpu_typename_name()? :)
>>
>
> Yeah, it's definitely a good idea. The helper is needed by all architectures,
> not ARM alone. The following CPU types don't have explicit definition of
> XXXX_CPU_TYPE_SUFFIX. We need take "-" TYPE_CPU as the suffix.
>
> target/microblaze/cpu.c TYPE_MICROBLAZE_CPU
> target/hppa/cpu.c TYPE_HPPA_CPU
> target/nios2/cpu.c TYPE_NIOS2_CPU
>
> target/microblaze/cpu-qom.h:#define TYPE_MICROBLAZE_CPU "microblaze-cpu"
> target/hppa/cpu-qom.h: #define TYPE_HPPA_CPU "hppa-cpu"
> target/nios2/cpu.h: #define TYPE_NIOS2_CPU "nios2-cpu"
>
> I think the function name can be cpu_model_name() since we have called it
> as 'model' in cpu.c::parse_cpu_option(). Something like below. Please let
> me know if you have more comments.
>
> target/xxxx/cpu.h
> -----------------
>
> static inline char *cpu_model_name(const char *typename)
> {
> return g_strndup(typename, strlen(typename) - strlen(TYPE_XXX_CPU_SUFFIX));
> }
>
I found the generic CPU type invalidation in hw/core/machine.c can't see functions
from target/xxx/, including cpu_model_name(). In order to call this function from
hw/core/machine.c, we need transit in cpu.c
include/exec/cpu-common.h
-------------------------
char *cpu_get_model_name(const char *name);
void list_cpus(void);
cpu.c
-----
char *cpu_get_model_name(const char *name)
{
return cpu_model_name(name);
}
With above hunk of changes, cpu_get_model_name() can be called in hw/core/machine.c,
to extract the CPU model name from the CPU type name.
Thanks,
Gavin
W dniu 13.07.2023 o 14:34, Gavin Shan pisze:
> 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...
>> I like the change but it (IMHO) needs to cut "-{TYPE_*_CPU}"
>> string from names:
> 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.
Note that it should probably propagate to all architectures handled in
QEMU, not only Arm ones. I do not know which machines have list of
supported cpu types like arm/virt or arm/sbsa-ref ones.
>> 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.
I am fine with it.
Hi Marcin,
On 7/13/23 22:44, Marcin Juszkiewicz wrote:
> W dniu 13.07.2023 o 14:34, Gavin Shan pisze:
>> 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...
>
>>> I like the change but it (IMHO) needs to cut "-{TYPE_*_CPU}"
>>> string from names:
>
>> 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.
>
> Note that it should probably propagate to all architectures handled in QEMU, not only Arm ones. I do not know which machines have list of supported cpu types like arm/virt or arm/sbsa-ref ones.
Right, there are more boards eligible for this generic CPU type invalidation. I will
cover all of them in next revision. Currently, the pattern of prefix + suffix, used
to split the CPU type name, can work well for all cases.
[gshan@gshan hw]$ git grep strcmp.*cpu_type
arm/bananapi_m2u.c: if (strcmp(machine->cpu_type, ARM_CPU_TYPE_NAME("cortex-a7")) != 0) {
arm/cubieboard.c: if (strcmp(machine->cpu_type, ARM_CPU_TYPE_NAME("cortex-a8")) != 0) {
arm/mps2-tz.c: if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) {
arm/mps2.c: if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) {
arm/msf2-som.c: if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) {
arm/musca.c: if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) {
arm/npcm7xx_boards.c: if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) {
arm/orangepi.c: if (strcmp(machine->cpu_type, ARM_CPU_TYPE_NAME("cortex-a7")) != 0) {
avr/boot.c: if (strcmp(elf_cpu, mcu_cpu_type)) { <<<<< needsn't CPU type validation
riscv/shakti_c.c: if (strcmp(mstate->cpu_type, TYPE_RISCV_CPU_SHAKTI_C) != 0) {
>>> 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.
>
> I am fine with it.
>
Thanks a lot for your confirm, Marcin.
Thanks,
Gavin
On Thu, 13 Jul 2023 at 12:52, Marcin Juszkiewicz
<marcin.juszkiewicz@linaro.org> 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.
Yes; my question is "why are we not using the user-facing
string rather than the internal type name?".
-- PMM
On Thu, 13 Jul 2023 12:59:55 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:
> On Thu, 13 Jul 2023 at 12:52, Marcin Juszkiewicz
> <marcin.juszkiewicz@linaro.org> 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.
>
> Yes; my question is "why are we not using the user-facing
> string rather than the internal type name?".
With other targets full CPU type name can also be valid
user-facing string. Namely we use it with -device/device_add
interface. Considering we would like to have CPU hotplug
on ARM as well, we shouldn't not outlaw full type name.
(QMP/monitor interface also mostly uses full type names)
Instead it might be better to consolidate on what has
been done on making CPU '-device' compatible and
allow to use full CPU type name with '-cpu' on arm machines.
Then later call suffix-less legacy => deprecate/drop it from
user-facing side including cleanup of all the infra we've
invented to keep mapping between cpu_model and typename.
With that gone, listing/restricting (supported) cpu types
can be done without extra processing and likely can be
done in one place for all targets/cpus instead of zoo
we have now.
(extra bonus: all error messages that include
CPU name will become consistent with the rest as well,
since only CPU typename is left around)
> -- PMM
>
On Fri, 14 Jul 2023 at 12:50, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Thu, 13 Jul 2023 12:59:55 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
> > On Thu, 13 Jul 2023 at 12:52, Marcin Juszkiewicz
> > <marcin.juszkiewicz@linaro.org> 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.
> >
> > Yes; my question is "why are we not using the user-facing
> > string rather than the internal type name?".
>
> With other targets full CPU type name can also be valid
> user-facing string. Namely we use it with -device/device_add
> interface. Considering we would like to have CPU hotplug
> on ARM as well, we shouldn't not outlaw full type name.
> (QMP/monitor interface also mostly uses full type names)
You don't seem to be able to use the full type name on
x86-64 either:
$ ./build/all/qemu-system-x86_64 -cpu pentium-x86_64-cpu
qemu-system-x86_64: unable to find CPU model 'pentium-x86_64-cpu'
and '-cpu help' does not list them with the suffix.
> Instead it might be better to consolidate on what has
> been done on making CPU '-device' compatible and
> allow to use full CPU type name with '-cpu' on arm machines.
>
> Then later call suffix-less legacy => deprecate/drop it from
> user-facing side including cleanup of all the infra we've
> invented to keep mapping between cpu_model and typename.
This seems to me like a worsening of the user interface,
and in practice there is not much likelihood of being
able to deprecate-and-drop the nicer user-facing names,
because they are baked into so many command lines and
scripts.
thanks
-- PMM
On Fri, 14 Jul 2023 13:56:00 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:
> On Fri, 14 Jul 2023 at 12:50, Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Thu, 13 Jul 2023 12:59:55 +0100
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > > On Thu, 13 Jul 2023 at 12:52, Marcin Juszkiewicz
> > > <marcin.juszkiewicz@linaro.org> 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.
> > >
> > > Yes; my question is "why are we not using the user-facing
> > > string rather than the internal type name?".
> >
> > With other targets full CPU type name can also be valid
> > user-facing string. Namely we use it with -device/device_add
> > interface. Considering we would like to have CPU hotplug
> > on ARM as well, we shouldn't not outlaw full type name.
> > (QMP/monitor interface also mostly uses full type names)
>
> You don't seem to be able to use the full type name on
> x86-64 either:
>
> $ ./build/all/qemu-system-x86_64 -cpu pentium-x86_64-cpu
> qemu-system-x86_64: unable to find CPU model 'pentium-x86_64-cpu'
that's because it also tied into old cpu_model resolving
routines, and I haven't added typename lookup the last
time I've touched it (it was out of topic change anyway).
but some targets do recognize typename, while some
do a lot more juggling with cpu_model (alpha/ppc),
and yet another class (garbage in => cpu type out).
With the last one we could just error,
while with alpha/ppc we could dumb it down to typenames
only.
> and '-cpu help' does not list them with the suffix.
both above points are fixable,
I can prepare PoC patches for that if there is
no opposition to the idea.
> > Instead it might be better to consolidate on what has
> > been done on making CPU '-device' compatible and
> > allow to use full CPU type name with '-cpu' on arm machines.
> >
> > Then later call suffix-less legacy => deprecate/drop it from
> > user-facing side including cleanup of all the infra we've
> > invented to keep mapping between cpu_model and typename.
>
> This seems to me like a worsening of the user interface,
> and in practice there is not much likelihood of being
> able to deprecate-and-drop the nicer user-facing names,
> because they are baked into so many command lines and
> scripts.
Nice names are subjective point, I suspect in a long run
once users switched to using longer names, they won't care much
about that either.
Also it's arguable if it is worsening UI or not.
I see using consolidated typenames across the board (incl. UI)
as a positive development.
As for scripts/CLI users out there, yes it would be disruptive
for a while but one can adapt to new naming (or use a wrapper
around QEMU that does suffix adding/model mapping as a crutch).
It weren't possible to drop anything before we introduced
deprecation process, but with it we can do it.
> thanks
> -- PMM
>
Hi Igor,
On 7/17/23 22:44, Igor Mammedov wrote:
> On Fri, 14 Jul 2023 13:56:00 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> On Fri, 14 Jul 2023 at 12:50, Igor Mammedov <imammedo@redhat.com> wrote:
>>>
>>> On Thu, 13 Jul 2023 12:59:55 +0100
>>> Peter Maydell <peter.maydell@linaro.org> wrote:
>>>
>>>> On Thu, 13 Jul 2023 at 12:52, Marcin Juszkiewicz
>>>> <marcin.juszkiewicz@linaro.org> 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.
>>>>
>>>> Yes; my question is "why are we not using the user-facing
>>>> string rather than the internal type name?".
>>>
>>> With other targets full CPU type name can also be valid
>>> user-facing string. Namely we use it with -device/device_add
>>> interface. Considering we would like to have CPU hotplug
>>> on ARM as well, we shouldn't not outlaw full type name.
>>> (QMP/monitor interface also mostly uses full type names)
>>
>> You don't seem to be able to use the full type name on
>> x86-64 either:
>>
>> $ ./build/all/qemu-system-x86_64 -cpu pentium-x86_64-cpu
>> qemu-system-x86_64: unable to find CPU model 'pentium-x86_64-cpu'
>
> that's because it also tied into old cpu_model resolving
> routines, and I haven't added typename lookup the last
> time I've touched it (it was out of topic change anyway).
>
> but some targets do recognize typename, while some
> do a lot more juggling with cpu_model (alpha/ppc),
> and yet another class (garbage in => cpu type out).
>
> With the last one we could just error,
> while with alpha/ppc we could dumb it down to typenames
> only.
>
Your summary here is correct to me. However, I don't quiet understand
the issue you're trying to resolve. As you mentioned, there are two
cases where the users need to specify CPU typename: (1) In the command
lines to start VM; (2) When CPU is hot added.
For (1), the list of all available CPU is provided by each individual
target. It's to say, each individual target is responsible for correlating
the name (typename, CPU model name, or whatever else). Each individual
target has its own rules for this correlation. Why do we bother to unify
the rules so that only the typename is allowed?
//
// The output can be directly used in the command lines to start VM.
// I don't see any problems we have. Note that the list of available
// CPU names is printed by cpu_list(), which is a target specific
// implementation.
//
# aarch64-softmmu/qemu-system-aarch64 -cpu help
Available CPUs:
a64fx
arm1026
arm1136
arm1136-r2
arm1176
arm11mpcore
arm926
arm946
cortex-a15
cortex-a35
cortex-a53
For (2) where CPU is hot added, the help option can also be used to dump
the available CPUs. Nothing went to wrong as I can see. The rule used here
to correlate names with CPUs is global: typename <-> CPU
//
// The printed CPU typenames can be taken as the driver directly
//
(qemu) device_add driver=?
:
CPU devices:
name "a64fx-arm-cpu"
name "cortex-a35-arm-cpu"
name "cortex-a53-arm-cpu"
name "cortex-a55-arm-cpu"
name "cortex-a57-arm-cpu"
name "cortex-a72-arm-cpu"
name "cortex-a76-arm-cpu"
name "max-arm-cpu"
name "neoverse-n1-arm-cpu"
>> and '-cpu help' does not list them with the suffix.
>
> both above points are fixable,
>
> I can prepare PoC patches for that if there is
> no opposition to the idea.
>
Please refer to above for the argument. If I'm correct, there is nothing
to be resolved or improved.
>>> Instead it might be better to consolidate on what has
>>> been done on making CPU '-device' compatible and
>>> allow to use full CPU type name with '-cpu' on arm machines.
>>>
>>> Then later call suffix-less legacy => deprecate/drop it from
>>> user-facing side including cleanup of all the infra we've
>>> invented to keep mapping between cpu_model and typename.
>>
>> This seems to me like a worsening of the user interface,
>> and in practice there is not much likelihood of being
>> able to deprecate-and-drop the nicer user-facing names,
>> because they are baked into so many command lines and
>> scripts.
> Nice names are subjective point, I suspect in a long run
> once users switched to using longer names, they won't care much
> about that either.
>
> Also it's arguable if it is worsening UI or not.
> I see using consolidated typenames across the board (incl. UI)
> as a positive development.
>
> As for scripts/CLI users out there, yes it would be disruptive
> for a while but one can adapt to new naming (or use a wrapper
> around QEMU that does suffix adding/model mapping as a crutch).
>
> It weren't possible to drop anything before we introduced
> deprecation process, but with it we can do it.
>
Thanks,
Gavin
On Tue, 18 Jul 2023 20:31:39 +1000
Gavin Shan <gshan@redhat.com> wrote:
> Hi Igor,
>
> On 7/17/23 22:44, Igor Mammedov wrote:
> > On Fri, 14 Jul 2023 13:56:00 +0100
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> >> On Fri, 14 Jul 2023 at 12:50, Igor Mammedov <imammedo@redhat.com> wrote:
> >>>
> >>> On Thu, 13 Jul 2023 12:59:55 +0100
> >>> Peter Maydell <peter.maydell@linaro.org> wrote:
> >>>
> >>>> On Thu, 13 Jul 2023 at 12:52, Marcin Juszkiewicz
> >>>> <marcin.juszkiewicz@linaro.org> 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.
> >>>>
> >>>> Yes; my question is "why are we not using the user-facing
> >>>> string rather than the internal type name?".
> >>>
> >>> With other targets full CPU type name can also be valid
> >>> user-facing string. Namely we use it with -device/device_add
> >>> interface. Considering we would like to have CPU hotplug
> >>> on ARM as well, we shouldn't not outlaw full type name.
> >>> (QMP/monitor interface also mostly uses full type names)
> >>
> >> You don't seem to be able to use the full type name on
> >> x86-64 either:
> >>
> >> $ ./build/all/qemu-system-x86_64 -cpu pentium-x86_64-cpu
> >> qemu-system-x86_64: unable to find CPU model 'pentium-x86_64-cpu'
> >
> > that's because it also tied into old cpu_model resolving
> > routines, and I haven't added typename lookup the last
> > time I've touched it (it was out of topic change anyway).
> >
> > but some targets do recognize typename, while some
> > do a lot more juggling with cpu_model (alpha/ppc),
> > and yet another class (garbage in => cpu type out).
> >
> > With the last one we could just error,
> > while with alpha/ppc we could dumb it down to typenames
> > only.
> >
>
> Your summary here is correct to me. However, I don't quiet understand
> the issue you're trying to resolve. As you mentioned, there are two
> cases where the users need to specify CPU typename: (1) In the command
> lines to start VM; (2) When CPU is hot added.
>
> For (1), the list of all available CPU is provided by each individual
> target. It's to say, each individual target is responsible for correlating
> the name (typename, CPU model name, or whatever else). Each individual
> target has its own rules for this correlation. Why do we bother to unify
> the rules so that only the typename is allowed?
I've seen others asking why you print type name instead of shorter cpu-model
used on CLI. To do that would make you write a patch to implement reverse mapping.
In some cases it's simple, in others plain impossible unless you can get
access to -cpu foo stored somewhere.
What I don't particularity like about adding reverse type->cpu_model mapping,
is that it would complicate code to carter to QEMU's interface inconsistencies.
And if you do it easy way (instead of fixing every target) touching only ARM,
it will be spotty at best and just add to technical debt elsewhere ->
more inconsistencies.
What I'm proposing is for you to keep printing type names.
So if others won't object to type names I'm more or less fine with your
current approach.
Instead of adding type->cpu_model mapping (it's not the first time
this particular question had arisen - there were similar patches before
on qemu-devel), get rid of shortened cpu_model in user interface (-cpu)
altogether and use CPU type name there.
Beside making UI consistent across QEMU it will also simplify
QEMU codebase (cpu-model -> type resolving machinery + all legacy junk
that was accumulated for years QEMU has existed).
> // The output can be directly used in the command lines to start VM.
> // I don't see any problems we have. Note that the list of available
> // CPU names is printed by cpu_list(), which is a target specific
> // implementation.
> //
> # aarch64-softmmu/qemu-system-aarch64 -cpu help
> Available CPUs:
> a64fx
> arm1026
> arm1136
> arm1136-r2
> arm1176
> arm11mpcore
> arm926
> arm946
> cortex-a15
> cortex-a35
> cortex-a53
>
> For (2) where CPU is hot added, the help option can also be used to dump
> the available CPUs. Nothing went to wrong as I can see. The rule used here
> to correlate names with CPUs is global: typename <-> CPU
>
> //
> // The printed CPU typenames can be taken as the driver directly
> //
> (qemu) device_add driver=?
> :
> CPU devices:
> name "a64fx-arm-cpu"
> name "cortex-a35-arm-cpu"
> name "cortex-a53-arm-cpu"
> name "cortex-a55-arm-cpu"
> name "cortex-a57-arm-cpu"
> name "cortex-a72-arm-cpu"
> name "cortex-a76-arm-cpu"
> name "max-arm-cpu"
> name "neoverse-n1-arm-cpu"
>
> >> and '-cpu help' does not list them with the suffix.
> >
> > both above points are fixable,
> >
> > I can prepare PoC patches for that if there is
> > no opposition to the idea.
> >
>
> Please refer to above for the argument. If I'm correct, there is nothing
> to be resolved or improved.
>
> >>> Instead it might be better to consolidate on what has
> >>> been done on making CPU '-device' compatible and
> >>> allow to use full CPU type name with '-cpu' on arm machines.
> >>>
> >>> Then later call suffix-less legacy => deprecate/drop it from
> >>> user-facing side including cleanup of all the infra we've
> >>> invented to keep mapping between cpu_model and typename.
> >>
> >> This seems to me like a worsening of the user interface,
> >> and in practice there is not much likelihood of being
> >> able to deprecate-and-drop the nicer user-facing names,
> >> because they are baked into so many command lines and
> >> scripts.
> > Nice names are subjective point, I suspect in a long run
> > once users switched to using longer names, they won't care much
> > about that either.
> >
> > Also it's arguable if it is worsening UI or not.
> > I see using consolidated typenames across the board (incl. UI)
> > as a positive development.
> >
> > As for scripts/CLI users out there, yes it would be disruptive
> > for a while but one can adapt to new naming (or use a wrapper
> > around QEMU that does suffix adding/model mapping as a crutch).
> >
> > It weren't possible to drop anything before we introduced
> > deprecation process, but with it we can do it.
> >
>
> Thanks,
> Gavin
>
On Mon, 24 Jul 2023 at 16:06, Igor Mammedov <imammedo@redhat.com> wrote:
> I've seen others asking why you print type name instead of shorter cpu-model
> used on CLI. To do that would make you write a patch to implement reverse mapping.
> In some cases it's simple, in others plain impossible unless you can get
> access to -cpu foo stored somewhere.
>
> What I don't particularity like about adding reverse type->cpu_model mapping,
> is that it would complicate code to carter to QEMU's interface inconsistencies.
> And if you do it easy way (instead of fixing every target) touching only ARM,
> it will be spotty at best and just add to technical debt elsewhere ->
> more inconsistencies.
>
> What I'm proposing is for you to keep printing type names.
> So if others won't object to type names I'm more or less fine with your
> current approach.
I do object to type names, because the UI for choosing
a CPU ("-cpu whatever") does not take type names, it
takes CPU names. The QOM type names that those end up
being under the hood are a detail of QEMU's implementation
that we shouldn't expose to users in the help messages.
> Instead of adding type->cpu_model mapping (it's not the first time
> this particular question had arisen - there were similar patches before
> on qemu-devel), get rid of shortened cpu_model in user interface (-cpu)
> altogether and use CPU type name there.
I also think we should not do this, because it will break
a ton of existing command lines, and it's not clear it
has any benefit to users.
thanks
-- PMM
On Mon, 24 Jul 2023 16:14:22 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:
> On Mon, 24 Jul 2023 at 16:06, Igor Mammedov <imammedo@redhat.com> wrote:
> > I've seen others asking why you print type name instead of shorter cpu-model
> > used on CLI. To do that would make you write a patch to implement reverse mapping.
> > In some cases it's simple, in others plain impossible unless you can get
> > access to -cpu foo stored somewhere.
> >
> > What I don't particularity like about adding reverse type->cpu_model mapping,
> > is that it would complicate code to carter to QEMU's interface inconsistencies.
> > And if you do it easy way (instead of fixing every target) touching only ARM,
> > it will be spotty at best and just add to technical debt elsewhere ->
> > more inconsistencies.
> >
> > What I'm proposing is for you to keep printing type names.
> > So if others won't object to type names I'm more or less fine with your
> > current approach.
>
> I do object to type names, because the UI for choosing
> a CPU ("-cpu whatever") does not take type names, it
> takes CPU names. The QOM type names that those end up
> being under the hood are a detail of QEMU's implementation
> that we shouldn't expose to users in the help messages.
those are exposed to users as a part of -device interface
which uses typenames for any device and based on that interface
in some monitor/qmp commands
> > Instead of adding type->cpu_model mapping (it's not the first time
> > this particular question had arisen - there were similar patches before
> > on qemu-devel), get rid of shortened cpu_model in user interface (-cpu)
> > altogether and use CPU type name there.
>
> I also think we should not do this, because it will break
> a ton of existing command lines, and it's not clear it
> has any benefit to users.
Yes it will break existing scripts but that for users to fix up once
(not to mention that could be worked around with a wrapper,
QEMU can even supply that for existing cpu types).
(so along with deprecation, we can provide a warning +
a wrapper to use, and after deprecation make it a hard error
but keep wrapper around for those that do not wish to use typenames)
Consistent naming across UI and consistent name -> type handling
would be beneficial to users in the long run (especially ones
that have to deal with both interfaces so that they won't
have to bother which use where).
> thanks
> -- PMM
>
© 2016 - 2024 Red Hat, Inc.