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

["Alex Bennée" <alex.bennee@linaro.org>]

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