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

[Richard Henderson <richard.henderson@linaro.org>]

On 3/4/24 03:21, Peter Maydell wrote:
> 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) ?

Looks good, thanks.


r~