The HCR.TID3 bit defines that we should trap to the hypervisor for
reads to a collection of ID registers. Different architecture versions
have defined this differently:
* v7A has a set of ID regs that definitely must trap:
- ID_PFR{0,1}, ID_DFR0, ID_AFR0, ID_MMFR{0,1,2,3},
ID_ISAR{0,1,2,3,4,5}, MVFR{0,1}
and somewhat vaguely says that "there is no requirement"
to trap for registers that are reserved in the ID reg space
(i.e. which RAZ and might be used for new ID regs in future)
* v8A adds to this list:
- ID_PFR2 and MVFR2 must trap
- ID_MMFR4, ID_MMFR5, ID_ISAR6, ID_DFR1 and reserved registers
in the ID reg space must trap if FEAT_FGT is implemented,
and it is IMPDEF if they trap if FEAT_FGT is not implemented
In QEMU we seem to have attempted to implement this distinction
(taking the "we do trap" IMPDEF choice if no FEAT_FGT), with
access_aa64_tid3() always trapping on TID3 and access_aa32_tid3()
trapping only if ARM_FEATURE_V8 is set. However, we didn't apply
these to the right set of registers: we use access_aa32_tid3() on all
the 32-bit ID registers *except* ID_PFR2, ID_DFR1, ID_MMFR5 and the
RES0 space, which means that for a v7 CPU we don't trap on a lot of
registers that we should trap on, and we do trap on various things
that the v7A Arm ARM says there is "no requirement" to trap on.
Straighten this out by naming the access functions more clearly for
their purpose, and documenting this: access_v7_tid3() is only for the
fixed set of ID registers that v7A traps on HCR.TID3, and
access_tid3() is for any others, including the reserved encoding
spaces and any new registers we add in future.
AArch32 MVFR2 access is handled differently, in check_hcr_el2_trap;
there we already do not trap on TID3 on v7A cores (where MVFR2
doesn't exist), because we in the code-generation function we UNDEF
if ARM_FEATURE_V8 is not set, without generating code to call
check_hcr_el2_trap.
This bug was causing a problem for Xen which (after a recent change
to Xen) expects to be able to trap ID_PFR0 on a Cortex-A15.
The result of these changes is that our v8A behaviour remains
the same, and on v7A we now trap the registers the Arm ARM definitely
requires us to trap, and don't trap the reserved space that "there is
no requirement" to trap.
Cc: qemu-stable@nongnu.org
Fixes: 6a4ef4e5d1084c ("target/arm: Honor HCR_EL2.TID3 trapping requirements")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/helper.c | 146 ++++++++++++++++++++++++--------------------
1 file changed, 81 insertions(+), 65 deletions(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index ec82ea6203..c4f73eb3f3 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5835,9 +5835,18 @@ static const ARMCPRegInfo ccsidr2_reginfo[] = {
.readfn = ccsidr2_read, .type = ARM_CP_NO_RAW },
};
-static CPAccessResult access_aa64_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
- bool isread)
+static CPAccessResult access_v7_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
+ bool isread)
{
+ /*
+ * Trap on TID3 always. This should be used only for the fixed set of
+ * registers which are defined to trap on HCR.TID3 in v7A, which is:
+ * ID_PFR{0,1}, ID_DFR0, ID_AFR0, ID_MMFR{0,1,2,3}, ID_ISAR{0,1,2,3,4,5}
+ * (MVFR0 and MVFR1 also trap in v7A, but this is not handled via
+ * this accessfn but in check_hcr_el2_trap.)
+ * Any other registers in the TID3 trap space should use access_tid3(),
+ * so that they trap on v8 and above, but not on v7.
+ */
if ((arm_current_el(env) < 2) && (arm_hcr_el2_eff(env) & HCR_TID3)) {
return CP_ACCESS_TRAP_EL2;
}
@@ -5845,11 +5854,18 @@ static CPAccessResult access_aa64_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
return CP_ACCESS_OK;
}
-static CPAccessResult access_aa32_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
- bool isread)
+static CPAccessResult access_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
+ bool isread)
{
+ /*
+ * Trap on TID3, if we implement at least v8. For v8 and above
+ * the ID register space is at least IMPDEF permitted to trap,
+ * and must trap if FEAT_FGT is implemented. We choose to trap
+ * always. Use this function for any new registers that should
+ * trap on TID3.
+ */
if (arm_feature(env, ARM_FEATURE_V8)) {
- return access_aa64_tid3(env, ri, isread);
+ return access_v7_tid3(env, ri, isread);
}
return CP_ACCESS_OK;
@@ -6287,7 +6303,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
{ .name = "ID_PFR0", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa32_tid3,
+ .accessfn = access_v7_tid3,
.resetvalue = GET_IDREG(isar, ID_PFR0)},
/*
* ID_PFR1 is not a plain ARM_CP_CONST because we don't know
@@ -6301,7 +6317,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
.resetvalue = GET_IDREG(isar, ID_PFR1),
#else
.type = ARM_CP_NO_RAW,
- .accessfn = access_aa32_tid3,
+ .accessfn = access_v7_tid3,
.readfn = id_pfr1_read,
.writefn = arm_cp_write_ignore
#endif
@@ -6309,72 +6325,72 @@ void register_cp_regs_for_features(ARMCPU *cpu)
{ .name = "ID_DFR0", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 2,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa32_tid3,
+ .accessfn = access_v7_tid3,
.resetvalue = GET_IDREG(isar, ID_DFR0)},
{ .name = "ID_AFR0", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 3,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa32_tid3,
+ .accessfn = access_v7_tid3,
.resetvalue = GET_IDREG(isar, ID_AFR0)},
{ .name = "ID_MMFR0", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 4,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa32_tid3,
+ .accessfn = access_v7_tid3,
.resetvalue = GET_IDREG(isar, ID_MMFR0)},
{ .name = "ID_MMFR1", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 5,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa32_tid3,
+ .accessfn = access_v7_tid3,
.resetvalue = GET_IDREG(isar, ID_MMFR1)},
{ .name = "ID_MMFR2", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 6,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa32_tid3,
+ .accessfn = access_v7_tid3,
.resetvalue = GET_IDREG(isar, ID_MMFR2)},
{ .name = "ID_MMFR3", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 7,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa32_tid3,
+ .accessfn = access_v7_tid3,
.resetvalue = GET_IDREG(isar, ID_MMFR3)},
{ .name = "ID_ISAR0", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 0,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa32_tid3,
+ .accessfn = access_v7_tid3,
.resetvalue = GET_IDREG(isar, ID_ISAR0)},
{ .name = "ID_ISAR1", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 1,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa32_tid3,
+ .accessfn = access_v7_tid3,
.resetvalue = GET_IDREG(isar, ID_ISAR1)},
{ .name = "ID_ISAR2", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 2,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa32_tid3,
+ .accessfn = access_v7_tid3,
.resetvalue = GET_IDREG(isar, ID_ISAR2)},
{ .name = "ID_ISAR3", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 3,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa32_tid3,
+ .accessfn = access_v7_tid3,
.resetvalue = GET_IDREG(isar, ID_ISAR3) },
{ .name = "ID_ISAR4", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 4,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa32_tid3,
+ .accessfn = access_v7_tid3,
.resetvalue = GET_IDREG(isar, ID_ISAR4) },
{ .name = "ID_ISAR5", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 5,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa32_tid3,
+ .accessfn = access_v7_tid3,
.resetvalue = GET_IDREG(isar, ID_ISAR5) },
{ .name = "ID_MMFR4", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 6,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa32_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_MMFR4)},
{ .name = "ID_ISAR6", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 7,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa32_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_ISAR6) },
};
define_arm_cp_regs(cpu, v6_idregs);
@@ -6425,7 +6441,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
.resetvalue = GET_IDREG(isar, ID_AA64PFR0)
#else
.type = ARM_CP_NO_RAW,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.readfn = id_aa64pfr0_read,
.writefn = arm_cp_write_ignore
#endif
@@ -6433,172 +6449,172 @@ void register_cp_regs_for_features(ARMCPU *cpu)
{ .name = "ID_AA64PFR1_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 1,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_AA64PFR1)},
{ .name = "ID_AA64PFR2_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 2,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_AA64PFR2)},
{ .name = "ID_AA64PFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 3,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "ID_AA64ZFR0_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 4,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_AA64ZFR0)},
{ .name = "ID_AA64SMFR0_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 5,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_AA64SMFR0)},
{ .name = "ID_AA64PFR6_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 6,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "ID_AA64PFR7_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 7,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "ID_AA64DFR0_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 0,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_AA64DFR0) },
{ .name = "ID_AA64DFR1_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_AA64DFR1) },
{ .name = "ID_AA64DFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 2,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "ID_AA64DFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 3,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "ID_AA64AFR0_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 4,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_AA64AFR0) },
{ .name = "ID_AA64AFR1_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 5,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_AA64AFR1) },
{ .name = "ID_AA64AFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 6,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "ID_AA64AFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 7,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "ID_AA64ISAR0_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 0,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_AA64ISAR0)},
{ .name = "ID_AA64ISAR1_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 1,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_AA64ISAR1)},
{ .name = "ID_AA64ISAR2_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 2,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_AA64ISAR2)},
{ .name = "ID_AA64ISAR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 3,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "ID_AA64ISAR4_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 4,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "ID_AA64ISAR5_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 5,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "ID_AA64ISAR6_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 6,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "ID_AA64ISAR7_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 7,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "ID_AA64MMFR0_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 0,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_AA64MMFR0)},
{ .name = "ID_AA64MMFR1_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 1,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_AA64MMFR1) },
{ .name = "ID_AA64MMFR2_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 2,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_AA64MMFR2) },
{ .name = "ID_AA64MMFR3_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 3,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_AA64MMFR3) },
{ .name = "ID_AA64MMFR4_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 4,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "ID_AA64MMFR5_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 5,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "ID_AA64MMFR6_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 6,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "ID_AA64MMFR7_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 7,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "MVFR0_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 0,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = cpu->isar.mvfr0 },
{ .name = "MVFR1_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 1,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = cpu->isar.mvfr1 },
{ .name = "MVFR2_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 2,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = cpu->isar.mvfr2 },
/*
* "0, c0, c3, {0,1,2}" are the encodings corresponding to
@@ -6609,17 +6625,17 @@ void register_cp_regs_for_features(ARMCPU *cpu)
{ .name = "RES_0_C0_C3_0", .state = ARM_CP_STATE_AA32,
.cp = 15, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 0,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "RES_0_C0_C3_1", .state = ARM_CP_STATE_AA32,
.cp = 15, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 1,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "RES_0_C0_C3_2", .state = ARM_CP_STATE_AA32,
.cp = 15, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 2,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
/*
* Other encodings in "0, c0, c3, ..." are STATE_BOTH because
@@ -6630,27 +6646,27 @@ void register_cp_regs_for_features(ARMCPU *cpu)
{ .name = "RES_0_C0_C3_3", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 3,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
{ .name = "ID_PFR2", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 4,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_PFR2)},
{ .name = "ID_DFR1", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 5,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_DFR1)},
{ .name = "ID_MMFR5", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 6,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = GET_IDREG(isar, ID_MMFR5)},
{ .name = "RES_0_C0_C3_7", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 7,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 },
};
#ifdef CONFIG_USER_ONLY
@@ -6800,7 +6816,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
.state = ARM_CP_STATE_AA32,
.cp = 15, .opc1 = 0, .crn = 0, .crm = i, .opc2 = CP_ANY,
.access = PL1_R, .type = ARM_CP_CONST,
- .accessfn = access_aa64_tid3,
+ .accessfn = access_tid3,
.resetvalue = 0 };
define_one_arm_cp_reg(cpu, &v8_aa32_raz_idregs);
}
--
2.47.3On 1/1/26 04:08, Peter Maydell wrote:
> The HCR.TID3 bit defines that we should trap to the hypervisor for
> reads to a collection of ID registers. Different architecture versions
> have defined this differently:
>
> * v7A has a set of ID regs that definitely must trap:
> - ID_PFR{0,1}, ID_DFR0, ID_AFR0, ID_MMFR{0,1,2,3},
> ID_ISAR{0,1,2,3,4,5}, MVFR{0,1}
> and somewhat vaguely says that "there is no requirement"
> to trap for registers that are reserved in the ID reg space
> (i.e. which RAZ and might be used for new ID regs in future)
> * v8A adds to this list:
> - ID_PFR2 and MVFR2 must trap
> - ID_MMFR4, ID_MMFR5, ID_ISAR6, ID_DFR1 and reserved registers
> in the ID reg space must trap if FEAT_FGT is implemented,
> and it is IMPDEF if they trap if FEAT_FGT is not implemented
>
> In QEMU we seem to have attempted to implement this distinction
> (taking the "we do trap" IMPDEF choice if no FEAT_FGT), with
> access_aa64_tid3() always trapping on TID3 and access_aa32_tid3()
> trapping only if ARM_FEATURE_V8 is set. However, we didn't apply
> these to the right set of registers: we use access_aa32_tid3() on all
> the 32-bit ID registers*except* ID_PFR2, ID_DFR1, ID_MMFR5 and the
> RES0 space, which means that for a v7 CPU we don't trap on a lot of
> registers that we should trap on, and we do trap on various things
> that the v7A Arm ARM says there is "no requirement" to trap on.
>
> Straighten this out by naming the access functions more clearly for
> their purpose, and documenting this: access_v7_tid3() is only for the
> fixed set of ID registers that v7A traps on HCR.TID3, and
> access_tid3() is for any others, including the reserved encoding
> spaces and any new registers we add in future.
>
> AArch32 MVFR2 access is handled differently, in check_hcr_el2_trap;
> there we already do not trap on TID3 on v7A cores (where MVFR2
> doesn't exist), because we in the code-generation function we UNDEF
> if ARM_FEATURE_V8 is not set, without generating code to call
> check_hcr_el2_trap.
>
> This bug was causing a problem for Xen which (after a recent change
> to Xen) expects to be able to trap ID_PFR0 on a Cortex-A15.
>
> The result of these changes is that our v8A behaviour remains
> the same, and on v7A we now trap the registers the Arm ARM definitely
> requires us to trap, and don't trap the reserved space that "there is
> no requirement" to trap.
>
> Cc:qemu-stable@nongnu.org
> Fixes: 6a4ef4e5d1084c ("target/arm: Honor HCR_EL2.TID3 trapping requirements")
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> target/arm/helper.c | 146 ++++++++++++++++++++++++--------------------
> 1 file changed, 81 insertions(+), 65 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
Peter Maydell <peter.maydell@linaro.org> writes:
> The HCR.TID3 bit defines that we should trap to the hypervisor for
> reads to a collection of ID registers. Different architecture versions
> have defined this differently:
>
> * v7A has a set of ID regs that definitely must trap:
> - ID_PFR{0,1}, ID_DFR0, ID_AFR0, ID_MMFR{0,1,2,3},
> ID_ISAR{0,1,2,3,4,5}, MVFR{0,1}
> and somewhat vaguely says that "there is no requirement"
> to trap for registers that are reserved in the ID reg space
> (i.e. which RAZ and might be used for new ID regs in future)
> * v8A adds to this list:
> - ID_PFR2 and MVFR2 must trap
> - ID_MMFR4, ID_MMFR5, ID_ISAR6, ID_DFR1 and reserved registers
> in the ID reg space must trap if FEAT_FGT is implemented,
> and it is IMPDEF if they trap if FEAT_FGT is not implemented
>
> In QEMU we seem to have attempted to implement this distinction
> (taking the "we do trap" IMPDEF choice if no FEAT_FGT), with
> access_aa64_tid3() always trapping on TID3 and access_aa32_tid3()
> trapping only if ARM_FEATURE_V8 is set. However, we didn't apply
> these to the right set of registers: we use access_aa32_tid3() on all
> the 32-bit ID registers *except* ID_PFR2, ID_DFR1, ID_MMFR5 and the
> RES0 space, which means that for a v7 CPU we don't trap on a lot of
> registers that we should trap on, and we do trap on various things
> that the v7A Arm ARM says there is "no requirement" to trap on.
>
> Straighten this out by naming the access functions more clearly for
> their purpose, and documenting this: access_v7_tid3() is only for the
> fixed set of ID registers that v7A traps on HCR.TID3, and
> access_tid3() is for any others, including the reserved encoding
> spaces and any new registers we add in future.
I'm confused by the naming - especially as searching the Arm doc site
with the Armv7-A filter didn't show up an HCR register (although it does
show up in the PDF).
I guess what you are saying is these registers trap from v7a onwards?
v8/v9 don't change the trapping on this subset of registers.
>
> AArch32 MVFR2 access is handled differently, in check_hcr_el2_trap;
> there we already do not trap on TID3 on v7A cores (where MVFR2
> doesn't exist), because we in the code-generation function we UNDEF
> if ARM_FEATURE_V8 is not set, without generating code to call
> check_hcr_el2_trap.
>
> This bug was causing a problem for Xen which (after a recent change
> to Xen) expects to be able to trap ID_PFR0 on a Cortex-A15.
>
> The result of these changes is that our v8A behaviour remains
> the same, and on v7A we now trap the registers the Arm ARM definitely
> requires us to trap, and don't trap the reserved space that "there is
> no requirement" to trap.
>
> Cc: qemu-stable@nongnu.org
> Fixes: 6a4ef4e5d1084c ("target/arm: Honor HCR_EL2.TID3 trapping requirements")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> target/arm/helper.c | 146 ++++++++++++++++++++++++--------------------
> 1 file changed, 81 insertions(+), 65 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index ec82ea6203..c4f73eb3f3 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5835,9 +5835,18 @@ static const ARMCPRegInfo ccsidr2_reginfo[] = {
> .readfn = ccsidr2_read, .type = ARM_CP_NO_RAW },
> };
>
> -static CPAccessResult access_aa64_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
> - bool isread)
> +static CPAccessResult access_v7_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
> + bool isread)
> {
> + /*
> + * Trap on TID3 always. This should be used only for the fixed set of
> + * registers which are defined to trap on HCR.TID3 in v7A, which is:
> + * ID_PFR{0,1}, ID_DFR0, ID_AFR0, ID_MMFR{0,1,2,3}, ID_ISAR{0,1,2,3,4,5}
> + * (MVFR0 and MVFR1 also trap in v7A, but this is not handled via
> + * this accessfn but in check_hcr_el2_trap.)
> + * Any other registers in the TID3 trap space should use access_tid3(),
> + * so that they trap on v8 and above, but not on v7.
> + */
> if ((arm_current_el(env) < 2) && (arm_hcr_el2_eff(env) & HCR_TID3)) {
> return CP_ACCESS_TRAP_EL2;
> }
> @@ -5845,11 +5854,18 @@ static CPAccessResult access_aa64_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
> return CP_ACCESS_OK;
> }
>
> -static CPAccessResult access_aa32_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
> - bool isread)
> +static CPAccessResult access_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
> + bool isread)
> {
> + /*
> + * Trap on TID3, if we implement at least v8. For v8 and above
> + * the ID register space is at least IMPDEF permitted to trap,
> + * and must trap if FEAT_FGT is implemented. We choose to trap
> + * always. Use this function for any new registers that should
> + * trap on TID3.
> + */
> if (arm_feature(env, ARM_FEATURE_V8)) {
> - return access_aa64_tid3(env, ri, isread);
> + return access_v7_tid3(env, ri, isread);
This seems even more incongruous - we test for v8 but use the v7 helper.
<snip>
I think I understand whats happening but it is confusing to follow.
Naming things is hard.
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
On Fri, 2 Jan 2026 at 11:17, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > The HCR.TID3 bit defines that we should trap to the hypervisor for
> > reads to a collection of ID registers. Different architecture versions
> > have defined this differently:
> >
> > * v7A has a set of ID regs that definitely must trap:
> > - ID_PFR{0,1}, ID_DFR0, ID_AFR0, ID_MMFR{0,1,2,3},
> > ID_ISAR{0,1,2,3,4,5}, MVFR{0,1}
> > and somewhat vaguely says that "there is no requirement"
> > to trap for registers that are reserved in the ID reg space
> > (i.e. which RAZ and might be used for new ID regs in future)
> > * v8A adds to this list:
> > - ID_PFR2 and MVFR2 must trap
> > - ID_MMFR4, ID_MMFR5, ID_ISAR6, ID_DFR1 and reserved registers
> > in the ID reg space must trap if FEAT_FGT is implemented,
> > and it is IMPDEF if they trap if FEAT_FGT is not implemented
> >
> > In QEMU we seem to have attempted to implement this distinction
> > (taking the "we do trap" IMPDEF choice if no FEAT_FGT), with
> > access_aa64_tid3() always trapping on TID3 and access_aa32_tid3()
> > trapping only if ARM_FEATURE_V8 is set. However, we didn't apply
> > these to the right set of registers: we use access_aa32_tid3() on all
> > the 32-bit ID registers *except* ID_PFR2, ID_DFR1, ID_MMFR5 and the
> > RES0 space, which means that for a v7 CPU we don't trap on a lot of
> > registers that we should trap on, and we do trap on various things
> > that the v7A Arm ARM says there is "no requirement" to trap on.
> >
> > Straighten this out by naming the access functions more clearly for
> > their purpose, and documenting this: access_v7_tid3() is only for the
> > fixed set of ID registers that v7A traps on HCR.TID3, and
> > access_tid3() is for any others, including the reserved encoding
> > spaces and any new registers we add in future.
>
> I'm confused by the naming - especially as searching the Arm doc site
> with the Armv7-A filter didn't show up an HCR register (although it does
> show up in the PDF).
Not sure why it wouldn't show up -- this is the main hypervisor
trap configuration register, and it's always been HCR for AArch32
and HCR_EL2 for AArch64.
> I guess what you are saying is these registers trap from v7a onwards?
> v8/v9 don't change the trapping on this subset of registers.
I tried to lay this out in the commit message, but basically what
we have is that this trap bit is trapping a set of the ID registers.
In v7A it was specified to trap the ID registers that were defined
at that time, but not the encodings that were reserved in the
ID register space. (As ID register space, 'reserved' means RAZ,
not UNDEF as it would elsewhere in the system register space.)
In v8A some new ID registers were defined in what was previously
the reserved space, and so v8A says that TID3 must trap those also
(and that it IMPDEF is allowed to trap the rest of the reserved space).
Finally, FEAT_FGT fixes up the unhelpful IMPDEF variability by mandating
trapping on the whole space, reserved or not.
(Before v7A HCR didn't exist at all and these ID registers never
trap anywhere: since HCR.TID3 in our implementation will always
be 0 on CPUs without EL2, we don't need to special case "doesn't
actually have Hyp mode" in the access functions.)
> > -static CPAccessResult access_aa64_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
> > - bool isread)
> > +static CPAccessResult access_v7_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
> > + bool isread)
> > {
> > + /*
> > + * Trap on TID3 always. This should be used only for the fixed set of
> > + * registers which are defined to trap on HCR.TID3 in v7A, which is:
> > + * ID_PFR{0,1}, ID_DFR0, ID_AFR0, ID_MMFR{0,1,2,3}, ID_ISAR{0,1,2,3,4,5}
> > + * (MVFR0 and MVFR1 also trap in v7A, but this is not handled via
> > + * this accessfn but in check_hcr_el2_trap.)
> > + * Any other registers in the TID3 trap space should use access_tid3(),
> > + * so that they trap on v8 and above, but not on v7.
> > + */
> > if ((arm_current_el(env) < 2) && (arm_hcr_el2_eff(env) & HCR_TID3)) {
> > return CP_ACCESS_TRAP_EL2;
> > }
> > @@ -5845,11 +5854,18 @@ static CPAccessResult access_aa64_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
> > return CP_ACCESS_OK;
> > }
> >
> > -static CPAccessResult access_aa32_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
> > - bool isread)
> > +static CPAccessResult access_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
> > + bool isread)
> > {
> > + /*
> > + * Trap on TID3, if we implement at least v8. For v8 and above
> > + * the ID register space is at least IMPDEF permitted to trap,
> > + * and must trap if FEAT_FGT is implemented. We choose to trap
> > + * always. Use this function for any new registers that should
> > + * trap on TID3.
> > + */
> > if (arm_feature(env, ARM_FEATURE_V8)) {
> > - return access_aa64_tid3(env, ri, isread);
> > + return access_v7_tid3(env, ri, isread);
>
> This seems even more incongruous - we test for v8 but use the v7 helper.
We have two different things we want to do:
(1) always trap if TID3 is set
(2) trap if we are v8 or better and TID3 is set
We want to use function 1 for the set of ID registers that
existed back in v7A -- these are the ones that trap for any
implementation.
We want function 2 for the ID registers that were only defined
in v8A, and for the reserved-ID-register space. These must not
trap on v7A, and either must or are IMPDEF-permitted to trap
on v8A and later.
I have tried to pick function names that make sense for "what
is this doing", and for "if somebody adds a new register here,
make the function they want be the one with a name that seems
most inviting, so they pick the right one, not the wrong one".
So I have access_v7_tid3 for ID registers defined in
v7, and access_tid3 for the rest, including any new ones.
This seemed to me better than picking a function name that
describes the internal implementation of functions 1 and 2,
on the basis that people are a lot more likely to need to
use the functions than look inside them.
If you have suggested names that you think make more sense,
I'm open to them -- since I started by knowing the behaviour
to me the names I ended up with seem more "obvious" to me than
they would to somebody else, and it's the "somebody else" that
I'm trying to help with the naming...
Separately, the implementation of function (2) in this patch
is "if ARM_FEATURE_V8 is set, call function (1) to do the test-TID3,
otherwise return ACCESS_OK to indicate don't-trap". (Inherited
from the existing implementation choice.)
Given that function (1) is only a simple test of
"((arm_current_el(env) < 2) && (arm_hcr_el2_eff(env) & HCR_TID3))"
we could alternatively open-code that check in function (2)
if you think that would be clearer.
thanks
-- PMM
Peter Maydell <peter.maydell@linaro.org> writes:
> On Fri, 2 Jan 2026 at 11:17, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> > The HCR.TID3 bit defines that we should trap to the hypervisor for
>> > reads to a collection of ID registers. Different architecture versions
>> > have defined this differently:
>> >
>> > * v7A has a set of ID regs that definitely must trap:
>> > - ID_PFR{0,1}, ID_DFR0, ID_AFR0, ID_MMFR{0,1,2,3},
>> > ID_ISAR{0,1,2,3,4,5}, MVFR{0,1}
>> > and somewhat vaguely says that "there is no requirement"
>> > to trap for registers that are reserved in the ID reg space
>> > (i.e. which RAZ and might be used for new ID regs in future)
>> > * v8A adds to this list:
>> > - ID_PFR2 and MVFR2 must trap
>> > - ID_MMFR4, ID_MMFR5, ID_ISAR6, ID_DFR1 and reserved registers
>> > in the ID reg space must trap if FEAT_FGT is implemented,
>> > and it is IMPDEF if they trap if FEAT_FGT is not implemented
>> >
>> > In QEMU we seem to have attempted to implement this distinction
>> > (taking the "we do trap" IMPDEF choice if no FEAT_FGT), with
>> > access_aa64_tid3() always trapping on TID3 and access_aa32_tid3()
>> > trapping only if ARM_FEATURE_V8 is set. However, we didn't apply
>> > these to the right set of registers: we use access_aa32_tid3() on all
>> > the 32-bit ID registers *except* ID_PFR2, ID_DFR1, ID_MMFR5 and the
>> > RES0 space, which means that for a v7 CPU we don't trap on a lot of
>> > registers that we should trap on, and we do trap on various things
>> > that the v7A Arm ARM says there is "no requirement" to trap on.
>> >
>> > Straighten this out by naming the access functions more clearly for
>> > their purpose, and documenting this: access_v7_tid3() is only for the
>> > fixed set of ID registers that v7A traps on HCR.TID3, and
>> > access_tid3() is for any others, including the reserved encoding
>> > spaces and any new registers we add in future.
>>
>> I'm confused by the naming - especially as searching the Arm doc site
>> with the Armv7-A filter didn't show up an HCR register (although it does
>> show up in the PDF).
>
> Not sure why it wouldn't show up -- this is the main hypervisor
> trap configuration register, and it's always been HCR for AArch32
> and HCR_EL2 for AArch64.
>
>> I guess what you are saying is these registers trap from v7a onwards?
>> v8/v9 don't change the trapping on this subset of registers.
>
> I tried to lay this out in the commit message, but basically what
> we have is that this trap bit is trapping a set of the ID registers.
> In v7A it was specified to trap the ID registers that were defined
> at that time, but not the encodings that were reserved in the
> ID register space. (As ID register space, 'reserved' means RAZ,
> not UNDEF as it would elsewhere in the system register space.)
> In v8A some new ID registers were defined in what was previously
> the reserved space, and so v8A says that TID3 must trap those also
> (and that it IMPDEF is allowed to trap the rest of the reserved space).
> Finally, FEAT_FGT fixes up the unhelpful IMPDEF variability by mandating
> trapping on the whole space, reserved or not.
>
> (Before v7A HCR didn't exist at all and these ID registers never
> trap anywhere: since HCR.TID3 in our implementation will always
> be 0 on CPUs without EL2, we don't need to special case "doesn't
> actually have Hyp mode" in the access functions.)
>
>> > -static CPAccessResult access_aa64_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
>> > - bool isread)
>> > +static CPAccessResult access_v7_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
>> > + bool isread)
>> > {
>> > + /*
>> > + * Trap on TID3 always. This should be used only for the fixed set of
>> > + * registers which are defined to trap on HCR.TID3 in v7A, which is:
>> > + * ID_PFR{0,1}, ID_DFR0, ID_AFR0, ID_MMFR{0,1,2,3}, ID_ISAR{0,1,2,3,4,5}
>> > + * (MVFR0 and MVFR1 also trap in v7A, but this is not handled via
>> > + * this accessfn but in check_hcr_el2_trap.)
>> > + * Any other registers in the TID3 trap space should use access_tid3(),
>> > + * so that they trap on v8 and above, but not on v7.
>> > + */
>> > if ((arm_current_el(env) < 2) && (arm_hcr_el2_eff(env) & HCR_TID3)) {
>> > return CP_ACCESS_TRAP_EL2;
>> > }
>> > @@ -5845,11 +5854,18 @@ static CPAccessResult access_aa64_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
>> > return CP_ACCESS_OK;
>> > }
>> >
>> > -static CPAccessResult access_aa32_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
>> > - bool isread)
>> > +static CPAccessResult access_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
>> > + bool isread)
>> > {
>> > + /*
>> > + * Trap on TID3, if we implement at least v8. For v8 and above
>> > + * the ID register space is at least IMPDEF permitted to trap,
>> > + * and must trap if FEAT_FGT is implemented. We choose to trap
>> > + * always. Use this function for any new registers that should
>> > + * trap on TID3.
>> > + */
>> > if (arm_feature(env, ARM_FEATURE_V8)) {
>> > - return access_aa64_tid3(env, ri, isread);
>> > + return access_v7_tid3(env, ri, isread);
>>
>> This seems even more incongruous - we test for v8 but use the v7 helper.
>
> We have two different things we want to do:
>
> (1) always trap if TID3 is set
> (2) trap if we are v8 or better and TID3 is set
>
> We want to use function 1 for the set of ID registers that
> existed back in v7A -- these are the ones that trap for any
> implementation.
> We want function 2 for the ID registers that were only defined
> in v8A, and for the reserved-ID-register space. These must not
> trap on v7A, and either must or are IMPDEF-permitted to trap
> on v8A and later.
>
> I have tried to pick function names that make sense for "what
> is this doing", and for "if somebody adds a new register here,
> make the function they want be the one with a name that seems
> most inviting, so they pick the right one, not the wrong one".
> So I have access_v7_tid3 for ID registers defined in
> v7, and access_tid3 for the rest, including any new ones.
> This seemed to me better than picking a function name that
> describes the internal implementation of functions 1 and 2,
> on the basis that people are a lot more likely to need to
> use the functions than look inside them.
>
> If you have suggested names that you think make more sense,
> I'm open to them -- since I started by knowing the behaviour
> to me the names I ended up with seem more "obvious" to me than
> they would to somebody else, and it's the "somebody else" that
> I'm trying to help with the naming...
I think I follow now. My only real suggestion would be to make the name
_v7a to be distinct from the v7m profile. Although HCR.TID3 seems to
exist for v7r as well.
Anyway:
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>
> Separately, the implementation of function (2) in this patch
> is "if ARM_FEATURE_V8 is set, call function (1) to do the test-TID3,
> otherwise return ACCESS_OK to indicate don't-trap". (Inherited
> from the existing implementation choice.)
>
> Given that function (1) is only a simple test of
> "((arm_current_el(env) < 2) && (arm_hcr_el2_eff(env) & HCR_TID3))"
> we could alternatively open-code that check in function (2)
> if you think that would be clearer.
>
> thanks
> -- PMM
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
On Thu, 15 Jan 2026 at 15:20, Alex Bennée <alex.bennee@linaro.org> wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > > If you have suggested names that you think make more sense, > > I'm open to them -- since I started by knowing the behaviour > > to me the names I ended up with seem more "obvious" to me than > > they would to somebody else, and it's the "somebody else" that > > I'm trying to help with the naming... > > I think I follow now. My only real suggestion would be to make the name > _v7a to be distinct from the v7m profile. Although HCR.TID3 seems to > exist for v7r as well. > > Anyway: > > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Thanks; I'll make that change and apply these to target-arm.next. FWIW, v7R can't have the virtualization extension, so it doesn't have HCR.TID3. Virtualization for R-profile only came in with v8R. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Thu, 15 Jan 2026 at 15:20, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> Peter Maydell <peter.maydell@linaro.org> writes: >> > If you have suggested names that you think make more sense, >> > I'm open to them -- since I started by knowing the behaviour >> > to me the names I ended up with seem more "obvious" to me than >> > they would to somebody else, and it's the "somebody else" that >> > I'm trying to help with the naming... >> >> I think I follow now. My only real suggestion would be to make the name >> _v7a to be distinct from the v7m profile. Although HCR.TID3 seems to >> exist for v7r as well. >> >> Anyway: >> >> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > > Thanks; I'll make that change and apply these to > target-arm.next. > > FWIW, v7R can't have the virtualization extension, so > it doesn't have HCR.TID3. Virtualization for R-profile > only came in with v8R. Ahh found it described as "OPTIONAL set of extensions to VMSAv7" - so as a subset of the memory model architecture. I guess because v7r is PMSAv7. So many acronyms ;-) > > thanks > -- PMM -- Alex Bennée Virtualisation Tech Lead @ Linaro
© 2016 - 2026 Red Hat, Inc.