[PATCH v6 0/8] Unified CPU type check

Gavin Shan posted 8 patches 1 year ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20231120002724.986326-1-gshan@redhat.com
Maintainers: Peter Maydell <peter.maydell@linaro.org>, Beniamino Galvani <b.galvani@gmail.com>, Strahinja Jankovic <strahinja.p.jankovic@gmail.com>, Subbaraya Sundeep <sundeep.lkml@gmail.com>, Tyrone Ting <kfting@nuvoton.com>, Hao Wu <wuhaotsh@google.com>, Niek Linnenbank <nieklinnenbank@gmail.com>, Radoslaw Biernacki <rad@semihalf.com>, Leif Lindholm <quic_llindhol@quicinc.com>, Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>, Vijai Kumar K <vijai@behindbytes.com>, Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
There is a newer version of this series
hw/arm/bananapi_m2u.c   | 12 ++---
hw/arm/cubieboard.c     | 12 ++---
hw/arm/mps2-tz.c        | 26 ++++++++---
hw/arm/mps2.c           | 26 ++++++++---
hw/arm/msf2-som.c       | 12 ++---
hw/arm/musca.c          | 12 +++--
hw/arm/npcm7xx_boards.c | 12 +++--
hw/arm/orangepi.c       | 12 ++---
hw/arm/sbsa-ref.c       | 36 +++++----------
hw/arm/virt.c           | 60 ++++++++++---------------
hw/core/machine.c       | 97 +++++++++++++++++++++++++----------------
hw/riscv/shakti_c.c     | 13 +++---
12 files changed, 173 insertions(+), 157 deletions(-)
[PATCH v6 0/8] Unified CPU type check
Posted by Gavin Shan 1 year ago
This series bases on Phil's repository because the prepatory commits
have been queued to the branch.

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

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

PATCH[1-3] refactors the logic to validate CPU type in machine_run_board_init()
PATCH[4-8] validates the CPU type in machine_run_board_init() for the
           individual boards

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

Testing
=======

With the following command lines, the output messages are varied before
and after the series is applied.

  ./build/qemu-system-aarch64            \
  -accel tcg -machine virt,gic-version=3 \
  -cpu cortex-a8 -smp maxcpus=2,cpus=1

Before the series is applied:

  qemu-system-aarch64: mach-virt: CPU type cortex-a8-arm-cpu not supported

After the series is applied:

  qemu-system-aarch64: Invalid CPU type: cortex-a8-arm-cpu
  The valid models are: cortex-a7, cortex-a15, cortex-a35, cortex-a55,
                        cortex-a72, cortex-a76, a64fx, neoverse-n1,
                        neoverse-v1, cortex-a53, cortex-a57, max

Changelog
=========
v6:
  * Drop PATCH[v5 01-23], queued by Phil                         (Phil)
  * Clearer hint if only one CPU type is supported and have
    'const MachineState *' in is_cpu_type_supported()            (Phil)
  * Move valid_cpu_types[] to board's class_init() function      (Phil)
v5:
  * PATCH[v5 01] to remove CPU class 'ev67' for alpha            (Ricard/Igor)
  * PATCH[v5 02] to remove object_class_is_abstract() for hppa   (Gavin)
  * Don't move cpu_class_by_name()                               (Richard)
  * PATCH[v5 04] to remove 'oc == NULL' since the check has
    been covered in object_class_dynamic_cast()                  (Igor)
  * Introduce generic cpu_list(), shared by most of the targets  (Richard)
  * Use g_str_has_suffix and g_auto_free                         (Richard)
  * Collect r-bs from Igor and Richard                           (Gavin)
v4:
  * Integrate Philippe's patches where cpu_class_by_name()
    is consolidated and my duplicate code is dropped            (Philippe)
  * Simplified changelog and improvements                       (Thomas)
  * g_assert() on the return value from cpu_model_from_type()
    in is_cpu_type_supported()                                  (Philippe)
  * Collected r-bs from Philippe Mathieu-Daudé, Leif Lindholm,
    Bastian Koppelmann, Daniel Henrique Barboza, Cédric Le Goater,
    Gavin Shan                                                  (Gavin)
v3:
  * Generic helper cpu_model_from_type()                        (Igor)
  * Apply cpu_model_from_type() to the individual targets       (Igor)
  * Implement cpu_list() for the missed targets                 (Gavin)
  * Remove mc->valid_cpu_models                                 (Richard)
  * Separate patch to constify mc->validate_cpu_types           (Gavin)
v2:
  * Constify mc->valid_cpu_types                                (Richard)
  * Print the supported CPU models, instead of typenames        (Peter)
  * Misc improvements for the hleper to do the check            (Igor)
  * More patches to move the check                              (Marcin)

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

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

-- 
2.41.0


Re: [PATCH v6 0/8] Unified CPU type check
Posted by Marcin Juszkiewicz 1 year ago
W dniu 20.11.2023 o 01:27, Gavin Shan pisze:
> Testing
> =======
> 
> With the following command lines, the output messages are varied before
> and after the series is applied.
> 
>    ./build/qemu-system-aarch64            \
>    -accel tcg -machine virt,gic-version=3 \
>    -cpu cortex-a8 -smp maxcpus=2,cpus=1
> 
> Before the series is applied:
> 
>    qemu-system-aarch64: mach-virt: CPU type cortex-a8-arm-cpu not supported
> 
> After the series is applied:
> 
>    qemu-system-aarch64: Invalid CPU type: cortex-a8-arm-cpu
>    The valid models are: cortex-a7, cortex-a15, cortex-a35, cortex-a55,
>                          cortex-a72, cortex-a76, a64fx, neoverse-n1,
>                          neoverse-v1, cortex-a53, cortex-a57, max


