Hoist the computation of key into the caller, where
state is a known constant.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 4a109a113d..a5195e296d 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7424,26 +7424,13 @@ void register_cp_regs_for_features(ARMCPU *cpu)
 static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
                                    CPState state, CPSecureState secstate,
                                    int cp, int crm, int opc1, int opc2,
-                                   const char *name)
+                                   const char *name, uint32_t key)
 {
     CPUARMState *env = &cpu->env;
-    uint32_t key;
     ARMCPRegInfo *r2;
-    bool is64 = r->type & ARM_CP_64BIT;
     bool ns = secstate & ARM_CP_SECSTATE_NS;
     size_t name_len;
 
-    switch (state) {
-    case ARM_CP_STATE_AA32:
-        key = ENCODE_CP_REG(cp, is64, ns, r->crn, crm, opc1, opc2);
-        break;
-    case ARM_CP_STATE_AA64:
-        key = ENCODE_AA64_CP_REG(r->opc0, opc1, r->crn, crm, opc2);
-        break;
-    default:
-        g_assert_not_reached();
-    }
-
     /* Overriding of an existing definition must be explicitly requested. */
     if (!(r->type & ARM_CP_OVERRIDE)) {
         const ARMCPRegInfo *oldreg = get_arm_cp_reginfo(cpu->cp_regs, key);
@@ -7548,22 +7535,28 @@ static void add_cpreg_to_hashtable_aa32(ARMCPU *cpu, const ARMCPRegInfo *r,
      * (same for secure and non-secure world) or banked.
      */
     char *name;
+    bool is64 = r->type & ARM_CP_64BIT;
+    uint32_t key = ENCODE_CP_REG(cp, is64, 0, r->crn, crm, opc1, opc2);
 
     assert(!(r->type & ARM_CP_ADD_TLBI_NXS)); /* aa64 only */
 
     switch (r->secure) {
-    case ARM_CP_SECSTATE_S:
     case ARM_CP_SECSTATE_NS:
+        key |= CP_REG_AA32_NS_MASK;
+        /* fall through */
+    case ARM_CP_SECSTATE_S:
         add_cpreg_to_hashtable(cpu, r, ARM_CP_STATE_AA32, r->secure,
-                               cp, crm, opc1, opc2, r->name);
+                               cp, crm, opc1, opc2, r->name, key);
         break;
     case ARM_CP_SECSTATE_BOTH:
         name = g_strdup_printf("%s_S", r->name);
         add_cpreg_to_hashtable(cpu, r, ARM_CP_STATE_AA32, ARM_CP_SECSTATE_S,
-                               cp, crm, opc1, opc2, name);
+                               cp, crm, opc1, opc2, name, key);
         g_free(name);
+
+        key |= CP_REG_AA32_NS_MASK;
         add_cpreg_to_hashtable(cpu, r, ARM_CP_STATE_AA32, ARM_CP_SECSTATE_NS,
-                               cp, crm, opc1, opc2, r->name);
+                               cp, crm, opc1, opc2, r->name, key);
         break;
     default:
         g_assert_not_reached();
@@ -7573,6 +7566,8 @@ static void add_cpreg_to_hashtable_aa32(ARMCPU *cpu, const ARMCPRegInfo *r,
 static void add_cpreg_to_hashtable_aa64(ARMCPU *cpu, const ARMCPRegInfo *r,
                                         int crm, int opc1, int opc2)
 {
+    uint32_t key = ENCODE_AA64_CP_REG(r->opc0, opc1, r->crn, crm, opc2);
+
     if ((r->type & ARM_CP_ADD_TLBI_NXS) &&
         cpu_isar_feature(aa64_xs, cpu)) {
         /*
@@ -7587,18 +7582,22 @@ static void add_cpreg_to_hashtable_aa64(ARMCPU *cpu, const ARMCPRegInfo *r,
          */
         ARMCPRegInfo nxs_ri = *r;
         g_autofree char *name = g_strdup_printf("%sNXS", r->name);
+        uint32_t nxs_key;
 
         assert(nxs_ri.crn < 0xf);
         nxs_ri.crn++;
+        nxs_key = key + (1 << CP_REG_ARM64_SYSREG_CRN_SHIFT);
         if (nxs_ri.fgt) {
             nxs_ri.fgt |= R_FGT_NXS_MASK;
         }
+
         add_cpreg_to_hashtable(cpu, &nxs_ri, ARM_CP_STATE_AA64,
-                               ARM_CP_SECSTATE_NS, 0, crm, opc1, opc2, name);
+                               ARM_CP_SECSTATE_NS, 0, crm, opc1, opc2,
+                               name, nxs_key);
     }
 
     add_cpreg_to_hashtable(cpu, r, ARM_CP_STATE_AA64, ARM_CP_SECSTATE_NS,
-                           0, crm, opc1, opc2, r->name);
+                           0, crm, opc1, opc2, r->name, key);
 }
 
 void define_one_arm_cp_reg(ARMCPU *cpu, const ARMCPRegInfo *r)
