arm64: perf: Do not write event type for cycle counter

Commit Message

Julien Thierry March 28, 2019, 9:49 a.m. UTC

Perf events using the dedicated cycle counter do not need to program the
event type, the counter only ever counts that kind of events.

Even worse, trying to program an event type without excluding the
cycle counter index might end up in the modification of the type of
event counted by another perf event.

Reported-by: Wei Li <[e-mail]>
Signed-off-by: Julien Thierry <[e-mail]>
Cc: Will Deacon <[e-mail]>
Cc: Mark Rutland <[e-mail]>
Cc: Catalin Marinas <[e-mail]>
Cc: Peter Zijlstra <[e-mail]>
Cc: Ingo Molnar <[e-mail]>
Cc: Arnaldo Carvalho de Melo <[e-mail]>
Cc: Alexander Shishkin <[e-mail]>
Cc: Jiri Olsa <[e-mail]>
Cc: Namhyung Kim <[e-mail]>
---
 arch/arm64/kernel/perf_event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
1.9.1

Comments

Mark Rutland April 1, 2019, 8:02 a.m. UTC | #1

On Thu, Mar 28, 2019 at 09:49:05AM +0000, Julien Thierry wrote:
> Perf events using the dedicated cycle counter do not need to program the
> event type, the counter only ever counts that kind of events.

Good catch!

> Even worse, trying to program an event type without excluding the
> cycle counter index might end up in the modification of the type of
> event counted by another perf event.

IIUC, we shouldn't affect an unrelated event. The ARM ARM says:

  When PMSELR_EL0.SEL == 31, this register accesses PMCCFILTR_EL0.

... and AFAICT we program PMSELR_EL0 appropriately to select the cycle counter.

I think we're erroneously programming the RES0 bits of PMCCFILTR_EL0, but we're
also relying on this write to clear all the filter controls in the high bits of
PMCCFILTR_EL0, which otherwise reset to UNKNOWN values.

Given that, I think we also need to explicitly reset PMCCFILTR_EL0 in
armv8pmu_reset().

Thanks,
Mark.

> 
> Reported-by: Wei Li <[e-mail]>
> Signed-off-by: Julien Thierry <[e-mail]>
> Cc: Will Deacon <[e-mail]>
> Cc: Mark Rutland <[e-mail]>
> Cc: Catalin Marinas <[e-mail]>
> Cc: Peter Zijlstra <[e-mail]>
> Cc: Ingo Molnar <[e-mail]>
> Cc: Arnaldo Carvalho de Melo <[e-mail]>
> Cc: Alexander Shishkin <[e-mail]>
> Cc: Jiri Olsa <[e-mail]>
> Cc: Namhyung Kim <[e-mail]>
> ---
>  arch/arm64/kernel/perf_event.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index 4addb38..3f898bc 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -514,7 +514,7 @@ static inline void armv8pmu_write_event_type(struct perf_event *event)
> 
>  		armv8pmu_write_evtype(idx - 1, hwc->config_base);
>  		armv8pmu_write_evtype(idx, chain_evt);
> -	} else {
> +	} else if (idx != ARMV8_IDX_CYCLE_COUNTER) {
>  		armv8pmu_write_evtype(idx, hwc->config_base);
>  	}
>  }
> --
> 1.9.1

Julien Thierry April 2, 2019, 3:42 p.m. UTC | #2

On 01/04/2019 09:02, Mark Rutland wrote:
> On Thu, Mar 28, 2019 at 09:49:05AM +0000, Julien Thierry wrote:
>> Perf events using the dedicated cycle counter do not need to program the
>> event type, the counter only ever counts that kind of events.
> 
> Good catch!
> 
>> Even worse, trying to program an event type without excluding the
>> cycle counter index might end up in the modification of the type of
>> event counted by another perf event.
> 
> IIUC, we shouldn't affect an unrelated event. The ARM ARM says:
> 
>   When PMSELR_EL0.SEL == 31, this register accesses PMCCFILTR_EL0.
> 

Ah yes, I missed that fact.

> ... and AFAICT we program PMSELR_EL0 appropriately to select the cycle counter.
> 
> I think we're erroneously programming the RES0 bits of PMCCFILTR_EL0, but we're
> also relying on this write to clear all the filter controls in the high bits of
> PMCCFILTR_EL0, which otherwise reset to UNKNOWN values.
> 
> Given that, I think we also need to explicitly reset PMCCFILTR_EL0 in
> armv8pmu_reset().
> 

I just realized that for the PMCCFILTR_EL0 there is also the exclude
control bits for the event. Do those not need to be set when we enable
the event rather than on reset?

Now, I'm thinking what we need is to use a different mask than
EVTYPE_MASK (one that excludes the event bits) when setting
PMCCFILTR_EL0, but still do that when enabling an event using the cycle
counter.

Does that approach sound correct? Or am I missing something that would
allow us to only set that upon reset?

Thanks,

