[PATCH 0/4] target/arm: Fix handling of HCR.TID* on v7A CPUs

[PATCH 0/4] target/arm: Fix handling of HCR.TID* on v7A CPUs

[Peter Maydell]

The HCR.TID* bits are intended to cause trapping of various
register accesses to EL2. Their behaviour changed slightly
between v7A and v8A. In particular for HCR.TID3:

 * 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 attempted to implement the v7A/v8A distinction via
accessfns like access_aa64_tid3() and access_aa32_tid3(). However,
we didn't use the right accessfns on the right registers, with
the effect that for a v7A CPU we don't trap on a lot of the
registers we should trap on, and we do trap on various things
that the v7A Arm ARM says there is no requirement to trap on.

This bug came to light because a recent change to Xen to trap
guest accesses to ID_PFR0 broke on QEMU's Cortex-A15 model.

This series sorts things out so that for v7A we trap only on the
things the architecture says we must trap on, and cleans up the
naming pattern and documentation of the accessfns so that
hopefully they don't get accidentally applied to the wrong
registers for any future additions.

thanks
-- PMM

Peter Maydell (4):
  target/arm: Don't specify ID_PFR1 accessfn twice
  target/arm: Correctly honour HCR.TID3 for v7A cores
  target/arm: Correctly trap HCR.TID1 registers in v7A
  target/arm: Rename access_aa64_tid5() to access_tid5()

 target/arm/helper.c | 177 +++++++++++++++++++++++---------------------
 1 file changed, 91 insertions(+), 86 deletions(-)

-- 
2.47.3

[PATCH 1/4] target/arm: Don't specify ID_PFR1 accessfn twice

[Peter Maydell]

In the definition of ID_PFR1 we have an ifdef block; we specify the
accessfn once in the common part of the ifdef and once in the
not-user-only part, which is redundant but harmless.

