Re: [PATCH v2 2/5] target/arm: Ensure icount is enabled when emulating INST_RETIRED

[Philippe Mathieu-Daudé]

On 8/12/23 12:23, Philippe Mathieu-Daudé wrote:
> Hi Peter,
>
> On 8/12/23 11:59, Peter Maydell wrote:
> > On Fri, 8 Dec 2023 at 10:36, Philippe Mathieu-Daudé 
> > <philmd@linaro.org> wrote:
> > > On 7/12/23 23:12, Richard Henderson wrote:
> > > > On 12/7/23 07:45, Philippe Mathieu-Daudé wrote:
> > > > > pmu_init() register its event checking the pm_event::supported()
> > > > > handler. For INST_RETIRED, the event is only registered and the
> > > > > bit enabled in the PMU Common Event Identification register when
> > > > > icount is enabled as ICOUNT_PRECISE.
> > > > >
> > > > > Assert the pm_event::get_count() and pm_event::ns_per_count()
> > > > > handler will only be called under this icount mode.
> > > > >
> > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > > ---
> > > > >    target/arm/helper.c | 2 ++
> > > > >    1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > > > > index adb0960bba..333fd5f4bf 100644
> > > > > --- a/target/arm/helper.c
> > > > > +++ b/target/arm/helper.c
> > > > > @@ -940,11 +940,13 @@ static bool instructions_supported(CPUARMState
> > > > > *env)
> > > > >    static uint64_t instructions_get_count(CPUARMState *env)
> > > > >    {
> > > > > +    assert(icount_enabled() == ICOUNT_PRECISE);
> > > > >        return (uint64_t)icount_get_raw();
> > > > >    }
> > > > >    static int64_t instructions_ns_per(uint64_t icount)
> > > > >    {
> > > > > +    assert(icount_enabled() == ICOUNT_PRECISE);
> > > > >        return icount_to_ns((int64_t)icount);
> > > > >    }
> > > > >    #endif
> > > >
> > > > I don't think an assert is required -- that's exactly what the
> > > > .supported field is for. If you think this needs additional
> > > > clarification, a comment is sufficient.
> > >
> > > Without this I'm getting this link failure with TCG disabled:
> > >
> > > ld: Undefined symbols:
> > >     _icount_to_ns, referenced from:
> > >         _instructions_ns_per in target_arm_helper.c.o
> > > clang: error: linker command failed with exit code 1 (use -v to see
> > > invocation)
> >
> > I think we should fix this earlier by not trying to enable
> > these TCG-only PMU event types in a non-TCG config.
>
> I agree... but (as discussed yesterday on IRC), this is a bigger rework.

Giving it a try, I figured HVF emulates PMC (cycle counter) within
some vPMU, containing "a single event source: the cycle counter"
(per Alex).
Some helpers are duplicated, such pmu_update_irq().

pmu_counter_enabled() diff (-KVM +HVF):

 /*
  * Returns true if the counter (pass 31 for PMCCNTR) should count 
events using
  * the current EL, security state, and register configuration.
  */
 static bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
 {
     uint64_t filter;
-    bool e, p, u, nsk, nsu, nsh, m;
-    bool enabled, prohibited = false, filtered;
-    bool secure = arm_is_secure(env);
+    bool enabled, filtered = true;
     int el = arm_current_el(env);
-    uint64_t mdcr_el2 = arm_mdcr_el2_eff(env);
-    uint8_t hpmn = mdcr_el2 & MDCR_HPMN;

-    if (!arm_feature(env, ARM_FEATURE_PMU)) {
-        return false;
-    }
-
-    if (!arm_feature(env, ARM_FEATURE_EL2) ||
-            (counter < hpmn || counter == 31)) {
-        e = env->cp15.c9_pmcr & PMCRE;
-    } else {
-        e = mdcr_el2 & MDCR_HPME;
-    }
-    enabled = e && (env->cp15.c9_pmcnten & (1 << counter));
-
-    /* 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 (counter == 31) {
-        /*
-         * The cycle counter defaults to running. PMCR.DP says "disable
-         * the cycle counter when event counting is prohibited".
-         * Some MDCR bits disable the cycle counter specifically.
-         */
-        prohibited = prohibited && env->cp15.c9_pmcr & PMCRDP;
-        if (cpu_isar_feature(any_pmuv3p5, env_archcpu(env))) {
-            if (secure) {
-                prohibited = prohibited || (env->cp15.mdcr_el3 & 
MDCR_SCCD);
-            }
-            if (el == 2) {
-                prohibited = prohibited || (mdcr_el2 & MDCR_HCCD);
-            }
-        }
-    }
+    enabled = (env->cp15.c9_pmcr & PMCRE) &&
+              (env->cp15.c9_pmcnten & (1 << counter));

     if (counter == 31) {
         filter = env->cp15.pmccfiltr_el0;
     } else {
         filter = env->cp15.c14_pmevtyper[counter];
     }

-    p   = filter & PMXEVTYPER_P;
-    u   = filter & PMXEVTYPER_U;
-    nsk = arm_feature(env, ARM_FEATURE_EL3) && (filter & PMXEVTYPER_NSK);
-    nsu = arm_feature(env, ARM_FEATURE_EL3) && (filter & PMXEVTYPER_NSU);
-    nsh = arm_feature(env, ARM_FEATURE_EL2) && (filter & PMXEVTYPER_NSH);
-    m   = arm_el_is_aa64(env, 1) &&
-              arm_feature(env, ARM_FEATURE_EL3) && (filter & PMXEVTYPER_M);
-
     if (el == 0) {
-        filtered = secure ? u : u != nsu;
+        filtered = filter & PMXEVTYPER_U;
     } else if (el == 1) {
-        filtered = secure ? p : p != nsk;
-    } else if (el == 2) {
-        filtered = !nsh;
-    } else { /* EL3 */
-        filtered = m != p;
+        filtered = filter & PMXEVTYPER_P;
     }

     if (counter != 31) {
         /*
          * If not checking PMCCNTR, ensure the counter is setup to an 
event we
          * support
          */
         uint16_t event = filter & PMXEVTYPER_EVTCOUNT;
-        if (!event_supported(event)) {
+        if (!pmu_event_supported(event)) {
             return false;
         }
     }

-    return enabled && !prohibited && !filtered;
+    return enabled && !filtered;
 }

I could link HVF without PMU a few surgery such:
---
@@ -1493,12 +1486,12 @@ static int hvf_sysreg_write(CPUState *cpu, 
uint32_t reg, uint64_t val)

     switch (reg) {
     case SYSREG_PMCCNTR_EL0:
-        pmu_op_start(env);
+        pmccntr_op_start(env);
         env->cp15.c15_ccnt = val;
-        pmu_op_finish(env);
+        pmccntr_op_finish(env);
         break;
     case SYSREG_PMCR_EL0:
-        pmu_op_start(env);
+        pmccntr_op_start(env);

---

I'll try to split as:

- target/arm/pmu_common_helper.c (?)
- target/arm/pmc_helper.c
- target/arm/tcg/pmu_helper.c

Ideally pmu_counter_enabled() should be unified, but I
don't feel confident enough to do it.

Regards,

Phil.