[PATCH 0/2] target/arm: Look up ARMCPRegInfo at runtime

[PATCH 0/2] target/arm: Look up ARMCPRegInfo at runtime

[Richard Henderson]

Here's a short-to-medium term alternative to moving all of the ARMCPU
cp_regs hash table to the ARMCPUClass, so that we're no longer leaving
dangling pointers to freed objects encoded in the compiled
TranslationBlocks.  (I still think we ought to do less work at
object_{init,realize}, but that may be a much longer term project.)

Instead of giving the helper a direct pointer, pass the cpreg hash key,
which will be constant across cpus.  Perform this lookup in the existing
helper_access_check_cp_reg (which had a return value going spare), or a
new helper_lookup_cp_reg.  The other cp_regs functions are unchanged,
because they still get a pointer.

This ought to be enough to re-instate Alex's linux-user patch
to free the cpu object after thread termination.


r~


Richard Henderson (2):
  target/arm: Reorg do_coproc_insn
  target/arm: Look up ARMCPRegInfo at runtime

 target/arm/helper.h        |  11 +-
 target/arm/translate.h     |   7 +
 target/arm/op_helper.c     |  27 ++-
 target/arm/translate-a64.c |  49 +++--
 target/arm/translate.c     | 430 +++++++++++++++++++------------------
 5 files changed, 285 insertions(+), 239 deletions(-)

-- 
2.34.1

[PATCH 1/2] target/arm: Reorg do_coproc_insn

[Richard Henderson]

