Re: [PATCH v8 00/14] arm: rework id register storage

[Eric Auger <eric.auger@redhat.com>]

Hi Connie,

On 6/17/25 5:39 PM, Cornelia Huck wrote:
> Mostly addressed Peter's feedback:
> - make sure every inbetween stage compiles (also fixed some bonus issues)
I compile-tested each patch and it is fixed now.
> - try to make the scripts more robust, add a note the generated file,
>   and make sure to grab only registers we actually want
>   - I did a half-hearted attempt to use python instead of awk, but I'm
>     out of my depth, especially with the script that will be used for
>     the register fields in the cpu models series
However there are other checkpatch errors besides the one you reported, in
52873a54ad arm/cpu: Store aa64isar0/aa64zfr0 into the idregs arrays
ERROR: line over 90 characters
#388: FILE: target/arm/kvm.c:225:
+    return ARM64_SYS_REG((sysreg & CP_REG_ARM64_SYSREG_OP0_MASK) >>
CP_REG_ARM64_SYSREG_OP0_SHIFT,

ERROR: line over 90 characters
#389: FILE: target/arm/kvm.c:226:
+                         (sysreg & CP_REG_ARM64_SYSREG_OP1_MASK) >>
CP_REG_ARM64_SYSREG_OP1_SHIFT,

ERROR: line over 90 characters
#390: FILE: target/arm/kvm.c:227:
+                         (sysreg & CP_REG_ARM64_SYSREG_CRN_MASK) >>
CP_REG_ARM64_SYSREG_CRN_SHIFT,

ERROR: line over 90 characters
#391: FILE: target/arm/kvm.c:228:
+                         (sysreg & CP_REG_ARM64_SYSREG_CRM_MASK) >>
CP_REG_ARM64_SYSREG_CRM_SHIFT,

ERROR: line over 90 characters
#392: FILE: target/arm/kvm.c:229:
+                         (sysreg & CP_REG_ARM64_SYSREG_OP2_MASK) >>
CP_REG_ARM64_SYSREG_OP2_SHIFT);

WARNING: line over 80 characters
#396: FILE: target/arm/kvm.c:233:
+static int get_host_cpu_reg(int fd, ARMHostCPUFeatures *ahcf,
ARMIDRegisterIdx index)

and in


5f15ebdf3f arm/cpu: Add sysreg definitions in cpu-sysregs.h
ERROR: Macros with complex values should be enclosed in parenthesis
#56: FILE: target/arm/cpu-sysregs.h:21:
+#define DEF(NAME, OP0, OP1, CRN, CRM, OP2) NAME##_IDX,

ERROR: Macros with complex values should be enclosed in parenthesis
#64: FILE: target/arm/cpu-sysregs.h:29:
+#define DEF(NAME, OP0, OP1, CRN, CRM, OP2) \
+    SYS_##NAME = ENCODE_ID_REG(OP0, OP1, CRN, CRM, OP2),

