1. Patchew
  2. QEMU
  3. [PATCH 0/8] target/arm: Implement FEAT_ECV (Enhanced Counter Virtualization)
  4. View patch

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

Peter Maydell posted 8 patches 8 months, 4 weeks ago
Maintainers: Peter Maydell <peter.maydell@linaro.org>
[PATCH 3/8] target/arm: use FIELD macro for CNTHCTL bit definitions
Posted by Peter Maydell 8 months, 4 weeks ago 
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, EVNTI, 4, 4)
+FIELD(CNTHCTL, EL0VTEN, 8, 1)
+FIELD(CNTHCTL, EL0PTEN, 9, 1)
+FIELD(CNTHCTL, EL1PCTEN, 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)
 
 /* We use a few fake FSR values for internal purposes in M profile.
  * M profile cores don't have A/R format FSRs, but currently our
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 978df6f2823..1c82d12a883 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2652,8 +2652,8 @@ static void gt_update_irq(ARMCPU *cpu, int timeridx)
      * It is RES0 in Secure and NonSecure state.
      */
     if ((ss == ARMSS_Root || ss == ARMSS_Realm) &&
-        ((timeridx == GTIMER_VIRT && (cnthctl & CNTHCTL_CNTVMASK)) ||
-         (timeridx == GTIMER_PHYS && (cnthctl & CNTHCTL_CNTPMASK)))) {
+        ((timeridx == GTIMER_VIRT && (cnthctl & R_CNTHCTL_CNTVMASK_MASK)) ||
+         (timeridx == GTIMER_PHYS && (cnthctl & R_CNTHCTL_CNTPMASK_MASK)))) {
         irqstate = 0;
     }
 
@@ -2968,12 +2968,11 @@ static void gt_cnthctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     ARMCPU *cpu = env_archcpu(env);
     uint32_t oldval = env->cp15.cnthctl_el2;
-
     raw_write(env, ri, value);
 
-    if ((oldval ^ value) & CNTHCTL_CNTVMASK) {
+    if ((oldval ^ value) & R_CNTHCTL_CNTVMASK_MASK) {
         gt_update_irq(cpu, GTIMER_VIRT);
-    } else if ((oldval ^ value) & CNTHCTL_CNTPMASK) {
+    } else if ((oldval ^ value) & R_CNTHCTL_CNTPMASK_MASK) {
         gt_update_irq(cpu, GTIMER_PHYS);
     }
 }
-- 
2.34.1
Re: [PATCH 3/8] target/arm: use FIELD macro for CNTHCTL bit definitions
Posted by Richard Henderson 8 months, 4 weeks ago 
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?


r~
Re: [PATCH 3/8] target/arm: use FIELD macro for CNTHCTL bit definitions
Posted by Peter Maydell 8 months, 3 weeks ago 
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
Re: [PATCH 3/8] target/arm: use FIELD macro for CNTHCTL bit definitions
Posted by Richard Henderson 8 months, 3 weeks ago 
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~
Re: [PATCH 3/8] target/arm: use FIELD macro for CNTHCTL bit definitions
Posted by Richard Henderson 8 months, 4 weeks ago 
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(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~
Re: [PATCH 3/8] target/arm: use FIELD macro for CNTHCTL bit definitions
Posted by Philippe Mathieu-Daudé 8 months, 4 weeks ago 
On 1/3/24 19: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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.