On Fri, 1 Mar 2024 at 21:19, Richard Henderson <richard.hender...@linaro.org> wrote: > > On 3/1/24 08:32, Peter Maydell wrote: > > We prefer the FIELD macro over ad-hoc #defines for register bits; > > switch CNTHCTL to that style before we add any more bits. > > > > Signed-off-by: Peter Maydell <peter.mayd...@linaro.org> > > --- > > target/arm/internals.h | 19 +++++++++++++++++-- > > target/arm/helper.c | 9 ++++----- > > 2 files changed, 21 insertions(+), 7 deletions(-) > > > > diff --git a/target/arm/internals.h b/target/arm/internals.h > > index c93acb270cc..6553e414934 100644 > > --- a/target/arm/internals.h > > +++ b/target/arm/internals.h > > @@ -224,8 +224,23 @@ FIELD(VTCR, SL2, 33, 1) > > #define HSTR_TTEE (1 << 16) > > #define HSTR_TJDBX (1 << 17) > > > > -#define CNTHCTL_CNTVMASK (1 << 18) > > -#define CNTHCTL_CNTPMASK (1 << 19) > > +FIELD(CNTHCTL, EL0PCTEN, 0, 1) > > +FIELD(CNTHCTL, EL0VCTEN, 1, 1) > > +FIELD(CNTHCTL, EVNTEN, 2, 1) > > +FIELD(CNTHCTL, EVNTDIR, 3, 1) > ... > > +FIELD(CNTHCTL, EL0VTEN, 8, 1) > > +FIELD(CNTHCTL, EL0PTEN, 9, 1) > > +FIELD(CNTHCTL, EL1PCTEN, 10, 1) > > +FIELD(CNTHCTL, EL1PTEN, 11, 1) > > These bits change definition based on HCR_E2H, which I remembered when I got > to patch 5, > which adds code nearby the existing tests of these bits. > > It might be confusing to only provide the E2H versions, without some extra > suffix?
Yeah, bits 8..11 are RES0 if E2H is 0; bits 3 and 2 are the same; bits 0 and 1 change (to EL1PCTEN and EL1PCEN, so bit 0 when E2H is 0 has the same name as bit 10 when E2H is 1). I don't see the need to suffix the bits that are only relevant in one context and RES0 in the other, only the ones where the bit has the same name but a different location, or where the same bit number has two names. So: +/* + * Depending on the value of HCR_EL2.E2H, bits 0 and 1 + * have different bit definitions, and EL1PCTEN might be + * bit 0 or bit 10. We use _E2H1 and _E2H0 suffixes to + * disambiguate if necessary. + */ +FIELD(CNTHCTL, EL0PCTEN_E2H1, 0, 1) +FIELD(CNTHCTL, EL0VCTEN_E2H1, 1, 1) +FIELD(CNTHCTL, EL1PCTEN_E2H0, 0, 1) +FIELD(CNTHCTL, EL1PCEN_E2H0, 1, 1) +FIELD(CNTHCTL, EVNTEN, 2, 1) +FIELD(CNTHCTL, EVNTDIR, 3, 1) +FIELD(CNTHCTL, EVNTI, 4, 4) +FIELD(CNTHCTL, EL0VTEN, 8, 1) +FIELD(CNTHCTL, EL0PTEN, 9, 1) +FIELD(CNTHCTL, EL1PCTEN_E2H1, 10, 1) +FIELD(CNTHCTL, EL1PTEN, 11, 1) +FIELD(CNTHCTL, ECV, 12, 1) +FIELD(CNTHCTL, EL1TVT, 13, 1) +FIELD(CNTHCTL, EL1TVCT, 14, 1) +FIELD(CNTHCTL, EL1NVPCT, 15, 1) +FIELD(CNTHCTL, EL1NVVCT, 16, 1) +FIELD(CNTHCTL, EVNTIS, 17, 1) +FIELD(CNTHCTL, CNTVMASK, 18, 1) +FIELD(CNTHCTL, CNTPMASK, 19, 1) (and updating the uses in following patches as needed) ? thanks -- PMM