ERROR: Macros with complex values should be enclosed in parenthesis
#203: FILE: target/arm/cpu64.c:40:
+#define DEF(NAME, OP0, OP1, CRN, CRM, OP2)      \
+    [NAME##_IDX] = SYS_##NAME,


Thanks

Eric

>
> Also available at
> https://gitlab.com/cohuck/qemu/-/commits/arm-rework-idreg-storage-v8
>
> <v7 cover letter>
> Some small fixes, including fixing up/adding SPDX identifiers, keeping the
> series bisectable, and updating MAINTAINERS (please check if that's ok.)
>
> <v6 cover letter>
> Just some small changes:
> - fixed up some botched conversions noted by Eric (thanks!)
> - rebased to current master
> - new patch with a small cleanup suggested by Eric
>
> <v5 cover letter>
> Just a quick respin to fix a missed conversion in hvf.c.
>
> <v4 cover letter>
> Next iteration of the id register patches; only small changes.
>
> Changed from v3:
> - added R-bs (thanks!)
> - added missing SPDX header
> - merged patch introducing accessors for kvm to the first user
> - skip over sysregs outside of the id register range when generating
>   register definitions again
>
> Also available at
> https://gitlab.com/cohuck/qemu/-/commits/arm-rework-idreg-storage-v4
>
> <v3 cover letter>
> Yet another update of the id register series, less changes this time
> around.
>
> Changed from v2:
> - changed generation of the various register defines via the "DEF"
>   magic suggested by Richard
> - some kvm-only code moved to kvm.c; some code potentially useful to
>   non-kvm code stayed out of there (the cpu model code will make use
>   of it, and that one should be extendable outside of kvm -- a
>   revised version of those patches is still in the works, but I'll be
>   off for a few days and rather wanted to get this one out first)
>
> Also available at
> https://gitlab.com/cohuck/qemu/-/commits/arm-rework-idreg-storage-v3
>
> <v2 cover letter>
>
> Changed from v1:
> - Noticed that we missed the hvf code. Converted, compiled, but not tested
>   as I'm lacking an environment for testing.
> - Hopefully incorporated most of the suggested changes -- if I missed
>   something, it was unintentional unless mentioned below.
>   - fixed repeated inclusion of definitions
>   - hopefully made macros more robust
>   - removed distinction between reading 32/64 values, which was mostly
>     adding churn for little value
>   - postponed generating property definitions to the cpu model patches,
>     where they are actually used
>   - juggled hunks and moved them to the right patches
>   - fixed some typos
> - rebased to a more recent code base
>
> NOT changed from v1:
> - definitions are still generated from the Linux sysregs file
>   - I still think updating the generated files on demand (so that we can
>     double check the result) is the right thing to do
>   - I'm open to changing the source of the definitions from the sysregs
>     file to the JSON definitions published by Arm; however, I first wanted
>     to get the code using it right -- we can switch out the code generating
>     the file to use a different source easily later on, and I'd also like
>     to steal parts of the script from Linux once integrated (which I think
>     hasn't happened yet?)
>
> <v1 cover letter>
>
> [Note: I've kept the cc list from the last round of cpu model patches;
> so if you're confused as to why you're cc:ed here, take it as a
> heads-up that a new cpu model series will come along soon]
>
> This patch series contains patches extracted from the larger cpu model
> series (RFC v2 last posted at
> https://lore.kernel.org/qemu-devel/20241206112213.88394-1-cohuck@redhat.com/)
> and aims at providing a base upon which we can continue with building
> support for cpu models, but which is hopefully already an improvement
> on its own.
>
> Main changes from the patches in that series include:
> - post-pone the changes to handle KVM writable ID registers for cpu models
>   (I have a series including that on top of this one)
> - change how we store the list of ID registers, and access them
>   basically, use an enum for indexing, and an enum doing encodings in a
>   pattern similar to cpregs
> - move some hunks to different patches
> - update the scripts to generate the register descriptions, and run
>   them against a recent Linux sysregs file
>
> What I've kept:
> - generating the register descriptions from the Linux sysregs file
>   I think that file is still our best bet to generate the descriptions
>   easily, and updating the definitions is a manual step that can be checked
>   for unintended changes
> - most of the hard work that Eric had been doing; all new bugs in there
>   are my own :)
>
> </v1 cover letter>
> </v2 cover letter>
> </v3 cover letter>
> </v4 cover letter>
> </v5 cover letter>
> </v6 cover letter>
> </v7 cover letter>
>
> Cornelia Huck (2):
>   arm/cpu: switch to a generated cpu-sysregs.h.inc
>   arm/kvm: use fd instead of fdarray[2]
>
> Eric Auger (12):
>   arm/cpu: Add sysreg definitions in cpu-sysregs.h
>   arm/cpu: Store aa64isar0/aa64zfr0 into the idregs arrays
>   arm/cpu: Store aa64isar1/2 into the idregs array
>   arm/cpu: Store aa64pfr0/1 into the idregs array
>   arm/cpu: Store aa64mmfr0-3 into the idregs array
>   arm/cpu: Store aa64dfr0/1 into the idregs array
>   arm/cpu: Store aa64smfr0 into the idregs array
>   arm/cpu: Store id_isar0-7 into the idregs array
>   arm/cpu: Store id_pfr0/1/2 into the idregs array
>   arm/cpu: Store id_dfr0/1 into the idregs array
>   arm/cpu: Store id_mmfr0-5 into the idregs array
>   arm/cpu: Add sysreg generation scripts
>
>  MAINTAINERS                            |   1 +
>  hw/intc/armv7m_nvic.c                  |  27 +-
>  scripts/arm-gen-cpu-sysregs-header.awk |  37 ++
>  scripts/update-aarch64-sysreg-code.sh  |  32 ++
>  target/arm/cpu-features.h              | 317 ++++++++---------
>  target/arm/cpu-sysregs.h               |  42 +++
>  target/arm/cpu-sysregs.h.inc           |  53 +++
>  target/arm/cpu.c                       | 111 +++---
>  target/arm/cpu.h                       |  80 +++--
>  target/arm/cpu64.c                     | 128 ++++---
>  target/arm/helper.c                    |  68 ++--
>  target/arm/hvf/hvf.c                   |  39 ++-
>  target/arm/internals.h                 |   6 +-
>  target/arm/kvm.c                       | 139 ++++----
>  target/arm/ptw.c                       |   6 +-
>  target/arm/tcg/cpu-v7m.c               | 174 +++++-----
>  target/arm/tcg/cpu32.c                 | 320 ++++++++---------
>  target/arm/tcg/cpu64.c                 | 459 +++++++++++++------------
>  18 files changed, 1114 insertions(+), 925 deletions(-)
>  create mode 100755 scripts/arm-gen-cpu-sysregs-header.awk
>  create mode 100755 scripts/update-aarch64-sysreg-code.sh
>  create mode 100644 target/arm/cpu-sysregs.h
>  create mode 100644 target/arm/cpu-sysregs.h.inc
>