Move the ri == NULL case to the top of the function and return.
This allows the else to be removed and the code unindented.

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

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 74a903072f..a84a02964e 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -4715,220 +4715,220 @@ static void do_coproc_insn(DisasContext *s, int cpnum, int is64,
                            bool isread, int rt, int rt2)
 {
     const ARMCPRegInfo *ri;
+    bool need_exit_tb;
 
     ri = get_arm_cp_reginfo(s->cp_regs,
             ENCODE_CP_REG(cpnum, is64, s->ns, crn, crm, opc1, opc2));
-    if (ri) {
-        bool need_exit_tb;
 
-        /* Check access permissions */
-        if (!cp_access_ok(s->current_el, ri, isread)) {
-            unallocated_encoding(s);
-            return;
-        }
-
-        if (s->hstr_active || ri->accessfn ||
-            (arm_dc_feature(s, ARM_FEATURE_XSCALE) && cpnum < 14)) {
-            /* Emit code to perform further access permissions checks at
-             * runtime; this may result in an exception.
-             * Note that on XScale all cp0..c13 registers do an access check
-             * call in order to handle c15_cpar.
-             */
-            uint32_t syndrome;
-
-            /* Note that since we are an implementation which takes an
-             * exception on a trapped conditional instruction only if the
-             * instruction passes its condition code check, we can take
-             * advantage of the clause in the ARM ARM that allows us to set
-             * the COND field in the instruction to 0xE in all cases.
-             * We could fish the actual condition out of the insn (ARM)
-             * or the condexec bits (Thumb) but it isn't necessary.
-             */
-            switch (cpnum) {
-            case 14:
-                if (is64) {
-                    syndrome = syn_cp14_rrt_trap(1, 0xe, opc1, crm, rt, rt2,
-                                                 isread, false);
-                } else {
-                    syndrome = syn_cp14_rt_trap(1, 0xe, opc1, opc2, crn, crm,
-                                                rt, isread, false);
-                }
-                break;
-            case 15:
-                if (is64) {
-                    syndrome = syn_cp15_rrt_trap(1, 0xe, opc1, crm, rt, rt2,
-                                                 isread, false);
-                } else {
-                    syndrome = syn_cp15_rt_trap(1, 0xe, opc1, opc2, crn, crm,
-                                                rt, isread, false);
-                }
-                break;
-            default:
-                /* ARMv8 defines that only coprocessors 14 and 15 exist,
-                 * so this can only happen if this is an ARMv7 or earlier CPU,
-                 * in which case the syndrome information won't actually be
-                 * guest visible.
-                 */
-                assert(!arm_dc_feature(s, ARM_FEATURE_V8));
-                syndrome = syn_uncategorized();
-                break;
-            }
-
-            gen_set_condexec(s);
-            gen_update_pc(s, 0);
-            gen_helper_access_check_cp_reg(cpu_env,
-                                           tcg_constant_ptr(ri),
-                                           tcg_constant_i32(syndrome),
-                                           tcg_constant_i32(isread));
-        } else if (ri->type & ARM_CP_RAISES_EXC) {
-            /*
-             * The readfn or writefn might raise an exception;
-             * synchronize the CPU state in case it does.
-             */
-            gen_set_condexec(s);
-            gen_update_pc(s, 0);
-        }
-
-        /* Handle special cases first */
-        switch (ri->type & ARM_CP_SPECIAL_MASK) {
-        case 0:
-            break;
-        case ARM_CP_NOP:
-            return;
-        case ARM_CP_WFI:
-            if (isread) {
-                unallocated_encoding(s);
-                return;
-            }
-            gen_update_pc(s, curr_insn_len(s));
-            s->base.is_jmp = DISAS_WFI;
-            return;
-        default:
-            g_assert_not_reached();
-        }
-
-        if ((tb_cflags(s->base.tb) & CF_USE_ICOUNT) && (ri->type & ARM_CP_IO)) {
-            gen_io_start();
-        }
-
-        if (isread) {
-            /* Read */
-            if (is64) {
-                TCGv_i64 tmp64;
-                TCGv_i32 tmp;
-                if (ri->type & ARM_CP_CONST) {
-                    tmp64 = tcg_constant_i64(ri->resetvalue);
-                } else if (ri->readfn) {
-                    tmp64 = tcg_temp_new_i64();
-                    gen_helper_get_cp_reg64(tmp64, cpu_env,
-                                            tcg_constant_ptr(ri));
-                } else {
-                    tmp64 = tcg_temp_new_i64();
-                    tcg_gen_ld_i64(tmp64, cpu_env, ri->fieldoffset);
-                }
-                tmp = tcg_temp_new_i32();
-                tcg_gen_extrl_i64_i32(tmp, tmp64);
-                store_reg(s, rt, tmp);
-                tmp = tcg_temp_new_i32();
-                tcg_gen_extrh_i64_i32(tmp, tmp64);
-                tcg_temp_free_i64(tmp64);
-                store_reg(s, rt2, tmp);
-            } else {
-                TCGv_i32 tmp;
-                if (ri->type & ARM_CP_CONST) {
-                    tmp = tcg_constant_i32(ri->resetvalue);
-                } else if (ri->readfn) {
-                    tmp = tcg_temp_new_i32();
-                    gen_helper_get_cp_reg(tmp, cpu_env, tcg_constant_ptr(ri));
-                } else {
-                    tmp = load_cpu_offset(ri->fieldoffset);
-                }
-                if (rt == 15) {
-                    /* Destination register of r15 for 32 bit loads sets
-                     * the condition codes from the high 4 bits of the value
-                     */
-                    gen_set_nzcv(tmp);
-                    tcg_temp_free_i32(tmp);
-                } else {
-                    store_reg(s, rt, tmp);
-                }
-            }
+    if (!ri) {
+        /*
+         * Unknown register; this might be a guest error or a QEMU
+         * unimplemented feature.
+         */
+        if (is64) {
+            qemu_log_mask(LOG_UNIMP, "%s access to unsupported AArch32 "
+                          "64 bit system register cp:%d opc1: %d crm:%d "
+                          "(%s)\n",
+                          isread ? "read" : "write", cpnum, opc1, crm,
+                          s->ns ? "non-secure" : "secure");
         } else {
-            /* Write */
-            if (ri->type & ARM_CP_CONST) {
-                /* If not forbidden by access permissions, treat as WI */
-                return;
-            }
-
-            if (is64) {
-                TCGv_i32 tmplo, tmphi;
-                TCGv_i64 tmp64 = tcg_temp_new_i64();
-                tmplo = load_reg(s, rt);
-                tmphi = load_reg(s, rt2);
-                tcg_gen_concat_i32_i64(tmp64, tmplo, tmphi);
-                tcg_temp_free_i32(tmplo);
-                tcg_temp_free_i32(tmphi);
-                if (ri->writefn) {
-                    gen_helper_set_cp_reg64(cpu_env, tcg_constant_ptr(ri),
-                                            tmp64);
-                } else {
-                    tcg_gen_st_i64(tmp64, cpu_env, ri->fieldoffset);
-                }
-                tcg_temp_free_i64(tmp64);
-            } else {
-                TCGv_i32 tmp = load_reg(s, rt);
-                if (ri->writefn) {
-                    gen_helper_set_cp_reg(cpu_env, tcg_constant_ptr(ri), tmp);
-                    tcg_temp_free_i32(tmp);
-                } else {
-                    store_cpu_offset(tmp, ri->fieldoffset, 4);
-                }
-            }
+            qemu_log_mask(LOG_UNIMP, "%s access to unsupported AArch32 "
+                          "system register cp:%d opc1:%d crn:%d crm:%d "
+                          "opc2:%d (%s)\n",
+                          isread ? "read" : "write", cpnum, opc1, crn,
+                          crm, opc2, s->ns ? "non-secure" : "secure");
         }
-
-        /* I/O operations must end the TB here (whether read or write) */
-        need_exit_tb = ((tb_cflags(s->base.tb) & CF_USE_ICOUNT) &&
-                        (ri->type & ARM_CP_IO));
-
-        if (!isread && !(ri->type & ARM_CP_SUPPRESS_TB_END)) {
-            /*
-             * A write to any coprocessor register that ends a TB
-             * must rebuild the hflags for the next TB.
-             */
-            gen_rebuild_hflags(s, ri->type & ARM_CP_NEWEL);
-            /*
-             * We default to ending the TB on a coprocessor register write,
-             * but allow this to be suppressed by the register definition
-             * (usually only necessary to work around guest bugs).
-             */
-            need_exit_tb = true;
-        }
-        if (need_exit_tb) {
-            gen_lookup_tb(s);
-        }
-
+        unallocated_encoding(s);
         return;
     }
 
-    /* Unknown register; this might be a guest error or a QEMU
-     * unimplemented feature.
-     */
-    if (is64) {
-        qemu_log_mask(LOG_UNIMP, "%s access to unsupported AArch32 "
-                      "64 bit system register cp:%d opc1: %d crm:%d "
-                      "(%s)\n",
-                      isread ? "read" : "write", cpnum, opc1, crm,
-                      s->ns ? "non-secure" : "secure");
-    } else {
-        qemu_log_mask(LOG_UNIMP, "%s access to unsupported AArch32 "
-                      "system register cp:%d opc1:%d crn:%d crm:%d opc2:%d "
-                      "(%s)\n",
-                      isread ? "read" : "write", cpnum, opc1, crn, crm, opc2,
-                      s->ns ? "non-secure" : "secure");
+    /* Check access permissions */
+    if (!cp_access_ok(s->current_el, ri, isread)) {
+        unallocated_encoding(s);
+        return;
     }
 
-    unallocated_encoding(s);
-    return;
+    if (s->hstr_active || ri->accessfn ||
+        (arm_dc_feature(s, ARM_FEATURE_XSCALE) && cpnum < 14)) {
+        /*
+         * Emit code to perform further access permissions checks at
+         * runtime; this may result in an exception.
+         * Note that on XScale all cp0..c13 registers do an access check
+         * call in order to handle c15_cpar.
+         */
+        uint32_t syndrome;
+
+        /*
+         * Note that since we are an implementation which takes an
+         * exception on a trapped conditional instruction only if the
+         * instruction passes its condition code check, we can take
+         * advantage of the clause in the ARM ARM that allows us to set
+         * the COND field in the instruction to 0xE in all cases.
+         * We could fish the actual condition out of the insn (ARM)
+         * or the condexec bits (Thumb) but it isn't necessary.
+         */
+        switch (cpnum) {
+        case 14:
+            if (is64) {
+                syndrome = syn_cp14_rrt_trap(1, 0xe, opc1, crm, rt, rt2,
+                                             isread, false);
+            } else {
+                syndrome = syn_cp14_rt_trap(1, 0xe, opc1, opc2, crn, crm,
+                                            rt, isread, false);
+            }
+            break;
+        case 15:
+            if (is64) {
+                syndrome = syn_cp15_rrt_trap(1, 0xe, opc1, crm, rt, rt2,
+                                             isread, false);
+            } else {
+                syndrome = syn_cp15_rt_trap(1, 0xe, opc1, opc2, crn, crm,
+                                            rt, isread, false);
+            }
+            break;
+        default:
+            /*
+             * ARMv8 defines that only coprocessors 14 and 15 exist,
+             * so this can only happen if this is an ARMv7 or earlier CPU,
+             * in which case the syndrome information won't actually be
+             * guest visible.
+             */
+            assert(!arm_dc_feature(s, ARM_FEATURE_V8));
+            syndrome = syn_uncategorized();
+            break;
+        }
+
+        gen_set_condexec(s);
+        gen_update_pc(s, 0);
+        gen_helper_access_check_cp_reg(cpu_env,
+                                       tcg_constant_ptr(ri),
+                                       tcg_constant_i32(syndrome),
+                                       tcg_constant_i32(isread));
+    } else if (ri->type & ARM_CP_RAISES_EXC) {
+        /*
+         * The readfn or writefn might raise an exception;
+         * synchronize the CPU state in case it does.
+         */
+        gen_set_condexec(s);
+        gen_update_pc(s, 0);
+    }
+
+    /* Handle special cases first */
+    switch (ri->type & ARM_CP_SPECIAL_MASK) {
+    case 0:
+        break;
+    case ARM_CP_NOP:
+        return;
+    case ARM_CP_WFI:
+        if (isread) {
+            unallocated_encoding(s);
+            return;
+        }
+        gen_update_pc(s, curr_insn_len(s));
+        s->base.is_jmp = DISAS_WFI;
+        return;
+    default:
+        g_assert_not_reached();
+    }
+
+    if ((tb_cflags(s->base.tb) & CF_USE_ICOUNT) && (ri->type & ARM_CP_IO)) {
+        gen_io_start();
+    }
+
+    if (isread) {
+        /* Read */
+        if (is64) {
+            TCGv_i64 tmp64;
+            TCGv_i32 tmp;
+            if (ri->type & ARM_CP_CONST) {
+                tmp64 = tcg_constant_i64(ri->resetvalue);
+            } else if (ri->readfn) {
+                tmp64 = tcg_temp_new_i64();
+                gen_helper_get_cp_reg64(tmp64, cpu_env,
+                                        tcg_constant_ptr(ri));
+            } else {
+                tmp64 = tcg_temp_new_i64();
+                tcg_gen_ld_i64(tmp64, cpu_env, ri->fieldoffset);
+            }
+            tmp = tcg_temp_new_i32();
+            tcg_gen_extrl_i64_i32(tmp, tmp64);
+            store_reg(s, rt, tmp);
+            tmp = tcg_temp_new_i32();
+            tcg_gen_extrh_i64_i32(tmp, tmp64);
+            tcg_temp_free_i64(tmp64);
+            store_reg(s, rt2, tmp);
+        } else {
+            TCGv_i32 tmp;
+            if (ri->type & ARM_CP_CONST) {
+                tmp = tcg_constant_i32(ri->resetvalue);
+            } else if (ri->readfn) {
+                tmp = tcg_temp_new_i32();
+                gen_helper_get_cp_reg(tmp, cpu_env, tcg_constant_ptr(ri));
+            } else {
+                tmp = load_cpu_offset(ri->fieldoffset);
+            }
+            if (rt == 15) {
+                /* Destination register of r15 for 32 bit loads sets
+                 * the condition codes from the high 4 bits of the value
+                 */
+                gen_set_nzcv(tmp);
+                tcg_temp_free_i32(tmp);
+            } else {
+                store_reg(s, rt, tmp);
+            }
+        }
+    } else {
+        /* Write */
+        if (ri->type & ARM_CP_CONST) {
+            /* If not forbidden by access permissions, treat as WI */
+            return;
+        }
+
+        if (is64) {
+            TCGv_i32 tmplo, tmphi;
+            TCGv_i64 tmp64 = tcg_temp_new_i64();
+            tmplo = load_reg(s, rt);
+            tmphi = load_reg(s, rt2);
+            tcg_gen_concat_i32_i64(tmp64, tmplo, tmphi);
+            tcg_temp_free_i32(tmplo);
+            tcg_temp_free_i32(tmphi);
+            if (ri->writefn) {
+                gen_helper_set_cp_reg64(cpu_env, tcg_constant_ptr(ri), tmp64);
+            } else {
+                tcg_gen_st_i64(tmp64, cpu_env, ri->fieldoffset);
+            }
+            tcg_temp_free_i64(tmp64);
+        } else {
+            TCGv_i32 tmp = load_reg(s, rt);
+            if (ri->writefn) {
+                gen_helper_set_cp_reg(cpu_env, tcg_constant_ptr(ri), tmp);
+                tcg_temp_free_i32(tmp);
+            } else {
+                store_cpu_offset(tmp, ri->fieldoffset, 4);
+            }
+        }
+    }
+
+    /* I/O operations must end the TB here (whether read or write) */
+    need_exit_tb = ((tb_cflags(s->base.tb) & CF_USE_ICOUNT) &&
+                    (ri->type & ARM_CP_IO));
+
+    if (!isread && !(ri->type & ARM_CP_SUPPRESS_TB_END)) {
+        /*
+         * A write to any coprocessor register that ends a TB
+         * must rebuild the hflags for the next TB.
+         */
+        gen_rebuild_hflags(s, ri->type & ARM_CP_NEWEL);
+        /*
+         * We default to ending the TB on a coprocessor register write,
+         * but allow this to be suppressed by the register definition
+         * (usually only necessary to work around guest bugs).
+         */
+        need_exit_tb = true;
+    }
+    if (need_exit_tb) {
+        gen_lookup_tb(s);
+    }
 }
 
 /* Decode XScale DSP or iWMMXt insn (in the copro space, cp=0 or 1) */
-- 
2.34.1

Re: [PATCH 1/2] target/arm: Reorg do_coproc_insn

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Move the ri == NULL case to the top of the function and return.
> This allows the else to be removed and the code unindented.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 2/2] target/arm: Look up ARMCPRegInfo at runtime

[Richard Henderson]

Do not encode the pointer as a constant in the opcode stream.
This pointer is specific to the cpu that first generated the
translation, which runs into problems with both hot-pluggable
cpus and user-only threads, as cpus are removed.

Perform the lookup in either helper_access_check_cp_reg,
or a new helper_lookup_cp_reg.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.h        | 11 +++++----
 target/arm/translate.h     |  7 ++++++
 target/arm/op_helper.c     | 27 ++++++++++++++------
 target/arm/translate-a64.c | 49 ++++++++++++++++++++++---------------
 target/arm/translate.c     | 50 +++++++++++++++++++++++++-------------
 5 files changed, 95 insertions(+), 49 deletions(-)

diff --git a/target/arm/helper.h b/target/arm/helper.h
index 92f36d9dbb..018b00ea75 100644
--- a/target/arm/helper.h
+++ b/target/arm/helper.h
@@ -79,11 +79,12 @@ DEF_HELPER_2(v8m_stackcheck, void, env, i32)
 
 DEF_HELPER_FLAGS_2(check_bxj_trap, TCG_CALL_NO_WG, void, env, i32)
 
-DEF_HELPER_4(access_check_cp_reg, void, env, ptr, i32, i32)
-DEF_HELPER_3(set_cp_reg, void, env, ptr, i32)
-DEF_HELPER_2(get_cp_reg, i32, env, ptr)
-DEF_HELPER_3(set_cp_reg64, void, env, ptr, i64)
-DEF_HELPER_2(get_cp_reg64, i64, env, ptr)
+DEF_HELPER_4(access_check_cp_reg, cptr, env, i32, i32, i32)
+DEF_HELPER_FLAGS_2(lookup_cp_reg, TCG_CALL_NO_RWG_SE, cptr, env, i32)
+DEF_HELPER_3(set_cp_reg, void, env, cptr, i32)
+DEF_HELPER_2(get_cp_reg, i32, env, cptr)
+DEF_HELPER_3(set_cp_reg64, void, env, cptr, i64)
+DEF_HELPER_2(get_cp_reg64, i64, env, cptr)
 
 DEF_HELPER_2(get_r13_banked, i32, env, i32)
 DEF_HELPER_3(set_r13_banked, void, env, i32, i32)
diff --git a/target/arm/translate.h b/target/arm/translate.h
index 3cdc7dbc2f..f17f095cbe 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -610,6 +610,13 @@ static inline void set_disas_label(DisasContext *s, DisasLabel l)
     s->pc_save = l.pc_save;
 }
 
+static inline TCGv_ptr gen_lookup_cp_reg(uint32_t key)
+{
+    TCGv_ptr ret = tcg_temp_new_ptr();
+    gen_helper_lookup_cp_reg(ret, cpu_env, tcg_constant_i32(key));
+    return ret;
+}
+
 /*
  * Helpers for implementing sets of trans_* functions.
  * Defer the implementation of NAME to FUNC, with optional extra arguments.
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 70672bcd9f..31f89db899 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -624,14 +624,16 @@ uint32_t HELPER(mrs_banked)(CPUARMState *env, uint32_t tgtmode, uint32_t regno)
     }
 }
 
-void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome,
-                                 uint32_t isread)
+const void *HELPER(access_check_cp_reg)(CPUARMState *env, uint32_t key,
+                                        uint32_t syndrome, uint32_t isread)
 {
     ARMCPU *cpu = env_archcpu(env);
-    const ARMCPRegInfo *ri = rip;
+    const ARMCPRegInfo *ri = get_arm_cp_reginfo(cpu->cp_regs, key);
     CPAccessResult res = CP_ACCESS_OK;
     int target_el;
 
+    assert(ri != NULL);
+
     if (arm_feature(env, ARM_FEATURE_XSCALE) && ri->cp < 14
         && extract32(env->cp15.c15_cpar, ri->cp, 1) == 0) {
         res = CP_ACCESS_TRAP;
@@ -663,7 +665,7 @@ void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome,
         res = ri->accessfn(env, ri, isread);
     }
     if (likely(res == CP_ACCESS_OK)) {
-        return;
+        return ri;
     }
 
  fail:
@@ -705,7 +707,16 @@ void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome,
     raise_exception(env, EXCP_UDEF, syndrome, target_el);
 }
 
-void HELPER(set_cp_reg)(CPUARMState *env, void *rip, uint32_t value)
+const void *HELPER(lookup_cp_reg)(CPUARMState *env, uint32_t key)
+{
+    ARMCPU *cpu = env_archcpu(env);
+    const ARMCPRegInfo *ri = get_arm_cp_reginfo(cpu->cp_regs, key);
+
+    assert(ri != NULL);
+    return ri;
+}
+
+void HELPER(set_cp_reg)(CPUARMState *env, const void *rip, uint32_t value)
 {
     const ARMCPRegInfo *ri = rip;
 
@@ -718,7 +729,7 @@ void HELPER(set_cp_reg)(CPUARMState *env, void *rip, uint32_t value)
     }
 }
 
-uint32_t HELPER(get_cp_reg)(CPUARMState *env, void *rip)
+uint32_t HELPER(get_cp_reg)(CPUARMState *env, const void *rip)
 {
     const ARMCPRegInfo *ri = rip;
     uint32_t res;
@@ -734,7 +745,7 @@ uint32_t HELPER(get_cp_reg)(CPUARMState *env, void *rip)
     return res;
 }
 
-void HELPER(set_cp_reg64)(CPUARMState *env, void *rip, uint64_t value)
+void HELPER(set_cp_reg64)(CPUARMState *env, const void *rip, uint64_t value)
 {
     const ARMCPRegInfo *ri = rip;
 
@@ -747,7 +758,7 @@ void HELPER(set_cp_reg64)(CPUARMState *env, void *rip, uint64_t value)
     }
 }
 
-uint64_t HELPER(get_cp_reg64)(CPUARMState *env, void *rip)
+uint64_t HELPER(get_cp_reg64)(CPUARMState *env, const void *rip)
 {
     const ARMCPRegInfo *ri = rip;
     uint64_t res;
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 2ee171f249..543635d6b0 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1944,13 +1944,12 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
                        unsigned int op0, unsigned int op1, unsigned int op2,
                        unsigned int crn, unsigned int crm, unsigned int rt)
 {
-    const ARMCPRegInfo *ri;
+    uint32_t key = ENCODE_AA64_CP_REG(CP_REG_ARM64_SYSREG_CP,
+                                      crn, crm, op0, op1, op2);
+    const ARMCPRegInfo *ri = get_arm_cp_reginfo(s->cp_regs, key);
+    TCGv_ptr tcg_ri = NULL;
     TCGv_i64 tcg_rt;
 
-    ri = get_arm_cp_reginfo(s->cp_regs,
-                            ENCODE_AA64_CP_REG(CP_REG_ARM64_SYSREG_CP,
-                                               crn, crm, op0, op1, op2));
-
     if (!ri) {
         /* Unknown register; this might be a guest error or a QEMU
          * unimplemented feature.
@@ -1976,8 +1975,9 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
 
         syndrome = syn_aa64_sysregtrap(op0, op1, op2, crn, crm, rt, isread);
         gen_a64_update_pc(s, 0);
-        gen_helper_access_check_cp_reg(cpu_env,
-                                       tcg_constant_ptr(ri),
+        tcg_ri = tcg_temp_new_ptr();
+        gen_helper_access_check_cp_reg(tcg_ri, cpu_env,
+                                       tcg_constant_i32(key),
                                        tcg_constant_i32(syndrome),
                                        tcg_constant_i32(isread));
     } else if (ri->type & ARM_CP_RAISES_EXC) {
@@ -1993,7 +1993,7 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
     case 0:
         break;
     case ARM_CP_NOP:
-        return;
+        goto exit;
     case ARM_CP_NZCV:
         tcg_rt = cpu_reg(s, rt);
         if (isread) {
@@ -2001,14 +2001,14 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
         } else {
             gen_set_nzcv(tcg_rt);
         }
-        return;
+        goto exit;
     case ARM_CP_CURRENTEL:
         /* Reads as current EL value from pstate, which is
          * guaranteed to be constant by the tb flags.
          */
         tcg_rt = cpu_reg(s, rt);
         tcg_gen_movi_i64(tcg_rt, s->current_el << 2);
-        return;
+        goto exit;
     case ARM_CP_DC_ZVA:
         /* Writes clear the aligned block of memory which rt points into. */
         if (s->mte_active[0]) {
@@ -2025,7 +2025,7 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
             tcg_rt = clean_data_tbi(s, cpu_reg(s, rt));
         }
         gen_helper_dc_zva(cpu_env, tcg_rt);
-        return;
+        goto exit;
     case ARM_CP_DC_GVA:
         {
             TCGv_i64 clean_addr, tag;
@@ -2046,7 +2046,7 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
                 tcg_temp_free_i64(tag);
             }
         }
