Re: [PATCH 3/8] target/arm: use FIELD macro for CNTHCTL bit definitions

[Peter Maydell]

On Fri, 1 Mar 2024 at 21:19, Richard Henderson
<richard.henderson@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.maydell@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