[PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Gavin Shan]

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

[PATCH 1/3] machine: Factor CPU type invalidation out into helper

[Gavin Shan]

The CPU type invalidation logic in machine_run_board_init() is
independent enough. Lets factor it out into helper validate_cpu_type().
Since we're here, the relevant comments are improved a bit.

No functional change intended.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 hw/core/machine.c | 81 +++++++++++++++++++++++++----------------------
 1 file changed, 43 insertions(+), 38 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index f0d35c6401..68b866c762 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1349,12 +1349,52 @@ out:
     return r;
 }
 
+static void validate_cpu_type(MachineState *machine)
+{
+    MachineClass *machine_class = MACHINE_GET_CLASS(machine);
+    ObjectClass *oc = object_class_by_name(machine->cpu_type);
+    CPUClass *cc = CPU_CLASS(oc);
+    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 given by '-cpu' option.
+     */
+    if (!machine->cpu_type || !machine_class->valid_cpu_types) {
+        goto out_no_check;
+    }
+
+    for (i = 0; machine_class->valid_cpu_types[i]; i++) {
+        if (object_class_dynamic_cast(oc, machine_class->valid_cpu_types[i])) {
+            break;
+        }
+    }
+
+    if (!machine_class->valid_cpu_types[i]) {
+        /* The user-specified CPU type is invalid */
+        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);
+    }
+}
 
 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;
 
     /* This checkpoint is required by replay to separate prior clock
        reading from the other reads, because timer polling functions query
@@ -1405,42 +1445,7 @@ 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_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 */
-    cc = CPU_CLASS(oc);
-    if (cc && cc->deprecation_note) {
-        warn_report("CPU model %s is deprecated -- %s", machine->cpu_type,
-                    cc->deprecation_note);
-    }
+    validate_cpu_type(machine);
 
     if (machine->cgs) {
         /*
-- 
2.41.0

[PATCH 2/3] hw/arm/virt: Use generic CPU type invalidation

[Gavin Shan]

There is a generic CPU type invalidation in machine_run_board_init()
and we needn't a same and private invalidation. Set mc->valid_cpu_types
to use the generic CPU type invalidation.

No functional change intended.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 hw/arm/virt.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7d9dbc2663..43d7772ffd 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 *valid_cpu_types[] = {
 #ifdef CONFIG_TCG
     ARM_CPU_TYPE_NAME("cortex-a7"),
     ARM_CPU_TYPE_NAME("cortex-a15"),
@@ -219,20 +219,9 @@ 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 void create_randomness(MachineState *ms, const char *node)
 {
     struct {
@@ -2030,11 +2019,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 +2937,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
 #else
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("max");
 #endif
+    mc->valid_cpu_types = valid_cpu_types;
     mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
     mc->kvm_type = virt_kvm_type;
     assert(!mc->get_hotplug_handler);
-- 
2.41.0

[PATCH 3/3] hw/arm/virt: Support host CPU type only when KVM or HVF is configured

[Gavin Shan]

The CPU type 'host-arm-cpu' class won't be registered until KVM or
HVF is configured in target/arm/cpu64.c. Support the corresponding
CPU type only when KVM or HVF is configured.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 hw/arm/virt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 43d7772ffd..ad28634445 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -217,7 +217,9 @@ static const char *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
 };
-- 
2.41.0

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Peter Maydell]

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

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Marcin Juszkiewicz]

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.

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Peter Maydell]

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

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Gavin Shan]

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

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Gavin Shan]

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

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Marcin Juszkiewicz]

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.

Re: [PATCH 3/3] hw/arm/virt: Support host CPU type only when KVM or HVF is configured

[Cornelia Huck]

On Thu, Jul 13 2023, Gavin Shan <gshan@redhat.com> wrote:

> The CPU type 'host-arm-cpu' class won't be registered until KVM or
> HVF is configured in target/arm/cpu64.c. Support the corresponding
> CPU type only when KVM or HVF is configured.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  hw/arm/virt.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 43d7772ffd..ad28634445 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -217,7 +217,9 @@ static const char *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
>  };

Doesn't the check in parse_cpu_option() already catch the case where
the "host" cpu model isn't registered? I might be getting lost in the
code flow, though.

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Gavin Shan]

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

Re: [PATCH 3/3] hw/arm/virt: Support host CPU type only when KVM or HVF is configured

[Gavin Shan]

Hi Connie,

On 7/13/23 22:46, Cornelia Huck wrote:
> On Thu, Jul 13 2023, Gavin Shan <gshan@redhat.com> wrote:
> 
>> The CPU type 'host-arm-cpu' class won't be registered until KVM or
>> HVF is configured in target/arm/cpu64.c. Support the corresponding
>> CPU type only when KVM or HVF is configured.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   hw/arm/virt.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 43d7772ffd..ad28634445 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -217,7 +217,9 @@ static const char *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
>>   };
> 
> Doesn't the check in parse_cpu_option() already catch the case where
> the "host" cpu model isn't registered? I might be getting lost in the
> code flow, though.
> 

Right, it's guranteed that the needed CPU type (class) is registered by parse_cpu_option().
However, we have different story here. The CPU type invalidation intends to limit the CPU type
(class) into a range for the specific machine (board). Taking "cortex-a8-arm-cpu" as an example,
it's not expected by hw/arm/virt machines even it has been registered when we have CONFIG_TCG=y.
the list of supported CPU type (class) will be dumped by hw/core/machine.c::validate_cpu_type()
in PATCH[1], "host" is obviously invalid when we have CONFIG_KVM=n and CONFIG_HVF=n. We can't
tell user that "host" is supported, to confuse user.

Thanks,
Gavin

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Philippe Mathieu-Daudé]

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

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Richard Henderson]

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~

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Gavin Shan]

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

     

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Gavin Shan]

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

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Gavin Shan]

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

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Igor Mammedov]

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
> 

Re: [PATCH 2/3] hw/arm/virt: Use generic CPU type invalidation

[Igor Mammedov]

On Thu, 13 Jul 2023 15:45:01 +1000
Gavin Shan <gshan@redhat.com> wrote:

> There is a generic CPU type invalidation in machine_run_board_init()
                              ^^^^^
using that throughout the series is confusing to me.
Perhaps use original phrase 'valid cpu types' with appropriate rephrasing
would be better 

> and we needn't a same and private invalidation. Set mc->valid_cpu_types
> to use the generic CPU type invalidation.

I's suggest to replace commit message/subj with something like

arm/virt specific cpu_type_valid() is duplicate of ...
drop it and use ...

> No functional change intended.



> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  hw/arm/virt.c | 21 +++------------------
>  1 file changed, 3 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 7d9dbc2663..43d7772ffd 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 *valid_cpu_types[] = {
>  #ifdef CONFIG_TCG
>      ARM_CPU_TYPE_NAME("cortex-a7"),
>      ARM_CPU_TYPE_NAME("cortex-a15"),
> @@ -219,20 +219,9 @@ 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 void create_randomness(MachineState *ms, const char *node)
>  {
>      struct {
> @@ -2030,11 +2019,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 +2937,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>  #else
>      mc->default_cpu_type = ARM_CPU_TYPE_NAME("max");
>  #endif
> +    mc->valid_cpu_types = valid_cpu_types;
>      mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
>      mc->kvm_type = virt_kvm_type;
>      assert(!mc->get_hotplug_handler);

Re: [PATCH 1/3] machine: Factor CPU type invalidation out into helper

[Igor Mammedov]

On Thu, 13 Jul 2023 15:45:00 +1000
Gavin Shan <gshan@redhat.com> wrote:

> The CPU type invalidation logic in machine_run_board_init() is
> independent enough. Lets factor it out into helper validate_cpu_type().
> Since we're here, the relevant comments are improved a bit.
> 
> No functional change intended.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  hw/core/machine.c | 81 +++++++++++++++++++++++++----------------------
>  1 file changed, 43 insertions(+), 38 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index f0d35c6401..68b866c762 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1349,12 +1349,52 @@ out:
>      return r;
>  }
>  
> +static void validate_cpu_type(MachineState *machine)
s/validate_cpu_type/is_cpu_type_valid or better is_cpu_type_supported

Is it going to be reused elsewhere (otherwise I don't see much reason to move code around)?

> +{
> +    MachineClass *machine_class = MACHINE_GET_CLASS(machine);
> +    ObjectClass *oc = object_class_by_name(machine->cpu_type);
> +    CPUClass *cc = CPU_CLASS(oc);
> +    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 given by '-cpu' option.
> +     */
> +    if (!machine->cpu_type || !machine_class->valid_cpu_types) {
> +        goto out_no_check;
no goto-s please

> +    }
> +
> +    for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> +        if (object_class_dynamic_cast(oc, machine_class->valid_cpu_types[i])) {
> +            break;
> +        }
> +    }
> +
> +    if (!machine_class->valid_cpu_types[i]) {
> +        /* The user-specified CPU type is invalid */
> +        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);