> Thanks,
> Mark.
> 
>>
>> Reported-by: Wei Li <[e-mail]>
>> Signed-off-by: Julien Thierry <[e-mail]>
>> Cc: Will Deacon <[e-mail]>
>> Cc: Mark Rutland <[e-mail]>
>> Cc: Catalin Marinas <[e-mail]>
>> Cc: Peter Zijlstra <[e-mail]>
>> Cc: Ingo Molnar <[e-mail]>
>> Cc: Arnaldo Carvalho de Melo <[e-mail]>
>> Cc: Alexander Shishkin <[e-mail]>
>> Cc: Jiri Olsa <[e-mail]>
>> Cc: Namhyung Kim <[e-mail]>
>> ---
>>  arch/arm64/kernel/perf_event.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>> index 4addb38..3f898bc 100644
>> --- a/arch/arm64/kernel/perf_event.c
>> +++ b/arch/arm64/kernel/perf_event.c
>> @@ -514,7 +514,7 @@ static inline void armv8pmu_write_event_type(struct perf_event *event)
>>
>>  		armv8pmu_write_evtype(idx - 1, hwc->config_base);
>>  		armv8pmu_write_evtype(idx, chain_evt);
>> -	} else {
>> +	} else if (idx != ARMV8_IDX_CYCLE_COUNTER) {
>>  		armv8pmu_write_evtype(idx, hwc->config_base);
>>  	}
>>  }
>> --
>> 1.9.1

Mark Rutland April 3, 2019, 3:10 a.m. UTC | #3

On Tue, Apr 02, 2019 at 04:42:58PM +0100, Julien Thierry wrote:
> 
> 
> On 01/04/2019 09:02, Mark Rutland wrote:
> > On Thu, Mar 28, 2019 at 09:49:05AM +0000, Julien Thierry wrote:
> >> Perf events using the dedicated cycle counter do not need to program the
> >> event type, the counter only ever counts that kind of events.
> > 
> > Good catch!
> > 
> >> Even worse, trying to program an event type without excluding the
> >> cycle counter index might end up in the modification of the type of
> >> event counted by another perf event.
> > 
> > IIUC, we shouldn't affect an unrelated event. The ARM ARM says:
> > 
> >   When PMSELR_EL0.SEL == 31, this register accesses PMCCFILTR_EL0.
> > 
> 
> Ah yes, I missed that fact.
> 
> > ... and AFAICT we program PMSELR_EL0 appropriately to select the cycle counter.
> > 
> > I think we're erroneously programming the RES0 bits of PMCCFILTR_EL0, but we're
> > also relying on this write to clear all the filter controls in the high bits of
> > PMCCFILTR_EL0, which otherwise reset to UNKNOWN values.
> > 
> > Given that, I think we also need to explicitly reset PMCCFILTR_EL0 in
> > armv8pmu_reset().
> > 
> 
> I just realized that for the PMCCFILTR_EL0 there is also the exclude
> control bits for the event. Do those not need to be set when we enable
> the event rather than on reset?

Yes, you're right. We will need to configure thos, so just doing the reset
isn't enough.

> Now, I'm thinking what we need is to use a different mask than
> EVTYPE_MASK (one that excludes the event bits) when setting
> PMCCFILTR_EL0, but still do that when enabling an event using the cycle
> counter.
> 
> Does that approach sound correct? Or am I missing something that would
> allow us to only set that upon reset?

That sounds right to me!

It would perhaps be nicer if we could configure event->hwc.config_base to be
the raw register value in all cases, but it looks like that's going to be more
painful than special-casing the cycle counter here.

Thanks,
Mark.

Julien Thierry April 3, 2019, 7:26 a.m. UTC | #4

On 03/04/2019 04:10, Mark Rutland wrote:
> On Tue, Apr 02, 2019 at 04:42:58PM +0100, Julien Thierry wrote:
>>
>>
>> On 01/04/2019 09:02, Mark Rutland wrote:
>>> On Thu, Mar 28, 2019 at 09:49:05AM +0000, Julien Thierry wrote:
>>>> Perf events using the dedicated cycle counter do not need to program the
>>>> event type, the counter only ever counts that kind of events.
>>>
>>> Good catch!
>>>
>>>> Even worse, trying to program an event type without excluding the
>>>> cycle counter index might end up in the modification of the type of
>>>> event counted by another perf event.
>>>
>>> IIUC, we shouldn't affect an unrelated event. The ARM ARM says:
>>>
>>>   When PMSELR_EL0.SEL == 31, this register accesses PMCCFILTR_EL0.
>>>
>>
>> Ah yes, I missed that fact.
>>
>>> ... and AFAICT we program PMSELR_EL0 appropriately to select the cycle counter.
>>>
>>> I think we're erroneously programming the RES0 bits of PMCCFILTR_EL0, but we're
>>> also relying on this write to clear all the filter controls in the high bits of
>>> PMCCFILTR_EL0, which otherwise reset to UNKNOWN values.
>>>
>>> Given that, I think we also need to explicitly reset PMCCFILTR_EL0 in
>>> armv8pmu_reset().
>>>
>>
>> I just realized that for the PMCCFILTR_EL0 there is also the exclude
>> control bits for the event. Do those not need to be set when we enable
>> the event rather than on reset?
> 
> Yes, you're right. We will need to configure thos, so just doing the reset
> isn't enough.
> 
>> Now, I'm thinking what we need is to use a different mask than
>> EVTYPE_MASK (one that excludes the event bits) when setting
>> PMCCFILTR_EL0, but still do that when enabling an event using the cycle
>> counter.
>>
>> Does that approach sound correct? Or am I missing something that would
>> allow us to only set that upon reset?
> 
> That sounds right to me!
> 
> It would perhaps be nicer if we could configure event->hwc.config_base to be
> the raw register value in all cases, but it looks like that's going to be more
> painful than special-casing the cycle counter here.
> 