$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu cortex-a53
qemu-system-aarch64: Invalid CPU type: cortex-a53
The valid types are: cortex-a57, cortex-a72, neoverse-n1, neoverse-v1, 
neoverse-n2, max

$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu sa1100
Unexpected error in object_property_find_err() at ../qom/object.c:1329:
qemu-system-aarch64: Property 'sa1100-arm-cpu.secure-memory' not found
Aborted (core dumped)


Similar with 'host' or 'pxa250' while QEMU/master does:

$ qemu-system-aarch64 -M sbsa-ref -cpu sa1100
qemu-system-aarch64: sbsa-ref: CPU type sa1100-arm-cpu not supported

Re: [PATCH v6 0/8] Unified CPU type check
Posted by Gavin Shan 1 year ago
On 11/21/23 05:42, Marcin Juszkiewicz wrote:
> W dniu 20.11.2023 o 01:27, Gavin Shan pisze:
>> Testing
>> =======
>>
>> With the following command lines, the output messages are varied before
>> and after the series is applied.
>>
>>    ./build/qemu-system-aarch64            \
>>    -accel tcg -machine virt,gic-version=3 \
>>    -cpu cortex-a8 -smp maxcpus=2,cpus=1
>>
>> Before the series is applied:
>>
>>    qemu-system-aarch64: mach-virt: CPU type cortex-a8-arm-cpu not supported
>>
>> After the series is applied:
>>
>>    qemu-system-aarch64: Invalid CPU type: cortex-a8-arm-cpu
>>    The valid models are: cortex-a7, cortex-a15, cortex-a35, cortex-a55,
>>                          cortex-a72, cortex-a76, a64fx, neoverse-n1,
>>                          neoverse-v1, cortex-a53, cortex-a57, max
> 
> 
> $ ./build/qemu-system-aarch64 -M sbsa-ref -cpu cortex-a53
> qemu-system-aarch64: Invalid CPU type: cortex-a53
> The valid types are: cortex-a57, cortex-a72, neoverse-n1, neoverse-v1, neoverse-n2, max
> 
> $ ./build/qemu-system-aarch64 -M sbsa-ref -cpu sa1100
> Unexpected error in object_property_find_err() at ../qom/object.c:1329:
> qemu-system-aarch64: Property 'sa1100-arm-cpu.secure-memory' not found
> Aborted (core dumped)
> 
> 
> Similar with 'host' or 'pxa250' while QEMU/master does:
> 
> $ qemu-system-aarch64 -M sbsa-ref -cpu sa1100
> qemu-system-aarch64: sbsa-ref: CPU type sa1100-arm-cpu not supported
> 

Good catch! I didn't realize that the syntax of error_propagate() has been changed
due to ERRP_GUARD() since ae7c80a7bd7 ("error: New macro ERRP_GUARD()"). Prior to
the commit, QEMU process is terminated immediately by error_propagate(). After the
commit, the termination is delayed until machine_run_board_init() returns. For the
later case, mc->init() is called and causing coredump on 'sbsa-ref' board.

A 'return' is needed after error_propagate() in machine_run_board_init() so that
mc->init() won't be called.

void machine_run_board_init(MachineState *machine, const char *mem_path, Error **errp)
{
     ERRP_GUARD();
     MachineClass *machine_class = MACHINE_GET_CLASS(machine);
     Error *local_err = NULL;
     :
     /* Check if the CPU type is supported */
     is_cpu_type_supported(machine, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;                               <<<<< A 'return' is needed here >>>>>
     }
     :
     accel_init_interfaces(ACCEL_GET_CLASS(machine->accelerator));
     machine_class->init(machine);
     phase_advance(PHASE_MACHINE_INITIALIZED);
}

With the 'return' added, everything looks good. I would let v6 floating for another 2-3
days before posting v7 to fix the issue.

[gshan@gshan q]$ ./build/qemu-system-aarch64 -M virt -cpu cortex-a8
qemu-system-aarch64: Invalid CPU type: cortex-a8
The valid types are: cortex-a7, cortex-a15, cortex-a35, cortex-a55, cortex-a72, cortex-a76, cortex-a710, a64fx, neoverse-n1, neoverse-v1, neoverse-n2, cortex-a53, cortex-a57, max

[gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu sa1100
qemu-system-aarch64: Invalid CPU type: sa1100
The valid types are: cortex-a57, cortex-a72, neoverse-n1, neoverse-v1, neoverse-n2, max

[gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu cortex-m0
qemu-system-aarch64: Invalid CPU type: cortex-m0
The valid types are: cortex-a57, cortex-a72, neoverse-n1, neoverse-v1, neoverse-n2, max

[gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu pxa250
qemu-system-aarch64: Invalid CPU type: pxa250
The valid types are: cortex-a57, cortex-a72, neoverse-n1, neoverse-v1, neoverse-n2, max

[gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu host
qemu-system-aarch64: unable to find CPU model 'host'

Thanks,
Gavin