since you are touching that,
turn it in errp handling, in separate patch 1st
and only then introduce your helper.

> +    }
> +
> +    /* 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);
> +    }
> +}
>  
>  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;
>  
>      /* This checkpoint is required by replay to separate prior clock
>         reading from the other reads, because timer polling functions query
> @@ -1405,42 +1445,7 @@ 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_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 */
> -    cc = CPU_CLASS(oc);
> -    if (cc && cc->deprecation_note) {
> -        warn_report("CPU model %s is deprecated -- %s", machine->cpu_type,
> -                    cc->deprecation_note);
> -    }
> +    validate_cpu_type(machine);
>  
>      if (machine->cgs) {
>          /*

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Peter Maydell]

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

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Igor Mammedov]

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
> 

Re: [PATCH 1/3] machine: Factor CPU type invalidation out into helper

[Gavin Shan]

Hi Igor,

On 7/14/23 22:07, Igor Mammedov wrote:
> On Thu, 13 Jul 2023 15:45:00 +1000
> Gavin Shan <gshan@redhat.com> wrote:
> 
>> The CPU type invalidation logic in machine_run_board_init() is
>> independent enough. Lets factor it out into helper validate_cpu_type().
>> Since we're here, the relevant comments are improved a bit.
>>
>> No functional change intended.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   hw/core/machine.c | 81 +++++++++++++++++++++++++----------------------
>>   1 file changed, 43 insertions(+), 38 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index f0d35c6401..68b866c762 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -1349,12 +1349,52 @@ out:
>>       return r;
>>   }
>>   
>> +static void validate_cpu_type(MachineState *machine)
> s/validate_cpu_type/is_cpu_type_valid or better is_cpu_type_supported
> 
> Is it going to be reused elsewhere (otherwise I don't see much reason to move code around)?
> 

The logic of checking if the CPU type is supported is independent enough. It's
the only reason why I factored it out into a standalone helper here. It has
been explained in the commit log. Lets have an individual helper for this if
you don't have strong taste. With it, machine_run_board_init() looks a bit more
clean.

I don't have strong opinion about the function name. Shall we return 'bool'
with is_cpu_type_supported()? Something like below. The 'bool' return value
is duplicate to 'local_err' in machine_run_board_init(). So I think the
function validate_cpu_type(machine, errp) looks good to me. Igor, could you
please help to confirm?

static bool is_cpu_type_supported(MachineState *machine, Error **errp)
{
     bool supported = true;

     :

     if (!machine_class->valid_cpu_types[i]) {
         error_setg(errp, "Invalid CPU type: %s", machine->cpu_type));
         error_append_hint(errp, "The valid types are: %s", model);
         for (i = 1; machine_class->valid_cpu_types[i]; i++) {
             error_append_hint(errp, ", %s", model);
         }
         error_append_hint(errp, "\n");

         supported = false;
     }

     :

     return supported;
}

void machine_run_board_init(MachineState *machine, const char *mem_path, Error **errp)
{
     Error *local_err = NULL;

     :

     /* These two conditions are duplicate to each other! */
     if (!is_cpu_type_supported(machine, &local_err) && local_err) {
         error_propagate(errp, local_err);
     }

     :
}

