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
--
2.41.0
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.
© 2016 - 2024 Red Hat, Inc.