-- 
2.43.0

On 16/9/25 16:22, Richard Henderson wrote:
> Hoist the computation of key into the caller, where
> state is a known constant.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/arm/helper.c | 39 +++++++++++++++++++--------------------
>   1 file changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 4a109a113d..a5195e296d 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -7424,26 +7424,13 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>   static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
>                                      CPState state, CPSecureState secstate,
>                                      int cp, int crm, int opc1, int opc2,
> -                                   const char *name)
> +                                   const char *name, uint32_t key)
>   {
>       CPUARMState *env = &cpu->env;
> -    uint32_t key;
>       ARMCPRegInfo *r2;
> -    bool is64 = r->type & ARM_CP_64BIT;
>       bool ns = secstate & ARM_CP_SECSTATE_NS;
>       size_t name_len;
>   
> -    switch (state) {
> -    case ARM_CP_STATE_AA32:
> -        key = ENCODE_CP_REG(cp, is64, ns, r->crn, crm, opc1, opc2);
> -        break;
> -    case ARM_CP_STATE_AA64:
> -        key = ENCODE_AA64_CP_REG(r->opc0, opc1, r->crn, crm, opc2);
> -        break;
> -    default:
> -        g_assert_not_reached();
> -    }
> -
>       /* Overriding of an existing definition must be explicitly requested. */
>       if (!(r->type & ARM_CP_OVERRIDE)) {
>           const ARMCPRegInfo *oldreg = get_arm_cp_reginfo(cpu->cp_regs, key);
> @@ -7548,22 +7535,28 @@ static void add_cpreg_to_hashtable_aa32(ARMCPU *cpu, const ARMCPRegInfo *r,
>        * (same for secure and non-secure world) or banked.
>        */
>       char *name;
> +    bool is64 = r->type & ARM_CP_64BIT;
> +    uint32_t key = ENCODE_CP_REG(cp, is64, 0, r->crn, crm, opc1, opc2);
>   
>       assert(!(r->type & ARM_CP_ADD_TLBI_NXS)); /* aa64 only */
>   
>       switch (r->secure) {
> -    case ARM_CP_SECSTATE_S:
>       case ARM_CP_SECSTATE_NS:
> +        key |= CP_REG_AA32_NS_MASK;
> +        /* fall through */
> +    case ARM_CP_SECSTATE_S:
>           add_cpreg_to_hashtable(cpu, r, ARM_CP_STATE_AA32, r->secure,
> -                               cp, crm, opc1, opc2, r->name);
> +                               cp, crm, opc1, opc2, r->name, key);
>           break;
>       case ARM_CP_SECSTATE_BOTH:
>           name = g_strdup_printf("%s_S", r->name);
>           add_cpreg_to_hashtable(cpu, r, ARM_CP_STATE_AA32, ARM_CP_SECSTATE_S,
> -                               cp, crm, opc1, opc2, name);
> +                               cp, crm, opc1, opc2, name, key);
>           g_free(name);
> +
> +        key |= CP_REG_AA32_NS_MASK;
>           add_cpreg_to_hashtable(cpu, r, ARM_CP_STATE_AA32, ARM_CP_SECSTATE_NS,
> -                               cp, crm, opc1, opc2, r->name);
> +                               cp, crm, opc1, opc2, r->name, key);
>           break;
>       default:
>           g_assert_not_reached();
> @@ -7573,6 +7566,8 @@ static void add_cpreg_to_hashtable_aa32(ARMCPU *cpu, const ARMCPRegInfo *r,
>   static void add_cpreg_to_hashtable_aa64(ARMCPU *cpu, const ARMCPRegInfo *r,
>                                           int crm, int opc1, int opc2)
>   {
> +    uint32_t key = ENCODE_AA64_CP_REG(r->opc0, opc1, r->crn, crm, opc2);
> +
>       if ((r->type & ARM_CP_ADD_TLBI_NXS) &&
>           cpu_isar_feature(aa64_xs, cpu)) {
>           /*
> @@ -7587,18 +7582,22 @@ static void add_cpreg_to_hashtable_aa64(ARMCPU *cpu, const ARMCPRegInfo *r,
>            */
>           ARMCPRegInfo nxs_ri = *r;
>           g_autofree char *name = g_strdup_printf("%sNXS", r->name);
> +        uint32_t nxs_key;
>   
>           assert(nxs_ri.crn < 0xf);
>           nxs_ri.crn++;
> +        nxs_key = key + (1 << CP_REG_ARM64_SYSREG_CRN_SHIFT);

This is new but not mentioned. While the CRN bit is know to be 0,
we usually use '|' to set a bit, not '+'. Preferably using '|':

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>           if (nxs_ri.fgt) {
>               nxs_ri.fgt |= R_FGT_NXS_MASK;
>           }
> +
>           add_cpreg_to_hashtable(cpu, &nxs_ri, ARM_CP_STATE_AA64,
> -                               ARM_CP_SECSTATE_NS, 0, crm, opc1, opc2, name);
> +                               ARM_CP_SECSTATE_NS, 0, crm, opc1, opc2,
> +                               name, nxs_key);
>       }
>   
>       add_cpreg_to_hashtable(cpu, r, ARM_CP_STATE_AA64, ARM_CP_SECSTATE_NS,
> -                           0, crm, opc1, opc2, r->name);
> +                           0, crm, opc1, opc2, r->name, key);
>   }
>   
>   void define_one_arm_cp_reg(ARMCPU *cpu, const ARMCPRegInfo *r)

