Re: [PATCH v3 3/9] arm: perf: save/resore pmsel

[Mark Rutland <mark.rutland@arm.com>]

On Mon, Jul 08, 2019 at 03:32:51PM +0100, Julien Thierry wrote:
> The callback pmu->read() can be called with interrupts enabled.
> Currently, on ARM, this can cause the following callchain:
> 
> armpmu_read() -> armpmu_event_update() -> armv7pmu_read_counter()
> 
> The last function might modify the counter selector register and then
> read the target counter, without taking any lock. With interrupts
> enabled, a PMU interrupt could occur and modify the selector register
> as well, between the selection and read of the interrupted context.
> 
> Save and restore the value of the selector register in the PMU interrupt
> handler, ensuring the interrupted context is left with the correct PMU
> registers selected.

IIUC, this is a latent bug, so I guess it should be Cc'd stable?

> 
> Signed-off-by: Julien Thierry <julien.thierry@arm.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: Russell King <linux@armlinux.org.uk>
> ---
>  arch/arm/kernel/perf_event_v7.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
> index a4fb0f8..c3da7a5 100644
> --- a/arch/arm/kernel/perf_event_v7.c
> +++ b/arch/arm/kernel/perf_event_v7.c
> @@ -736,10 +736,22 @@ static inline int armv7_pmnc_counter_has_overflowed(u32 pmnc, int idx)
>  	return pmnc & BIT(ARMV7_IDX_TO_COUNTER(idx));
>  }
> 
> -static inline void armv7_pmnc_select_counter(int idx)
> +static inline u32 armv7_pmsel_read(void)
> +{
> +	u32 pmsel;
> +
> +	asm volatile("mrc p15, 0, %0, c9, c12, 5" : "=&r" (pmsel));
> +	return pmsel;
> +}
> +
> +static inline void armv7_pmsel_write(u32 counter)
>  {
> -	u32 counter = ARMV7_IDX_TO_COUNTER(idx);
>  	asm volatile("mcr p15, 0, %0, c9, c12, 5" : : "r" (counter));
> +}
> +
> +static inline void armv7_pmnc_select_counter(int idx)
> +{
> +	armv7_pmsel_write(ARMV7_IDX_TO_COUNTER(idx));
>  	isb();
>  }
> 
> @@ -952,8 +964,11 @@ static irqreturn_t armv7pmu_handle_irq(struct arm_pmu *cpu_pmu)
>  	struct perf_sample_data data;
>  	struct pmu_hw_events *cpuc = this_cpu_ptr(cpu_pmu->hw_events);
>  	struct pt_regs *regs;
> +	u32 pmsel;
>  	int idx;
> 
> +	pmsel = armv7_pmsel_read();

Could we add a comment explaining why we need to save/restore this?

Otherwise, this looks good to me.

Thanks,
Mark.

> +
>  	/*
>  	 * Get and reset the IRQ flags
>  	 */
> @@ -1004,6 +1019,8 @@ static irqreturn_t armv7pmu_handle_irq(struct arm_pmu *cpu_pmu)
>  	 */
>  	irq_work_run();
> 
> +	armv7_pmsel_write(pmsel);
> +
>  	return IRQ_HANDLED;
>  }
> 
> --
> 1.9.1

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