| Message ID | 20231207154550.65087-3-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sysemu/replay: Restrict icount to TCG system emulation | expand |
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. r~
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)
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. -- PMM
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. This icount cleanup blocks 60+ patches on top :/ Since I have a v3 addressing Richard's comments already done, I'll post it, mentioning the PMU issue in the cover; then see if cleaning it isn't too invasive. If I end in another rabbit hole, I'll suggest to accept this current patch and clean the technical debt later, again. Thanks, Phil.
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.
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
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(+)