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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.