Re: [PATCH v2] drivers/perf: arm_spe: Use perf_allow_kernel() for permissions

[Will Deacon <will@kernel.org>]

On Wed, Aug 07, 2024 at 04:51:53PM +0100, James Clark wrote:
> For other PMUs, PERF_SAMPLE_PHYS_ADDR requires perf_allow_kernel()
> rather than just perfmon_capable(). Because PMSCR_EL1_PA is another form
> of physical address, make it consistent and use perf_allow_kernel() for
> SPE as well. PMSCR_EL1_PCT and PMSCR_EL1_CX also get the same change.
> 
> This improves consistency and indirectly fixes the following error
> message which is misleading because perf_event_paranoid is not taken
> into account by perfmon_capable():
> 
>   $ perf record -e arm_spe/pa_enable/
> 
>   Error:
>   Access to performance monitoring and observability operations is
>   limited. Consider adjusting /proc/sys/kernel/perf_event_paranoid
>   setting ...
> 
> Suggested-by: Al Grant <al.grant@arm.com>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
> Changes since v1:
> 
>   * Export perf_allow_kernel() instead of sysctl_perf_event_paranoid
> 
>  drivers/perf/arm_spe_pmu.c | 9 ++++-----
>  include/linux/perf_event.h | 8 +-------
>  kernel/events/core.c       | 9 +++++++++
>  3 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> index 9100d82bfabc..3569050f9cf3 100644
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c
> @@ -41,7 +41,7 @@
>  
>  /*
>   * Cache if the event is allowed to trace Context information.
> - * This allows us to perform the check, i.e, perfmon_capable(),
> + * This allows us to perform the check, i.e, perf_allow_kernel(),
>   * in the context of the event owner, once, during the event_init().
>   */
>  #define SPE_PMU_HW_FLAGS_CX			0x00001
> @@ -50,7 +50,7 @@ static_assert((PERF_EVENT_FLAG_ARCH & SPE_PMU_HW_FLAGS_CX) == SPE_PMU_HW_FLAGS_C
>  
>  static void set_spe_event_has_cx(struct perf_event *event)
>  {
> -	if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable())
> +	if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && !perf_allow_kernel(&event->attr))
>  		event->hw.flags |= SPE_PMU_HW_FLAGS_CX;

The rationale for this change in the commit message is because other
drivers gate PERF_SAMPLE_PHYS_ADDR on perf_allow_kernel(). However,
putting the PID in contextidr doesn't seem to have anything to do with
that...

>  }
>  
> @@ -745,9 +745,8 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
>  
>  	set_spe_event_has_cx(event);
>  	reg = arm_spe_event_to_pmscr(event);
> -	if (!perfmon_capable() &&
> -	    (reg & (PMSCR_EL1_PA | PMSCR_EL1_PCT)))
> -		return -EACCES;
> +	if (reg & (PMSCR_EL1_PA | PMSCR_EL1_PCT))

Similarly here. What does the physical counter have to do with physical
address sampling other than sharing the word "physical"?

Will