On Wed, 27 Aug 2025 04:04, Richard Henderson <richard.henderson@linaro.org> wrote:
>The last use of this interface was removed in 603bc048a27f
>("hw/arm: Remove pxa2xx_pic"). As the comment in gicv3
>stated, keeping pointer references to cpregs has SMP issues,
>so avoid future temptation by removing the interface.
>
>Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>---
Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> target/arm/cpregs.h | 32 ++++++++------------------------
> hw/intc/arm_gicv3_cpuif.c | 10 +---------
> target/arm/helper.c | 29 +++++++++++------------------
> 3 files changed, 20 insertions(+), 51 deletions(-)
>
>diff --git a/target/arm/cpregs.h b/target/arm/cpregs.h
>index 3344a02bd3..b610716c24 100644
>--- a/target/arm/cpregs.h
>+++ b/target/arm/cpregs.h
>@@ -906,11 +906,7 @@ struct ARMCPRegInfo {
> */
> uint32_t nv2_redirect_offset;
>
>- /*
>- * The opaque pointer passed to define_arm_cp_regs_with_opaque() when
>- * this register was defined: can be used to hand data through to the
>- * register read/write functions, since they are passed the ARMCPRegInfo*.
>- */
>+ /* This is used only by VHE. */
> void *opaque;
> /*
> * Value of this register, if it is ARM_CP_CONST. Otherwise, if
>@@ -1004,27 +1000,15 @@ struct ARMCPRegInfo {
> #define CPREG_FIELD64(env, ri) \
> (*(uint64_t *)((char *)(env) + (ri)->fieldoffset))
>
>-void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu, const ARMCPRegInfo *reg,
>- void *opaque);
>+void define_one_arm_cp_reg(ARMCPU *cpu, const ARMCPRegInfo *regs);
>+void define_arm_cp_regs_len(ARMCPU *cpu, const ARMCPRegInfo *regs, size_t len);
>
>-static inline void define_one_arm_cp_reg(ARMCPU *cpu, const ARMCPRegInfo *regs)
>-{
>- define_one_arm_cp_reg_with_opaque(cpu, regs, NULL);
>-}
>-
>-void define_arm_cp_regs_with_opaque_len(ARMCPU *cpu, const ARMCPRegInfo *regs,
>- void *opaque, size_t len);
>-
>-#define define_arm_cp_regs_with_opaque(CPU, REGS, OPAQUE) \
>- do { \
>- QEMU_BUILD_BUG_ON(ARRAY_SIZE(REGS) == 0); \
>- define_arm_cp_regs_with_opaque_len(CPU, REGS, OPAQUE, \
>- ARRAY_SIZE(REGS)); \
>+#define define_arm_cp_regs(CPU, REGS) \
>+ do { \
>+ QEMU_BUILD_BUG_ON(ARRAY_SIZE(REGS) == 0); \
>+ define_arm_cp_regs_len(CPU, REGS, ARRAY_SIZE(REGS)); \
> } while (0)
>
>-#define define_arm_cp_regs(CPU, REGS) \
>- define_arm_cp_regs_with_opaque(CPU, REGS, NULL)
>-
> const ARMCPRegInfo *get_arm_cp_reginfo(GHashTable *cpregs, uint32_t encoded_cp);
>
> /*
>@@ -1143,7 +1127,7 @@ static inline bool arm_cpreg_traps_in_nv(const ARMCPRegInfo *ri)
> * means that the right set of registers is exactly those where
> * the opc1 field is 4 or 5. (You can see this also in the assert
> * we do that the opc1 field and the permissions mask line up in
>- * define_one_arm_cp_reg_with_opaque().)
>+ * define_one_arm_cp_reg().)
> * Checking the opc1 field is easier for us and avoids the problem
> * that we do not consistently use the right architectural names
> * for all sysregs, since we treat the name field as largely for debug.
>diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
>index 4b4cf09157..72e91f971a 100644
>--- a/hw/intc/arm_gicv3_cpuif.c
>+++ b/hw/intc/arm_gicv3_cpuif.c
>@@ -3037,15 +3037,7 @@ void gicv3_init_cpuif(GICv3State *s)
> * cpu->gic_pribits
> */
>
>- /* Note that we can't just use the GICv3CPUState as an opaque pointer
>- * in define_arm_cp_regs_with_opaque(), because when we're called back
>- * it might be with code translated by CPU 0 but run by CPU 1, in
>- * which case we'd get the wrong value.
>- * So instead we define the regs with no ri->opaque info, and
>- * get back to the GICv3CPUState from the CPUARMState.
>- *
>- * These CP regs callbacks can be called from either TCG or HVF code.
>- */
>+ /* These CP regs callbacks can be called from either TCG or HVF. */
> define_arm_cp_regs(cpu, gicv3_cpuif_reginfo);
>
> /*
>diff --git a/target/arm/helper.c b/target/arm/helper.c
>index e03cbc0394..35a176ea3b 100644
>--- a/target/arm/helper.c
>+++ b/target/arm/helper.c
>@@ -7256,12 +7256,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
> }
>
> /*
>- * Private utility function for define_one_arm_cp_reg_with_opaque():
>+ * Private utility function for define_one_arm_cp_reg():
> * add a single reginfo struct to the hash table.
> */
> static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
>- void *opaque, CPState state,
>- CPSecureState secstate,
>+ CPState state, CPSecureState secstate,
> int crm, int opc1, int opc2,
> const char *name)
> {
>@@ -7349,9 +7348,6 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
> r2->opc2 = opc2;
> r2->state = state;
> r2->secure = secstate;
>- if (opaque) {
>- r2->opaque = opaque;
>- }
>
> if (make_const) {
> /* This should not have been a very special register to begin. */
>@@ -7456,8 +7452,7 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
> }
>
>
>-void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
>- const ARMCPRegInfo *r, void *opaque)
>+void define_one_arm_cp_reg(ARMCPU *cpu, const ARMCPRegInfo *r)
> {
> /*
> * Define implementations of coprocessor registers.
>@@ -7616,7 +7611,7 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
> if (nxs_ri.fgt) {
> nxs_ri.fgt |= R_FGT_NXS_MASK;
> }
>- add_cpreg_to_hashtable(cpu, &nxs_ri, opaque, state,
>+ add_cpreg_to_hashtable(cpu, &nxs_ri, state,
> ARM_CP_SECSTATE_NS,
> crm, opc1, opc2, name);
> }
>@@ -7630,17 +7625,17 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
> switch (r->secure) {
> case ARM_CP_SECSTATE_S:
> case ARM_CP_SECSTATE_NS:
>- add_cpreg_to_hashtable(cpu, r, opaque, state,
>+ add_cpreg_to_hashtable(cpu, r, state,
> r->secure, crm, opc1, opc2,
> r->name);
> break;
> case ARM_CP_SECSTATE_BOTH:
> name = g_strdup_printf("%s_S", r->name);
>- add_cpreg_to_hashtable(cpu, r, opaque, state,
>+ add_cpreg_to_hashtable(cpu, r, state,
> ARM_CP_SECSTATE_S,
> crm, opc1, opc2, name);
> g_free(name);
>- add_cpreg_to_hashtable(cpu, r, opaque, state,
>+ add_cpreg_to_hashtable(cpu, r, state,
> ARM_CP_SECSTATE_NS,
> crm, opc1, opc2, r->name);
> break;
>@@ -7652,7 +7647,7 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
> * AArch64 registers get mapped to non-secure instance
> * of AArch32
> */
>- add_cpreg_to_hashtable(cpu, r, opaque, state,
>+ add_cpreg_to_hashtable(cpu, r, state,
> ARM_CP_SECSTATE_NS,
> crm, opc1, opc2, r->name);
> }
>@@ -7663,12 +7658,10 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
> }
>
> /* Define a whole list of registers */
>-void define_arm_cp_regs_with_opaque_len(ARMCPU *cpu, const ARMCPRegInfo *regs,
>- void *opaque, size_t len)
>+void define_arm_cp_regs_len(ARMCPU *cpu, const ARMCPRegInfo *regs, size_t len)
> {
>- size_t i;
>- for (i = 0; i < len; ++i) {
>- define_one_arm_cp_reg_with_opaque(cpu, regs + i, opaque);
>+ for (size_t i = 0; i < len; ++i) {
>+ define_one_arm_cp_reg(cpu, regs + i);
> }
> }
>
>--
>2.43.0
>
>