| Message ID | 20200617113851.607706-2-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm_pmu: Use NMI for perf interrupt | expand |
Quoting Alexandru Elisei (2020-06-17 04:38:45) > Writes to the PMXEVTYPER_EL0 register are not self-synchronising. In > armv8pmu_enable_event(), the PE can reorder configuring the event type > after we have enabled the counter and the interrupt. This can lead to an > interrupt being asserted because the of the previous event type that we 'because the of the' doesn't read properly. > were counting, not the one that we've just enabled. > > The same rationale applies to writes to the PMINTENSET_EL1 register. The PE > can reorder enabling the interrupt at any point in the future after we have > enabled the event. > > Prevent both situations from happening by adding an ISB just before we > enable the event counter. > > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c > index 4d7879484cec..ee180b2a5b39 100644 > --- a/arch/arm64/kernel/perf_event.c > +++ b/arch/arm64/kernel/perf_event.c > @@ -605,6 +605,7 @@ static void armv8pmu_enable_event(struct perf_event *event) > * Enable interrupt for this counter > */ > armv8pmu_enable_event_irq(event); > + isb(); Please add a comment before the isb() explaining the situation. Nobody knows what this is for when reading the code and they don't want to do git archaeology to figure it out.
Hi Stephen, Thank you very much for taking the time to review the patches! Comments below. On 6/17/20 9:01 PM, Stephen Boyd wrote: > Quoting Alexandru Elisei (2020-06-17 04:38:45) >> Writes to the PMXEVTYPER_EL0 register are not self-synchronising. In >> armv8pmu_enable_event(), the PE can reorder configuring the event type >> after we have enabled the counter and the interrupt. This can lead to an >> interrupt being asserted because the of the previous event type that we > 'because the of the' doesn't read properly. Typo on my part, will fix it. > >> were counting, not the one that we've just enabled. >> >> The same rationale applies to writes to the PMINTENSET_EL1 register. The PE >> can reorder enabling the interrupt at any point in the future after we have >> enabled the event. >> >> Prevent both situations from happening by adding an ISB just before we >> enable the event counter. >> >> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c >> index 4d7879484cec..ee180b2a5b39 100644 >> --- a/arch/arm64/kernel/perf_event.c >> +++ b/arch/arm64/kernel/perf_event.c >> @@ -605,6 +605,7 @@ static void armv8pmu_enable_event(struct perf_event *event) >> * Enable interrupt for this counter >> */ >> armv8pmu_enable_event_irq(event); >> + isb(); > Please add a comment before the isb() explaining the situation. Nobody > knows what this is for when reading the code and they don't want to do > git archaeology to figure it out. That's a good idea, I'll do that. Thanks, Alex
diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c index 4d7879484cec..ee180b2a5b39 100644 --- a/arch/arm64/kernel/perf_event.c +++ b/arch/arm64/kernel/perf_event.c @@ -605,6 +605,7 @@ static void armv8pmu_enable_event(struct perf_event *event) * Enable interrupt for this counter */ armv8pmu_enable_event_irq(event); + isb(); /* * Enable counter
Writes to the PMXEVTYPER_EL0 register are not self-synchronising. In armv8pmu_enable_event(), the PE can reorder configuring the event type after we have enabled the counter and the interrupt. This can lead to an interrupt being asserted because the of the previous event type that we were counting, not the one that we've just enabled. The same rationale applies to writes to the PMINTENSET_EL1 register. The PE can reorder enabling the interrupt at any point in the future after we have enabled the event. Prevent both situations from happening by adding an ISB just before we enable the event counter. Cc: Julien Thierry <julien.thierry.kdev@gmail.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Arnaldo Carvalho de Melo <acme@kernel.org> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Jiri Olsa <jolsa@redhat.com> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Fixes: 030896885ade ("arm64: Performance counters support") Reported-by: Julien Thierry <julien.thierry@arm.com> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- arch/arm64/kernel/perf_event.c | 1 + 1 file changed, 1 insertion(+)