[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.
[PATCH v2 4/5] system/vl: Restrict icount to TCG emulation, Philippe Mathieu-Daudé, 2023/12/07
[PATCH v2 3/5] util/async: Only call icount_notify_exit() if icount is enabled, Philippe Mathieu-Daudé, 2023/12/07
[PATCH v2 5/5] sysemu/replay: Restrict icount to system emulation, Philippe Mathieu-Daudé, 2023/12/07