-        return;
+        goto exit;
     case ARM_CP_DC_GZVA:
         {
             TCGv_i64 clean_addr, tag;
@@ -2064,16 +2064,16 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
                 tcg_temp_free_i64(tag);
             }
         }
-        return;
+        goto exit;
     default:
         g_assert_not_reached();
     }
     if ((ri->type & ARM_CP_FPU) && !fp_access_check_only(s)) {
-        return;
+        goto exit;
     } else if ((ri->type & ARM_CP_SVE) && !sve_access_check(s)) {
-        return;
+        goto exit;
     } else if ((ri->type & ARM_CP_SME) && !sme_access_check(s)) {
-        return;
+        goto exit;
     }
 
     if ((tb_cflags(s->base.tb) & CF_USE_ICOUNT) && (ri->type & ARM_CP_IO)) {
@@ -2086,16 +2086,22 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
         if (ri->type & ARM_CP_CONST) {
             tcg_gen_movi_i64(tcg_rt, ri->resetvalue);
         } else if (ri->readfn) {
-            gen_helper_get_cp_reg64(tcg_rt, cpu_env, tcg_constant_ptr(ri));
+            if (!tcg_ri) {
+                tcg_ri = gen_lookup_cp_reg(key);
+            }
+            gen_helper_get_cp_reg64(tcg_rt, cpu_env, tcg_ri);
         } else {
             tcg_gen_ld_i64(tcg_rt, cpu_env, ri->fieldoffset);
         }
     } else {
         if (ri->type & ARM_CP_CONST) {
             /* If not forbidden by access permissions, treat as WI */
-            return;
+            goto exit;
         } else if (ri->writefn) {
-            gen_helper_set_cp_reg64(cpu_env, tcg_constant_ptr(ri), tcg_rt);
+            if (!tcg_ri) {
+                tcg_ri = gen_lookup_cp_reg(key);
+            }
+            gen_helper_set_cp_reg64(cpu_env, tcg_ri, tcg_rt);
         } else {
             tcg_gen_st_i64(tcg_rt, cpu_env, ri->fieldoffset);
         }
@@ -2118,6 +2124,11 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
          */
         s->base.is_jmp = DISAS_UPDATE_EXIT;
     }