>> +{
>> +    MachineClass *machine_class = MACHINE_GET_CLASS(machine);
>> +    ObjectClass *oc = object_class_by_name(machine->cpu_type);
>> +    CPUClass *cc = CPU_CLASS(oc);
>> +    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 given by '-cpu' option.
>> +     */
>> +    if (!machine->cpu_type || !machine_class->valid_cpu_types) {
>> +        goto out_no_check;
> no goto-s please
> 

Ok. Will be dropped in next revision.

>> +    }
>> +
>> +    for (i = 0; machine_class->valid_cpu_types[i]; i++) {
>> +        if (object_class_dynamic_cast(oc, machine_class->valid_cpu_types[i])) {
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (!machine_class->valid_cpu_types[i]) {
>> +        /* The user-specified CPU type is invalid */
>> +        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);
> 
> since you are touching that,
> turn it in errp handling, in separate patch 1st
> and only then introduce your helper.
> 

Right, it's a good idea. I will have a preparatory patch for it where
the error messages will be accumulated to @local_err and finally propagate
it to @errp of machine_run_board_init().

>> +    }
>> +
>> +    /* 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);
>> +    }
>> +}
>>   
>>   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;
>>   
>>       /* This checkpoint is required by replay to separate prior clock
>>          reading from the other reads, because timer polling functions query
>> @@ -1405,42 +1445,7 @@ 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_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 */
>> -    cc = CPU_CLASS(oc);
>> -    if (cc && cc->deprecation_note) {
>> -        warn_report("CPU model %s is deprecated -- %s", machine->cpu_type,
>> -                    cc->deprecation_note);
>> -    }
>> +    validate_cpu_type(machine);
>>   
>>       if (machine->cgs) {
>>           /*

Thanks,
Gavin

Re: [PATCH 2/3] hw/arm/virt: Use generic CPU type invalidation

[Gavin Shan]

Hi Igor,

On 7/14/23 21:59, Igor Mammedov wrote:
> On Thu, 13 Jul 2023 15:45:01 +1000
> Gavin Shan <gshan@redhat.com> wrote:
> 
>> There is a generic CPU type invalidation in machine_run_board_init()
>                                ^^^^^
> using that throughout the series is confusing to me.
> Perhaps use original phrase 'valid cpu types' with appropriate rephrasing
> would be better
> 

Sure and sorry about the confusion. I will improve accordingly in next revision.

>> and we needn't a same and private invalidation. Set mc->valid_cpu_types
>> to use the generic CPU type invalidation.
> 
> I's suggest to replace commit message/subj with something like
> 
> arm/virt specific cpu_type_valid() is duplicate of ...
> drop it and use ...
> 

Sure, will do in next revision.

>> No functional change intended.
> 
> 
> 
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   hw/arm/virt.c | 21 +++------------------
>>   1 file changed, 3 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 7d9dbc2663..43d7772ffd 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 *valid_cpu_types[] = {
>>   #ifdef CONFIG_TCG
>>       ARM_CPU_TYPE_NAME("cortex-a7"),
>>       ARM_CPU_TYPE_NAME("cortex-a15"),
>> @@ -219,20 +219,9 @@ 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 void create_randomness(MachineState *ms, const char *node)
>>   {
>>       struct {
>> @@ -2030,11 +2019,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 +2937,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>>   #else
>>       mc->default_cpu_type = ARM_CPU_TYPE_NAME("max");
>>   #endif
>> +    mc->valid_cpu_types = valid_cpu_types;
>>       mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
>>       mc->kvm_type = virt_kvm_type;
>>       assert(!mc->get_hotplug_handler);

Thanks,
Gavin

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Gavin Shan]

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

Re: [PATCH 1/3] machine: Factor CPU type invalidation out into helper

[Igor Mammedov]

On Tue, 18 Jul 2023 16:11:42 +1000
Gavin Shan <gshan@redhat.com> wrote:

> Hi Igor,
> 
> On 7/14/23 22:07, Igor Mammedov wrote:
> > On Thu, 13 Jul 2023 15:45:00 +1000
> > Gavin Shan <gshan@redhat.com> wrote:
> >   
> >> The CPU type invalidation logic in machine_run_board_init() is
> >> independent enough. Lets factor it out into helper validate_cpu_type().
> >> Since we're here, the relevant comments are improved a bit.
> >>
> >> No functional change intended.
> >>
> >> Signed-off-by: Gavin Shan <gshan@redhat.com>
> >> ---
> >>   hw/core/machine.c | 81 +++++++++++++++++++++++++----------------------
> >>   1 file changed, 43 insertions(+), 38 deletions(-)
> >>
> >> diff --git a/hw/core/machine.c b/hw/core/machine.c
> >> index f0d35c6401..68b866c762 100644
> >> --- a/hw/core/machine.c
> >> +++ b/hw/core/machine.c
> >> @@ -1349,12 +1349,52 @@ out:
> >>       return r;
> >>   }
> >>   
> >> +static void validate_cpu_type(MachineState *machine)  
> > s/validate_cpu_type/is_cpu_type_valid or better is_cpu_type_supported
> > 
> > Is it going to be reused elsewhere (otherwise I don't see much reason to move code around)?
> >   
> 
> The logic of checking if the CPU type is supported is independent enough. It's
> the only reason why I factored it out into a standalone helper here. It has
> been explained in the commit log. Lets have an individual helper for this if
> you don't have strong taste. With it, machine_run_board_init() looks a bit more
> clean.
> 
> I don't have strong opinion about the function name. Shall we return 'bool'
> with is_cpu_type_supported()? Something like below. The 'bool' return value
> is duplicate to 'local_err' in machine_run_board_init(). So I think the
> function validate_cpu_type(machine, errp) looks good to me. Igor, could you
> please help to confirm?

I'd check errp and drop bool return, otherwise looks fine to me

> 
> static bool is_cpu_type_supported(MachineState *machine, Error **errp)
> {
>      bool supported = true;
> 
>      :
> 
>      if (!machine_class->valid_cpu_types[i]) {
>          error_setg(errp, "Invalid CPU type: %s", machine->cpu_type));
>          error_append_hint(errp, "The valid types are: %s", model);
>          for (i = 1; machine_class->valid_cpu_types[i]; i++) {
>              error_append_hint(errp, ", %s", model);
>          }
>          error_append_hint(errp, "\n");
> 
>          supported = false;
>      }
> 
>      :
> 
>      return supported;
> }
> 
> void machine_run_board_init(MachineState *machine, const char *mem_path, Error **errp)
> {
>      Error *local_err = NULL;
> 
>      :
> 
>      /* These two conditions are duplicate to each other! */
>      if (!is_cpu_type_supported(machine, &local_err) && local_err) {
>          error_propagate(errp, local_err);
>      }
> 
>      :
> }
> 
> >> +{
> >> +    MachineClass *machine_class = MACHINE_GET_CLASS(machine);
> >> +    ObjectClass *oc = object_class_by_name(machine->cpu_type);
> >> +    CPUClass *cc = CPU_CLASS(oc);
> >> +    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 given by '-cpu' option.
> >> +     */
> >> +    if (!machine->cpu_type || !machine_class->valid_cpu_types) {
> >> +        goto out_no_check;  
> > no goto-s please
> >   
> 
> Ok. Will be dropped in next revision.
> 
> >> +    }
> >> +
> >> +    for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> >> +        if (object_class_dynamic_cast(oc, machine_class->valid_cpu_types[i])) {
> >> +            break;
> >> +        }
> >> +    }
> >> +
> >> +    if (!machine_class->valid_cpu_types[i]) {
> >> +        /* The user-specified CPU type is invalid */
> >> +        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);  
> > 
> > since you are touching that,
> > turn it in errp handling, in separate patch 1st
> > and only then introduce your helper.
> >   
> 
> Right, it's a good idea. I will have a preparatory patch for it where
> the error messages will be accumulated to @local_err and finally propagate
> it to @errp of machine_run_board_init().
> 
> >> +    }
> >> +
> >> +    /* 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);
> >> +    }
> >> +}
> >>   
> >>   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;
> >>   
> >>       /* This checkpoint is required by replay to separate prior clock
> >>          reading from the other reads, because timer polling functions query
> >> @@ -1405,42 +1445,7 @@ 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_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 */
> >> -    cc = CPU_CLASS(oc);
> >> -    if (cc && cc->deprecation_note) {
> >> -        warn_report("CPU model %s is deprecated -- %s", machine->cpu_type,
> >> -                    cc->deprecation_note);
> >> -    }
> >> +    validate_cpu_type(machine);
> >>   
> >>       if (machine->cgs) {
> >>           /*  
> 
> Thanks,
> Gavin
> 

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Igor Mammedov]

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
> 

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Peter Maydell]

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

Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Igor Mammedov]

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
>