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