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

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

[Will Deacon]

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 <jean.pihet@newoldbits.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 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

[PATCH 2/2] ARM: perf: add required isbs() to ARMv7 backend

[Will Deacon]

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 <jean.pihet@newoldbits.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 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

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

[Jean Pihet]

Hi Will,

On Wed, Mar 16, 2011 at 4:38 PM, Will Deacon <will.deacon@arm.com> 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 <jean.pihet@newoldbits.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Acked-by: Jean Pihet <j-pihet@ti.com>

Thanks,
Jean

> ---
> ?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 at 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
>
>

[PATCH 2/2] ARM: perf: add required isbs() to ARMv7 backend

[Jean Pihet]

On Wed, Mar 16, 2011 at 4:38 PM, Will Deacon <will.deacon@arm.com> 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

>
> Cc: Jean Pihet <jean.pihet@newoldbits.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
> ?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
>
>

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

[Will Deacon]

Hi Jean,

> On Wed, Mar 16, 2011 at 4:38 PM, Will Deacon <will.deacon@arm.com> 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?

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.
 
> >
> > 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.

> >
> > Cc: Jean Pihet <jean.pihet@newoldbits.com>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> 
> Acked-by: Jean Pihet <j-pihet@ti.com>

Thanks for the Ack,

Will

[PATCH 2/2] ARM: perf: add required isbs() to ARMv7 backend

[Will Deacon]

Hello,

> On Wed, Mar 16, 2011 at 4:38 PM, Will Deacon <will.deacon@arm.com> 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?

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

[PATCH 2/2] ARM: perf: add required isbs() to ARMv7 backend

[Will Deacon]

Hi Jean,

> > On Wed, Mar 16, 2011 at 4:38 PM, Will Deacon <will.deacon@arm.com> 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?
> 
> 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

[PATCH 2/2] ARM: perf: add required isbs() to ARMv7 backend

[Jean Pihet]

On Mon, Mar 21, 2011 at 7:22 PM, Will Deacon <will.deacon@arm.com> wrote:
> Hi Jean,
>
>> > On Wed, Mar 16, 2011 at 4:38 PM, Will Deacon <will.deacon@arm.com> 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?
>>
>> 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?

Yes I am OK

Acked-by: Jean Pihet <j-pihet@ti.com>

Cheers,
Jean

>
> Cheers,
>
> Will
>
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[PATCH 2/2] ARM: perf: add required isbs() to ARMv7 backend

[Will Deacon]

Hi Jean,

> > 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?
> 
> Yes I am OK
> 
> Acked-by: Jean Pihet <j-pihet@ti.com>

Thanks for the ack. I've submitted these two patches as 6823/1 and
6824/1 respectively.

Cheers,

Will

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

[Will Deacon]

Hi Jean,

> On Wed, Mar 16, 2011 at 4:38 PM, Will Deacon <will.deacon@arm.com> 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 <jean.pihet@newoldbits.com>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> 
> Acked-by: Jean Pihet <j-pihet@ti.com>

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

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

[Jean Pihet]

Hi Will,

On Wed, Mar 23, 2011 at 2:39 PM, Will Deacon <will.deacon@arm.com> wrote:
> Hi Jean,
>
>> On Wed, Mar 16, 2011 at 4:38 PM, Will Deacon <will.deacon@arm.com> 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 <jean.pihet@newoldbits.com>
>> > Signed-off-by: Will Deacon <will.deacon@arm.com>
>>
>> Acked-by: Jean Pihet <j-pihet@ti.com>
>
> 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.
Ok. The new code should probably be called from *read_num_pmnc_events.

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

>
> Stay tuned...
>
> Will
>
Thanks,
Jean

>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

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

[Will Deacon]

> > 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.
> Ok. The new code should probably be called from *read_num_pmnc_events.

I thought that initially, but we need SMP up and running for the cross
calls. Since read_num_pmnc_events is called via an early_initcall, that's
not possible (which is the reason I used an arch_initcall in the first place).

The solution I've come up with is adding a ->reset function pointer to
struct arm_pmu, which can be called via an arch_initcall in perf_event.c
for the PMU it ends up using. I'll post it in a bit.

Will