[PATCH 1/2] ARM: perf: reset counters on all CPUs during initialisation

ARMv7 dictates that the interrupt-enable and count-enable registers for
each PMU counter are UNKNOWN following core reset.

This patch adds an arch_initcall to the ARMv7 perf events backend which
disables the counters on each CPU prior to setting the Enable bit in the
PMCR.

Cc: Jean Pihet <redacted>
Signed-off-by: Will Deacon <redacted>
---
 arch/arm/kernel/perf_event_v7.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
index 2e14025..3f42473 100644
--- a/arch/arm/kernel/perf_event_v7.c
+++ b/arch/arm/kernel/perf_event_v7.c

@@ -860,12 +860,31 @@ static struct arm_pmu armv7pmu = {
 	.max_period		= (1LLU << 32) - 1,
 };
 
-static u32 __init armv7_reset_read_pmnc(void)
+static void armv7_reset_pmnc(void *info)
 {
-	u32 nb_cnt;
+	u32 idx, nb_cnt = armv7pmu.num_events;
+
+	/* The counter and interrupt enable registers are unknown@reset. */
+	for (idx = 1; idx < nb_cnt; ++idx)
+		armv7pmu_disable_event(NULL, idx);
 
 	/* Initialize & Reset PMNC: C and P bits */
 	armv7_pmnc_write(ARMV7_PMNC_P | ARMV7_PMNC_C);
+}
+
+static int __init armv7_pmnc_init(void)
+{
+	/*
+	 * Reset the counter status on each CPU so that we don't receive
+	 * spurious interrupts when we set the E bit.
+	 */
+	return on_each_cpu(armv7_reset_pmnc, NULL, 1);
+}
+arch_initcall(armv7_pmnc_init);
+
+static u32 __init armv7_read_num_pmnc_events(void)
+{
+	u32 nb_cnt;
 
 	/* Read the nb of CNTx counters supported from PMNC */
 	nb_cnt = (armv7_pmnc_read() >> ARMV7_PMNC_N_SHIFT) & ARMV7_PMNC_N_MASK;

@@ -880,7 +899,7 @@ static const struct arm_pmu *__init armv7_a8_pmu_init(void)
 	armv7pmu.name		= "ARMv7 Cortex-A8";
 	armv7pmu.cache_map	= &armv7_a8_perf_cache_map;
 	armv7pmu.event_map	= &armv7_a8_perf_map;
-	armv7pmu.num_events	= armv7_reset_read_pmnc();
+	armv7pmu.num_events	= armv7_read_num_pmnc_events();
 	return &armv7pmu;
 }
 

@@ -890,7 +909,7 @@ static const struct arm_pmu *__init armv7_a9_pmu_init(void)
 	armv7pmu.name		= "ARMv7 Cortex-A9";
 	armv7pmu.cache_map	= &armv7_a9_perf_cache_map;
 	armv7pmu.event_map	= &armv7_a9_perf_map;
-	armv7pmu.num_events	= armv7_reset_read_pmnc();
+	armv7pmu.num_events	= armv7_read_num_pmnc_events();
 	return &armv7pmu;
 }
 #else

-- 
1.7.0.4

The ARMv7 architecture does not guarantee that effects from co-processor
writes are immediately visible to following instructions.

This patch adds two isbs to the ARMv7 perf code:

(1) Immediately after selecting an event register, so that the PMU state
    following this instruction is consistent with the new event.

(2) Immediately before writing to the PMCR, so that any previous writes
    to the PMU have taken effect before (typically) enabling the
    counters.

Cc: Jean Pihet <redacted>
Signed-off-by: Will Deacon <redacted>
---
 arch/arm/kernel/perf_event_v7.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
index 3f42473..a79a7b6 100644
--- a/arch/arm/kernel/perf_event_v7.c
+++ b/arch/arm/kernel/perf_event_v7.c

@@ -466,6 +466,7 @@ static inline unsigned long armv7_pmnc_read(void)
 static inline void armv7_pmnc_write(unsigned long val)
 {
 	val &= ARMV7_PMNC_MASK;
+	isb();
 	asm volatile("mcr p15, 0, %0, c9, c12, 0" : : "r"(val));
 }
 

@@ -502,6 +503,7 @@ static inline int armv7_pmnc_select_counter(unsigned int idx)
 
 	val = (idx - ARMV7_EVENT_CNT_TO_CNTx) & ARMV7_SELECT_MASK;
 	asm volatile("mcr p15, 0, %0, c9, c12, 5" : : "r" (val));
+	isb();
 
 	return idx;
 }

-- 
1.7.0.4

Hi Will,

On Wed, Mar 16, 2011 at 4:38 PM, Will Deacon [off-list ref] wrote:

ARMv7 dictates that the interrupt-enable and count-enable registers for
each PMU counter are UNKNOWN following core reset.