The accessfn will always return success in user-only mode (because
we won't trap to EL2), so specify it only in the not-user-only
half of the ifdef, as was probably the intention.

This is only cc'd to stable to avoid a textual conflict with
the following patch, which is a bug fix.

Cc: qemu-stable@nongnu.org
Fixes: 0f150c8499e970bd ("target/arm: Constify ID_PFR1 on user emulation")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 27ebc6f29b..ec82ea6203 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6296,7 +6296,6 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             { .name = "ID_PFR1", .state = ARM_CP_STATE_BOTH,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 1,
               .access = PL1_R, .type = ARM_CP_NO_RAW,
-              .accessfn = access_aa32_tid3,
 #ifdef CONFIG_USER_ONLY
               .type = ARM_CP_CONST,
               .resetvalue = GET_IDREG(isar, ID_PFR1),
-- 
2.47.3

Re: [PATCH 1/4] target/arm: Don't specify ID_PFR1 accessfn twice

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> In the definition of ID_PFR1 we have an ifdef block; we specify the
> accessfn once in the common part of the ifdef and once in the
> not-user-only part, which is redundant but harmless.
>
> The accessfn will always return success in user-only mode (because
> we won't trap to EL2), so specify it only in the not-user-only
> half of the ifdef, as was probably the intention.
>
> This is only cc'd to stable to avoid a textual conflict with
> the following patch, which is a bug fix.
>
> Cc: qemu-stable@nongnu.org
> Fixes: 0f150c8499e970bd ("target/arm: Constify ID_PFR1 on user emulation")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 1/4] target/arm: Don't specify ID_PFR1 accessfn twice

[Richard Henderson]

On 1/1/26 04:08, Peter Maydell wrote:
> In the definition of ID_PFR1 we have an ifdef block; we specify the
> accessfn once in the common part of the ifdef and once in the
> not-user-only part, which is redundant but harmless.
> 
> The accessfn will always return success in user-only mode (because
> we won't trap to EL2), so specify it only in the not-user-only
> half of the ifdef, as was probably the intention.
> 
> This is only cc'd to stable to avoid a textual conflict with
> the following patch, which is a bug fix.
> 
> Cc:qemu-stable@nongnu.org
> Fixes: 0f150c8499e970bd ("target/arm: Constify ID_PFR1 on user emulation")
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 1 -
>   1 file changed, 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 2/4] target/arm: Correctly honour HCR.TID3 for v7A cores

[Peter Maydell]

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.3

Re: [PATCH 2/4] target/arm: Correctly honour HCR.TID3 for v7A cores

[Alex Bennée]

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

Re: [PATCH 2/4] target/arm: Correctly honour HCR.TID3 for v7A cores

[Peter Maydell]

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

Re: [PATCH 2/4] target/arm: Correctly honour HCR.TID3 for v7A cores

[Alex Bennée]

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

Re: [PATCH 2/4] target/arm: Correctly honour HCR.TID3 for v7A cores

[Peter Maydell]

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

Re: [PATCH 2/4] target/arm: Correctly honour HCR.TID3 for v7A cores

[Alex Bennée]

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

Re: [PATCH 2/4] target/arm: Correctly honour HCR.TID3 for v7A cores

[Richard Henderson]

On 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~

[PATCH 3/4] target/arm: Correctly trap HCR.TID1 registers in v7A

[Peter Maydell]

In v7A HCR.TID1 is defined to trap for TCMTR, TLBTR, REVIDR and AIDR.
We incorrectly use an accessfn for REVIDR and AIDR that only traps on
v8A cores.  Fix this by collapsing access_aa64_tid1() and
access_aa32_tid1() together and never doing a check for v8 vs v7.

The accessfn is also used for SMIDR_EL1, which is fine as this
register is AArch64 only.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index c4f73eb3f3..0896e90965 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -924,8 +924,8 @@ static uint64_t isr_read(CPUARMState *env, const ARMCPRegInfo *ri)
     return ret;
 }
 
-static CPAccessResult access_aa64_tid1(CPUARMState *env, const ARMCPRegInfo *ri,
-                                       bool isread)
+static CPAccessResult access_tid1(CPUARMState *env, const ARMCPRegInfo *ri,
+                                  bool isread)
 {
     if (arm_current_el(env) == 1 && (arm_hcr_el2_eff(env) & HCR_TID1)) {
         return CP_ACCESS_TRAP_EL2;
@@ -934,16 +934,6 @@ static CPAccessResult access_aa64_tid1(CPUARMState *env, const ARMCPRegInfo *ri,
     return CP_ACCESS_OK;
 }
 
-static CPAccessResult access_aa32_tid1(CPUARMState *env, const ARMCPRegInfo *ri,
-                                       bool isread)
-{
-    if (arm_feature(env, ARM_FEATURE_V8)) {
-        return access_aa64_tid1(env, ri, isread);
-    }
-
-    return CP_ACCESS_OK;
-}
-
 static const ARMCPRegInfo v7_cp_reginfo[] = {
     /* the old v6 WFI, UNPREDICTABLE in v7 but we choose to NOP */
     { .name = "NOP", .cp = 15, .crn = 7, .crm = 0, .opc1 = 0, .opc2 = 4,
@@ -969,7 +959,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
     { .name = "AIDR", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 1, .crn = 0, .crm = 0, .opc2 = 7,
       .access = PL1_R, .type = ARM_CP_CONST,
-      .accessfn = access_aa64_tid1,
+      .accessfn = access_tid1,
       .fgt = FGT_AIDR_EL1,
       .resetvalue = 0 },
     /*
@@ -4997,7 +4987,7 @@ static const ARMCPRegInfo sme_reginfo[] = {
       .writefn = smcr_write, .raw_writefn = raw_write },
     { .name = "SMIDR_EL1", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 1, .crn = 0, .crm = 0, .opc2 = 6,
-      .access = PL1_R, .accessfn = access_aa64_tid1,
+      .access = PL1_R, .accessfn = access_tid1,
       /*
        * IMPLEMENTOR = 0 (software)
        * REVISION    = 0 (implementation defined)
@@ -7094,7 +7084,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             { .name = "REVIDR_EL1", .state = ARM_CP_STATE_BOTH,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 0, .opc2 = 6,
               .access = PL1_R,
-              .accessfn = access_aa64_tid1,
+              .accessfn = access_tid1,
               .fgt = FGT_REVIDR_EL1,
               .type = ARM_CP_CONST, .resetvalue = cpu->revidr },
         };
@@ -7118,7 +7108,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             { .name = "TCMTR",
               .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 2,
               .access = PL1_R,
-              .accessfn = access_aa32_tid1,
+              .accessfn = access_tid1,
               .type = ARM_CP_CONST, .resetvalue = 0 },
         };
         /* TLBTR is specific to VMSA */
@@ -7126,7 +7116,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .name = "TLBTR",
               .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 3,
               .access = PL1_R,
-              .accessfn = access_aa32_tid1,
+              .accessfn = access_tid1,
               .type = ARM_CP_CONST, .resetvalue = 0,
         };
         /* MPUIR is specific to PMSA V6+ */
-- 
2.47.3

Re: [PATCH 3/4] target/arm: Correctly trap HCR.TID1 registers in v7A

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> In v7A HCR.TID1 is defined to trap for TCMTR, TLBTR, REVIDR and AIDR.
> We incorrectly use an accessfn for REVIDR and AIDR that only traps on
> v8A cores.  Fix this by collapsing access_aa64_tid1() and
> access_aa32_tid1() together and never doing a check for v8 vs v7.
>
> The accessfn is also used for SMIDR_EL1, which is fine as this
> register is AArch64 only.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index c4f73eb3f3..0896e90965 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -924,8 +924,8 @@ static uint64_t isr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>      return ret;
>  }
>  
> -static CPAccessResult access_aa64_tid1(CPUARMState *env, const ARMCPRegInfo *ri,
> -                                       bool isread)
> +static CPAccessResult access_tid1(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                  bool isread)
>  {
>      if (arm_current_el(env) == 1 && (arm_hcr_el2_eff(env) & HCR_TID1)) {
>          return CP_ACCESS_TRAP_EL2;
> @@ -934,16 +934,6 @@ static CPAccessResult access_aa64_tid1(CPUARMState *env, const ARMCPRegInfo *ri,
>      return CP_ACCESS_OK;
>  }
>  
> -static CPAccessResult access_aa32_tid1(CPUARMState *env, const ARMCPRegInfo *ri,
> -                                       bool isread)
> -{
> -    if (arm_feature(env, ARM_FEATURE_V8)) {
> -        return access_aa64_tid1(env, ri, isread);
> -    }
> -
> -    return CP_ACCESS_OK;
> -}
> -

This logic makes more sense from the descriptions compared to 2/4.

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 3/4] target/arm: Correctly trap HCR.TID1 registers in v7A

[Richard Henderson]

On 1/1/26 04:08, Peter Maydell wrote:
> In v7A HCR.TID1 is defined to trap for TCMTR, TLBTR, REVIDR and AIDR.
> We incorrectly use an accessfn for REVIDR and AIDR that only traps on
> v8A cores.  Fix this by collapsing access_aa64_tid1() and
> access_aa32_tid1() together and never doing a check for v8 vs v7.
> 
> The accessfn is also used for SMIDR_EL1, which is fine as this
> register is AArch64 only.
> 
> Cc:qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 24 +++++++-----------------
>   1 file changed, 7 insertions(+), 17 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 4/4] target/arm: Rename access_aa64_tid5() to access_tid5()

[Peter Maydell]

There is no equivalent access_aa32_tid5() (HCR_EL2.TID5 only exists
starting from v8); rename access_aa64_tid5() to access_tid5() to line
up with the naming we now have for the TID1 and TID3 check functions.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0896e90965..b914988ac9 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5425,8 +5425,8 @@ static const ARMCPRegInfo dcpodp_reg[] = {
       .accessfn = aa64_cacheop_poc_access, .writefn = dccvap_writefn },
 };
 
-static CPAccessResult access_aa64_tid5(CPUARMState *env, const ARMCPRegInfo *ri,
-                                       bool isread)
+static CPAccessResult access_tid5(CPUARMState *env, const ARMCPRegInfo *ri,
+                                  bool isread)
 {
     if ((arm_current_el(env) < 2) && (arm_hcr_el2_eff(env) & HCR_TID5)) {
         return CP_ACCESS_TRAP_EL2;
@@ -7449,7 +7449,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         ARMCPRegInfo gmid_reginfo = {
             .name = "GMID_EL1", .state = ARM_CP_STATE_AA64,
             .opc0 = 3, .opc1 = 1, .crn = 0, .crm = 0, .opc2 = 4,
-            .access = PL1_R, .accessfn = access_aa64_tid5,
+            .access = PL1_R, .accessfn = access_tid5,
             .type = ARM_CP_CONST, .resetvalue = cpu->gm_blocksize,
         };
         define_one_arm_cp_reg(cpu, &gmid_reginfo);
-- 
2.47.3

Re: [PATCH 4/4] target/arm: Rename access_aa64_tid5() to access_tid5()

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> There is no equivalent access_aa32_tid5() (HCR_EL2.TID5 only exists
> starting from v8); rename access_aa64_tid5() to access_tid5() to line
> up with the naming we now have for the TID1 and TID3 check functions.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 4/4] target/arm: Rename access_aa64_tid5() to access_tid5()

[Richard Henderson]

On 1/1/26 04:08, Peter Maydell wrote:
> There is no equivalent access_aa32_tid5() (HCR_EL2.TID5 only exists
> starting from v8); rename access_aa64_tid5() to access_tid5() to line
> up with the naming we now have for the TID1 and TID3 check functions.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~