qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v2 2/5] target/arm: Ensure icount is enabled when emulating INST_RETIRED
Date: Fri, 8 Dec 2023 15:00:50 +0100
User-agent: Mozilla Thunderbird

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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]