On Thu, 25 Sept 2025 at 12:07, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 16/9/25 16:22, Richard Henderson wrote:
> > Hoist the computation of key into the caller, where
> > state is a known constant.

> > @@ -7587,18 +7582,22 @@ static void add_cpreg_to_hashtable_aa64(ARMCPU *cpu, const ARMCPRegInfo *r,
> >            */
> >           ARMCPRegInfo nxs_ri = *r;
> >           g_autofree char *name = g_strdup_printf("%sNXS", r->name);
> > +        uint32_t nxs_key;
> >
> >           assert(nxs_ri.crn < 0xf);
> >           nxs_ri.crn++;
> > +        nxs_key = key + (1 << CP_REG_ARM64_SYSREG_CRN_SHIFT);
>
> This is new but not mentioned. While the CRN bit is know to be 0,
> we usually use '|' to set a bit, not '+'. Preferably using '|':

I thought so too at first glance -- but what we're doing here
is adding one to crn (there's a comment in this function that's
just outside the context of the diff that explains this).
Since crn is both in the reginfo field and also encoded into
the key, we need to increment both the crn and the bitfield
inside the key. As it happens, at the moment all the regdefs
with ARM_CP_ADD_TLBI_NXS have crn == 8 and so whether we add
or OR makes no difference, but conceptually the addition is
correct.

I have added a comment
 /* Also increment the CRN field inside the key value */
to hopefully make it a bit clearer that we're doing an
increment operation here.

thanks
-- PMM

On 25/9/25 16:51, Peter Maydell wrote:
> On Thu, 25 Sept 2025 at 12:07, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 16/9/25 16:22, Richard Henderson wrote:
>>> Hoist the computation of key into the caller, where
>>> state is a known constant.
> 
>>> @@ -7587,18 +7582,22 @@ static void add_cpreg_to_hashtable_aa64(ARMCPU *cpu, const ARMCPRegInfo *r,
>>>             */
>>>            ARMCPRegInfo nxs_ri = *r;
>>>            g_autofree char *name = g_strdup_printf("%sNXS", r->name);
>>> +        uint32_t nxs_key;
>>>
>>>            assert(nxs_ri.crn < 0xf);
>>>            nxs_ri.crn++;
>>> +        nxs_key = key + (1 << CP_REG_ARM64_SYSREG_CRN_SHIFT);
>>
>> This is new but not mentioned. While the CRN bit is know to be 0,
>> we usually use '|' to set a bit, not '+'. Preferably using '|':
> 
> I thought so too at first glance -- but what we're doing here
> is adding one to crn (there's a comment in this function that's
> just outside the context of the diff that explains this).
> Since crn is both in the reginfo field and also encoded into
> the key, we need to increment both the crn and the bitfield
> inside the key. As it happens, at the moment all the regdefs
> with ARM_CP_ADD_TLBI_NXS have crn == 8 and so whether we add
> or OR makes no difference, but conceptually the addition is
> correct.
> 
> I have added a comment
>   /* Also increment the CRN field inside the key value */
> to hopefully make it a bit clearer that we're doing an
> increment operation here.

Thanks!