| Message ID | 20220811171619.1154755-6-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Implement FEAT_PMUv3p5 | expand |
On 8/11/22 10:16, Peter Maydell wrote: > The logic in pmu_counter_enabled() for handling the 'prohibit event > counting' bits MDCR_EL2.HPMD and MDCR_EL3.SPME is written in a way > that assumes that EL2 is never Secure. This used to be true, but the > architecture now permits Secure EL2, and QEMU can emulate this. > > Refactor the prohibit logic so that we effectively OR together > the various prohibit bits when they apply, rather than trying to > construct an if-else ladder where any particular state of the CPU > ends up in exactly one branch of the ladder. > > This fixes the Secure EL2 case and also is a better structure for > adding the PMUv8.5 bits MDCR_EL2.HCCD and MDCR_EL3.SCCD. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > target/arm/helper.c | 17 +++++++---------- > 1 file changed, 7 insertions(+), 10 deletions(-) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 41def52cf7b..434885d024a 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -1094,7 +1094,7 @@ static bool pmu_counter_enabled(CPUARMState *env, uint8_t counter) > { > uint64_t filter; > bool e, p, u, nsk, nsu, nsh, m; > - bool enabled, prohibited, filtered; > + bool enabled, prohibited = false, filtered; > bool secure = arm_is_secure(env); > int el = arm_current_el(env); > uint64_t mdcr_el2 = arm_mdcr_el2_eff(env); > @@ -1112,15 +1112,12 @@ static bool pmu_counter_enabled(CPUARMState *env, uint8_t counter) > } > enabled = e && (env->cp15.c9_pmcnten & (1 << counter)); > > - if (!secure) { > - if (el == 2 && (counter < hpmn || counter == 31)) { > - prohibited = mdcr_el2 & MDCR_HPMD; > - } else { > - prohibited = false; > - } > - } else { > - prohibited = arm_feature(env, ARM_FEATURE_EL3) && > - !(env->cp15.mdcr_el3 & MDCR_SPME); > + /* Is event counting prohibited? */ > + if (el == 2 && (counter < hpmn || counter == 31)) { > + prohibited = mdcr_el2 & MDCR_HPMD; > + } > + if (secure) { > + prohibited = prohibited || !(env->cp15.mdcr_el3 & MDCR_SPME); You can use |= for bool as well. You don't need the short-circuting of || here. For consistency, perhaps better to use |= with the first if as well, relying on the compiler to fold the initial value false. But otherwise, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Thu, 11 Aug 2022 at 18:48, Richard Henderson
<richard.henderson@linaro.org> wrote:
> You can use |= for bool as well. You don't need the short-circuting of || here.
That seems like the kind of thing that -Wbool-operation is likely to
warn about either now or in future, though...
-- PMM
diff --git a/target/arm/helper.c b/target/arm/helper.c index 41def52cf7b..434885d024a 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -1094,7 +1094,7 @@ static bool pmu_counter_enabled(CPUARMState *env, uint8_t counter) { uint64_t filter; bool e, p, u, nsk, nsu, nsh, m; - bool enabled, prohibited, filtered; + bool enabled, prohibited = false, filtered; bool secure = arm_is_secure(env); int el = arm_current_el(env); uint64_t mdcr_el2 = arm_mdcr_el2_eff(env); @@ -1112,15 +1112,12 @@ static bool pmu_counter_enabled(CPUARMState *env, uint8_t counter) } enabled = e && (env->cp15.c9_pmcnten & (1 << counter)); - if (!secure) { - if (el == 2 && (counter < hpmn || counter == 31)) { - prohibited = mdcr_el2 & MDCR_HPMD; - } else { - prohibited = false; - } - } else { - prohibited = arm_feature(env, ARM_FEATURE_EL3) && - !(env->cp15.mdcr_el3 & MDCR_SPME); + /* Is event counting prohibited? */ + if (el == 2 && (counter < hpmn || counter == 31)) { + prohibited = mdcr_el2 & MDCR_HPMD; + } + if (secure) { + prohibited = prohibited || !(env->cp15.mdcr_el3 & MDCR_SPME); } if (prohibited && counter == 31) {
The logic in pmu_counter_enabled() for handling the 'prohibit event counting' bits MDCR_EL2.HPMD and MDCR_EL3.SPME is written in a way that assumes that EL2 is never Secure. This used to be true, but the architecture now permits Secure EL2, and QEMU can emulate this. Refactor the prohibit logic so that we effectively OR together the various prohibit bits when they apply, rather than trying to construct an if-else ladder where any particular state of the CPU ends up in exactly one branch of the ladder. This fixes the Secure EL2 case and also is a better structure for adding the PMUv8.5 bits MDCR_EL2.HCCD and MDCR_EL3.SCCD. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/helper.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)