+
+ exit:
+    if (tcg_ri) {
+        tcg_temp_free_ptr(tcg_ri);
+    }
 }
 
 /* System
diff --git a/target/arm/translate.c b/target/arm/translate.c
index a84a02964e..0f8db04bad 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -4714,12 +4714,11 @@ static void do_coproc_insn(DisasContext *s, int cpnum, int is64,
                            int opc1, int crn, int crm, int opc2,
                            bool isread, int rt, int rt2)
 {
-    const ARMCPRegInfo *ri;
+    uint32_t key = ENCODE_CP_REG(cpnum, is64, s->ns, crn, crm, opc1, opc2);
+    const ARMCPRegInfo *ri = get_arm_cp_reginfo(s->cp_regs, key);
+    TCGv_ptr tcg_ri = NULL;
     bool need_exit_tb;
 
-    ri = get_arm_cp_reginfo(s->cp_regs,
-            ENCODE_CP_REG(cpnum, is64, s->ns, crn, crm, opc1, opc2));
-
     if (!ri) {
         /*
          * Unknown register; this might be a guest error or a QEMU
@@ -4800,8 +4799,9 @@ static void do_coproc_insn(DisasContext *s, int cpnum, int is64,
 
         gen_set_condexec(s);
         gen_update_pc(s, 0);
-        gen_helper_access_check_cp_reg(cpu_env,
-                                       tcg_constant_ptr(ri),
+        tcg_ri = tcg_temp_new_ptr();
+        gen_helper_access_check_cp_reg(tcg_ri, cpu_env,
+                                       tcg_constant_i32(key),
                                        tcg_constant_i32(syndrome),
                                        tcg_constant_i32(isread));
     } else if (ri->type & ARM_CP_RAISES_EXC) {
@@ -4818,15 +4818,15 @@ static void do_coproc_insn(DisasContext *s, int cpnum, int is64,
     case 0:
         break;
     case ARM_CP_NOP:
-        return;
+        goto exit;
     case ARM_CP_WFI:
         if (isread) {
             unallocated_encoding(s);
-            return;
+        } else {
+            gen_update_pc(s, curr_insn_len(s));
+            s->base.is_jmp = DISAS_WFI;
         }
-        gen_update_pc(s, curr_insn_len(s));
-        s->base.is_jmp = DISAS_WFI;
-        return;
+        goto exit;
     default:
         g_assert_not_reached();
     }
@@ -4843,9 +4843,11 @@ static void do_coproc_insn(DisasContext *s, int cpnum, int is64,
             if (ri->type & ARM_CP_CONST) {
                 tmp64 = tcg_constant_i64(ri->resetvalue);
             } else if (ri->readfn) {
+                if (!tcg_ri) {
+                    tcg_ri = gen_lookup_cp_reg(key);
+                }
                 tmp64 = tcg_temp_new_i64();
-                gen_helper_get_cp_reg64(tmp64, cpu_env,
-                                        tcg_constant_ptr(ri));
+                gen_helper_get_cp_reg64(tmp64, cpu_env, tcg_ri);
             } else {
                 tmp64 = tcg_temp_new_i64();
                 tcg_gen_ld_i64(tmp64, cpu_env, ri->fieldoffset);
@@ -4862,8 +4864,11 @@ static void do_coproc_insn(DisasContext *s, int cpnum, int is64,
             if (ri->type & ARM_CP_CONST) {
                 tmp = tcg_constant_i32(ri->resetvalue);
             } else if (ri->readfn) {
+                if (!tcg_ri) {
+                    tcg_ri = gen_lookup_cp_reg(key);
+                }
                 tmp = tcg_temp_new_i32();
-                gen_helper_get_cp_reg(tmp, cpu_env, tcg_constant_ptr(ri));
+                gen_helper_get_cp_reg(tmp, cpu_env, tcg_ri);
             } else {
                 tmp = load_cpu_offset(ri->fieldoffset);
             }
@@ -4881,7 +4886,7 @@ static void do_coproc_insn(DisasContext *s, int cpnum, int is64,
         /* Write */
         if (ri->type & ARM_CP_CONST) {
             /* If not forbidden by access permissions, treat as WI */
-            return;
+            goto exit;
         }
 
         if (is64) {
@@ -4893,7 +4898,10 @@ static void do_coproc_insn(DisasContext *s, int cpnum, int is64,
             tcg_temp_free_i32(tmplo);
             tcg_temp_free_i32(tmphi);
             if (ri->writefn) {
-                gen_helper_set_cp_reg64(cpu_env, tcg_constant_ptr(ri), tmp64);
+                if (!tcg_ri) {
+                    tcg_ri = gen_lookup_cp_reg(key);
+                }
+                gen_helper_set_cp_reg64(cpu_env, tcg_ri, tmp64);
             } else {
                 tcg_gen_st_i64(tmp64, cpu_env, ri->fieldoffset);
             }
@@ -4901,7 +4909,10 @@ static void do_coproc_insn(DisasContext *s, int cpnum, int is64,
         } else {
             TCGv_i32 tmp = load_reg(s, rt);
             if (ri->writefn) {
-                gen_helper_set_cp_reg(cpu_env, tcg_constant_ptr(ri), tmp);
+                if (!tcg_ri) {
+                    tcg_ri = gen_lookup_cp_reg(key);
+                }
+                gen_helper_set_cp_reg(cpu_env, tcg_ri, tmp);
                 tcg_temp_free_i32(tmp);
             } else {
                 store_cpu_offset(tmp, ri->fieldoffset, 4);
@@ -4929,6 +4940,11 @@ static void do_coproc_insn(DisasContext *s, int cpnum, int is64,
     if (need_exit_tb) {
         gen_lookup_tb(s);
     }
+
+ exit:
+    if (tcg_ri) {
+        tcg_temp_free_ptr(tcg_ri);
+    }
 }
 
 /* Decode XScale DSP or iWMMXt insn (in the copro space, cp=0 or 1) */
-- 
2.34.1

Re: [PATCH 2/2] target/arm: Look up ARMCPRegInfo at runtime

[Peter Maydell]

On Fri, 6 Jan 2023 at 19:45, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Do not encode the pointer as a constant in the opcode stream.
> This pointer is specific to the cpu that first generated the
> translation, which runs into problems with both hot-pluggable
> cpus and user-only threads, as cpus are removed.
>
> Perform the lookup in either helper_access_check_cp_reg,
> or a new helper_lookup_cp_reg.

As well as the use-after-free, this is also a correctness
bug, isn't it? If we hardwire in the cpregs pointer for
CPU 0 into the TB, and then CPU 1 with a slightly different
config executes the TB, it will get the cpregs of CPU 0,
not its own, so it might see a register it should not or
vice-versa.

So I think we need this patch anyway, even if we're going
to try to do something to improve sharing of cpreg hashtables
across CPUs.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 2/2] target/arm: Look up ARMCPRegInfo at runtime

[Richard Henderson]

On 1/23/23 02:53, Peter Maydell wrote:
> On Fri, 6 Jan 2023 at 19:45, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Do not encode the pointer as a constant in the opcode stream.
>> This pointer is specific to the cpu that first generated the
>> translation, which runs into problems with both hot-pluggable
>> cpus and user-only threads, as cpus are removed.
>>
>> Perform the lookup in either helper_access_check_cp_reg,
>> or a new helper_lookup_cp_reg.
> 
> As well as the use-after-free, this is also a correctness
> bug, isn't it? If we hardwire in the cpregs pointer for
> CPU 0 into the TB, and then CPU 1 with a slightly different
> config executes the TB, it will get the cpregs of CPU 0,
> not its own, so it might see a register it should not or
> vice-versa.

Existing assumption was that each cpu configuration would have its own cluster_index, 
which gets encoded into cpu->tcg_cflags, which is part of the comparison used when hashing 
TBs.

But including this patch allows relaxation of what constitutes a "cpu configuration".


r~

Re: [PATCH 2/2] target/arm: Look up ARMCPRegInfo at runtime

[Peter Maydell]

On Tue, 24 Jan 2023 at 00:20, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 1/23/23 02:53, Peter Maydell wrote:
> > On Fri, 6 Jan 2023 at 19:45, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> Do not encode the pointer as a constant in the opcode stream.
> >> This pointer is specific to the cpu that first generated the
> >> translation, which runs into problems with both hot-pluggable
> >> cpus and user-only threads, as cpus are removed.
> >>
> >> Perform the lookup in either helper_access_check_cp_reg,
> >> or a new helper_lookup_cp_reg.
> >
> > As well as the use-after-free, this is also a correctness
> > bug, isn't it? If we hardwire in the cpregs pointer for
> > CPU 0 into the TB, and then CPU 1 with a slightly different
> > config executes the TB, it will get the cpregs of CPU 0,
> > not its own, so it might see a register it should not or
> > vice-versa.
>
> Existing assumption was that each cpu configuration would have its own cluster_index,
> which gets encoded into cpu->tcg_cflags, which is part of the comparison used when hashing
> TBs.

Yes, I realized that last night (so my edit to the commit message
is unfortunately wrong). If we really did need to share the TB
between different-cpregs CPUs we'd need to do more than this
patch does, because we couldn't generate "no such register" code
in the TB or the other things we do based on what the cpreg lookup
returns, we'd have to do everything at runtime.

We could in theory share cpregs hashtables between the CPUs in
a cluster, except that at CPU creation time you don't know
which cluster the CPU is going to be in...

thanks
-- PMM

Re: [PATCH 2/2] target/arm: Look up ARMCPRegInfo at runtime

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 1/23/23 02:53, Peter Maydell wrote:
>> On Fri, 6 Jan 2023 at 19:45, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>
>>> Do not encode the pointer as a constant in the opcode stream.
>>> This pointer is specific to the cpu that first generated the
>>> translation, which runs into problems with both hot-pluggable
>>> cpus and user-only threads, as cpus are removed.
>>>
>>> Perform the lookup in either helper_access_check_cp_reg,
>>> or a new helper_lookup_cp_reg.
>> As well as the use-after-free, this is also a correctness
>> bug, isn't it? If we hardwire in the cpregs pointer for
>> CPU 0 into the TB, and then CPU 1 with a slightly different
>> config executes the TB, it will get the cpregs of CPU 0,
>> not its own, so it might see a register it should not or
>> vice-versa.
>
> Existing assumption was that each cpu configuration would have its own
> cluster_index, which gets encoded into cpu->tcg_cflags, which is part
> of the comparison used when hashing TBs.

I understand the cluster_index is used for things like A/R and A/M
splits (and eventually heterogeneous). We also have language to cover
the increasingly weird set of big.LITTLE offspring like M1's
Performance/Efficiency split or the Tensor's X1/A76/A55 split.

 * If CPUs are not identical (for example, Cortex-A53 and Cortex-A57 CPUs in an
 * Arm big.LITTLE system) they should be in different clusters. If the CPUs do
 * not have the same view of memory (for example the main CPU and a management
 * controller processor) they should be in different clusters.

> But including this patch allows relaxation of what constitutes a "cpu configuration".
>
>
> r~


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 0/2] target/arm: Look up ARMCPRegInfo at runtime

[Richard Henderson]

Ping.

r~

On 1/6/23 09:44, Richard Henderson wrote:
> Here's a short-to-medium term alternative to moving all of the ARMCPU
> cp_regs hash table to the ARMCPUClass, so that we're no longer leaving
> dangling pointers to freed objects encoded in the compiled
> TranslationBlocks.  (I still think we ought to do less work at
> object_{init,realize}, but that may be a much longer term project.)
> 
> Instead of giving the helper a direct pointer, pass the cpreg hash key,
> which will be constant across cpus.  Perform this lookup in the existing
> helper_access_check_cp_reg (which had a return value going spare), or a
> new helper_lookup_cp_reg.  The other cp_regs functions are unchanged,
> because they still get a pointer.
> 
> This ought to be enough to re-instate Alex's linux-user patch
> to free the cpu object after thread termination.
> 
> 
> r~
> 
> 
> Richard Henderson (2):
>    target/arm: Reorg do_coproc_insn
>    target/arm: Look up ARMCPRegInfo at runtime
> 
>   target/arm/helper.h        |  11 +-
>   target/arm/translate.h     |   7 +
>   target/arm/op_helper.c     |  27 ++-
>   target/arm/translate-a64.c |  49 +++--
>   target/arm/translate.c     | 430 +++++++++++++++++++------------------
>   5 files changed, 285 insertions(+), 239 deletions(-)
> 

Re: [PATCH 0/2] target/arm: Look up ARMCPRegInfo at runtime

[Peter Maydell]

On Mon, 16 Jan 2023 at 20:16, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Ping.

What did you think of my suggestion in the other thread of hashing
the info we need to determine the cpreg set (ID regs, feature flags,
etc) and using that to look up whether we've already created a
cpreg hashtable for this config? If we did that, we could refcount
the cpreg hashtable and only free it when all CPUs are done with
it, which would mean we don't need this indirection.

thanks
-- PMM

Re: [PATCH 0/2] target/arm: Look up ARMCPRegInfo at runtime

[Richard Henderson]

On 1/17/23 00:28, Peter Maydell wrote:
> On Mon, 16 Jan 2023 at 20:16, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Ping.
> 
> What did you think of my suggestion in the other thread of hashing
> the info we need to determine the cpreg set (ID regs, feature flags,
> etc) and using that to look up whether we've already created a
> cpreg hashtable for this config? If we did that, we could refcount
> the cpreg hashtable and only free it when all CPUs are done with
> it, which would mean we don't need this indirection.

I thought it a decent idea, but not small.

While we currently have a struct for some isar regs, we'd want a larger struct containing 
every bit of info that wants hashing.  I think it would take 40-50 patches to get all of 
the properties etc moved out of ARMCPU and CPUARMState.

Anyway, I didn't want to leave the user-only thread leak blocked in the meantime.


r~

Re: [PATCH 0/2] target/arm: Look up ARMCPRegInfo at runtime

[Peter Maydell]

On Fri, 6 Jan 2023 at 19:45, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Here's a short-to-medium term alternative to moving all of the ARMCPU
> cp_regs hash table to the ARMCPUClass, so that we're no longer leaving
> dangling pointers to freed objects encoded in the compiled
> TranslationBlocks.  (I still think we ought to do less work at
> object_{init,realize}, but that may be a much longer term project.)
>
> Instead of giving the helper a direct pointer, pass the cpreg hash key,
> which will be constant across cpus.  Perform this lookup in the existing
> helper_access_check_cp_reg (which had a return value going spare), or a
> new helper_lookup_cp_reg.  The other cp_regs functions are unchanged,
> because they still get a pointer.
>
> This ought to be enough to re-instate Alex's linux-user patch
> to free the cpu object after thread termination.



Applied to target-arm.next, thanks.

-- PMM