Right, I'm gonna need to special case the cycle counter to adapt the
event type writing to this patch[1]. But since this is not as
critical/beneficial as I initially thought, I'll just incorporate the
special-casing to the PMU interrupt as NMI series.

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2019-March/640535.html

Thanks,

Will Deacon April 3, 2019, 1:54 p.m. UTC | #5

On Wed, Apr 03, 2019 at 08:26:20AM +0100, Julien Thierry wrote:
> On 03/04/2019 04:10, Mark Rutland wrote:
> > On Tue, Apr 02, 2019 at 04:42:58PM +0100, Julien Thierry wrote:
> >> On 01/04/2019 09:02, Mark Rutland wrote:
> >>> On Thu, Mar 28, 2019 at 09:49:05AM +0000, Julien Thierry wrote:
> >> Now, I'm thinking what we need is to use a different mask than
> >> EVTYPE_MASK (one that excludes the event bits) when setting
> >> PMCCFILTR_EL0, but still do that when enabling an event using the cycle
> >> counter.
> >>
> >> Does that approach sound correct? Or am I missing something that would
> >> allow us to only set that upon reset?
> > 
> > That sounds right to me!
> > 
> > It would perhaps be nicer if we could configure event->hwc.config_base to be
> > the raw register value in all cases, but it looks like that's going to be more
> > painful than special-casing the cycle counter here.
> > 
> 
> Right, I'm gonna need to special case the cycle counter to adapt the
> event type writing to this patch[1]. But since this is not as
> critical/beneficial as I initially thought, I'll just incorporate the
> special-casing to the PMU interrupt as NMI series.

I'm confused as to what we're actually fixing here. PMCCFILTR_EL0 and
PMEVTYPER<n>_EL0 have the same layout, modulo some RES0 bits, so the
existing code should work fine afaict.

What's the problem?

Will

Julien Thierry April 3, 2019, 2:09 p.m. UTC | #6

On 03/04/2019 14:54, Will Deacon wrote:
> On Wed, Apr 03, 2019 at 08:26:20AM +0100, Julien Thierry wrote:
>> On 03/04/2019 04:10, Mark Rutland wrote:
>>> On Tue, Apr 02, 2019 at 04:42:58PM +0100, Julien Thierry wrote:
>>>> On 01/04/2019 09:02, Mark Rutland wrote:
>>>>> On Thu, Mar 28, 2019 at 09:49:05AM +0000, Julien Thierry wrote:
>>>> Now, I'm thinking what we need is to use a different mask than
>>>> EVTYPE_MASK (one that excludes the event bits) when setting
>>>> PMCCFILTR_EL0, but still do that when enabling an event using the cycle
>>>> counter.
>>>>
>>>> Does that approach sound correct? Or am I missing something that would
>>>> allow us to only set that upon reset?
>>>
>>> That sounds right to me!
>>>
>>> It would perhaps be nicer if we could configure event->hwc.config_base to be
>>> the raw register value in all cases, but it looks like that's going to be more
>>> painful than special-casing the cycle counter here.
>>>
>>
>> Right, I'm gonna need to special case the cycle counter to adapt the
>> event type writing to this patch[1]. But since this is not as
>> critical/beneficial as I initially thought, I'll just incorporate the
>> special-casing to the PMU interrupt as NMI series.
> 
> I'm confused as to what we're actually fixing here. PMCCFILTR_EL0 and
> PMEVTYPER<n>_EL0 have the same layout, modulo some RES0 bits, so the
> existing code should work fine afaict.
> 
> What's the problem?
> 

None at the moment, I failed to realize that PMSELR did allow to select
the counter register. But issues start to arise once we try to replace
the "select + access" register with the "direct access" registers for
counter and evtype. But that is another story that fits into another
patch series.

Sorry for the confusion.

Thanks,

Patch

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 4addb38..3f898bc 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -514,7 +514,7 @@  static inline void armv8pmu_write_event_type(struct perf_event *event)

 		armv8pmu_write_evtype(idx - 1, hwc->config_base);
 		armv8pmu_write_evtype(idx, chain_evt);
-	} else {
+	} else if (idx != ARMV8_IDX_CYCLE_COUNTER) {
 		armv8pmu_write_evtype(idx, hwc->config_base);
 	}
 }