| Message ID | 20240301183219.2424889-4-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Implement FEAT_ECV (Enhanced Counter Virtualization) | expand |
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>
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~
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~
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
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~
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); } }
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(-)