Great! Is this development driven by actual issues or by precaution?

This patch adds an arch_initcall to the ARMv7 perf events backend which
disables the counters on each CPU prior to setting the Enable bit in the
PMCR.

Also the function rename is a good thing.

Cc: Jean Pihet <redacted>
Signed-off-by: Will Deacon <redacted>

Acked-by: Jean Pihet <redacted>

Thanks,
Jean

On Wed, Mar 16, 2011 at 4:38 PM, Will Deacon [off-list ref] wrote:

The ARMv7 architecture does not guarantee that effects from co-processor
writes are immediately visible to following instructions.

This patch adds two isbs to the ARMv7 perf code:

(1) Immediately after selecting an event register, so that the PMU state
? ?following this instruction is consistent with the new event.

Ok

(2) Immediately before writing to the PMCR, so that any previous writes
? ?to the PMU have taken effect before (typically) enabling the
? ?counters.

Should the isb come _after_ the cp15 instruction so that the current
access is actually performed?

Thanks,
Jean

Hi Jean,

On Wed, Mar 16, 2011 at 4:38 PM, Will Deacon [off-list ref] wrote:

quoted

ARMv7 dictates that the interrupt-enable and count-enable registers for
each PMU counter are UNKNOWN following core reset.

Great! Is this development driven by actual issues or by precaution?

Just precautionary, but it's also a precursor to some power management
work in ARM which might leave the registers in a funny state when they
come out of a low-power state. This code at least gives them an entry
hook for dealing with that.
 

quoted

This patch adds an arch_initcall to the ARMv7 perf events backend which
disables the counters on each CPU prior to setting the Enable bit in the
PMCR.

Also the function rename is a good thing.

Yup.

quoted

Cc: Jean Pihet <redacted>
Signed-off-by: Will Deacon <redacted>

Acked-by: Jean Pihet <redacted>

Thanks for the Ack,

Will

Hello,

On Wed, Mar 16, 2011 at 4:38 PM, Will Deacon [off-list ref] wrote:

quoted

The ARMv7 architecture does not guarantee that effects from co-processor
writes are immediately visible to following instructions.

This patch adds two isbs to the ARMv7 perf code:

(1) Immediately after selecting an event register, so that the PMU state
? ?following this instruction is consistent with the new event.

Ok

quoted

(2) Immediately before writing to the PMCR, so that any previous writes
? ?to the PMU have taken effect before (typically) enabling the
? ?counters.

Should the isb come _after_ the cp15 instruction so that the current
access is actually performed?

No. We want to ensure that the _other_ PMU registers are up-to-date before
playing with the control register, otherwise we could end up in a horrible
situation where we enable all the counters, but the writes to the counters
themselves haven't yet made it.

The write to the control register will take effect before the next
exception return, which is fine for what we want.

Will

Hi Jean,

quoted

On Wed, Mar 16, 2011 at 4:38 PM, Will Deacon [off-list ref] wrote:

quoted

The ARMv7 architecture does not guarantee that effects from co-processor
writes are immediately visible to following instructions.

This patch adds two isbs to the ARMv7 perf code:

(1) Immediately after selecting an event register, so that the PMU state
? ?following this instruction is consistent with the new event.

Ok

quoted

(2) Immediately before writing to the PMCR, so that any previous writes
? ?to the PMU have taken effect before (typically) enabling the
? ?counters.

Should the isb come _after_ the cp15 instruction so that the current
access is actually performed?

No. We want to ensure that the _other_ PMU registers are up-to-date before
playing with the control register, otherwise we could end up in a horrible
situation where we enable all the counters, but the writes to the counters
themselves haven't yet made it.

The write to the control register will take effect before the next
exception return, which is fine for what we want.

I'd like to submit this to the patch system if you're happy with it.
Are you OK with the barrier coming before the write to the PMCR?

Cheers,

Will

Hi Jean,

On Wed, Mar 16, 2011 at 4:38 PM, Will Deacon [off-list ref] wrote:

quoted

ARMv7 dictates that the interrupt-enable and count-enable registers for
each PMU counter are UNKNOWN following core reset.

Great! Is this development driven by actual issues or by precaution?

quoted

This patch adds an arch_initcall to the ARMv7 perf events backend which
disables the counters on each CPU prior to setting the Enable bit in the
PMCR.

Also the function rename is a good thing.

quoted

Cc: Jean Pihet <redacted>
Signed-off-by: Will Deacon <redacted>

Acked-by: Jean Pihet <redacted>

I don't think this patch is safe for combined v6/v7 kernels because the
arch_initcall will try to poke the v7 PMU regardless of whether or not
it exists. I'll try and rework something and send another version to the
list.

I've removed it from the patch system but left the isb patch there
because that is fine as it is.

Stay tuned...

Will