Peter Maydell <[email protected]> writes: > On Fri, 2 Jan 2026 at 11:17, Alex Bennée <[email protected]> wrote: >> >> 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). > > 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 <[email protected]> > > 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
