Re: [PATCH v2 24/36] target/arm: Add key parameter to add_cpreg_to_hashtable

[Philippe Mathieu-Daudé]

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.hender...@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é <phi...@linaro.org>
Tested-by: Philippe Mathieu-Daudé <phi...@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)