Peter Maydell <[email protected]> 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: [email protected] > Fixes: 6a4ef4e5d1084c ("target/arm: Honor HCR_EL2.TID3 trapping requirements") > Signed-off-by: Peter Maydell <[email protected]> > --- > 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
