Re: [RFCv1 4/4] perf: arm_spe: Dynamically switch PID tracing to contextidr

From: Kees Cook <keescook@chromium.org>
To: Leo Yan <leo.yan@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Ard Biesheuvel <ardb@kernel.org>,
	Sami Tolvanen <samitolvanen@google.com>,
	Nicholas Piggin <npiggin@gmail.com>,
	James Morse <james.morse@arm.com>, Marc Zyngier <maz@kernel.org>,
	Joey Gouly <joey.gouly@arm.com>,
	Peter Collingbourne <pcc@google.com>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	"Peter Zijlstra (Intel)" <peterz@infradead.org>,
	Stephane Eranian <eranian@google.com>,
	James Clark <james.clark@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [RFCv1 4/4] perf: arm_spe: Dynamically switch PID tracing to contextidr
Date: Thu, 21 Oct 2021 08:49:46 -0700	[thread overview]
Message-ID: <202110210848.35971643C6@keescook> (raw)
In-Reply-To: <20211021134530.206216-5-leo.yan@linaro.org>

On Thu, Oct 21, 2021 at 09:45:30PM +0800, Leo Yan wrote:
> Now Arm64 provides API for enabling and disable PID tracing, Arm SPE
> driver invokes these functions to dynamically enable it during
> profiling when the program runs in root PID name space, and disable PID
> tracing when the perf event is stopped.
> 
> Device drivers should not depend on CONFIG_PID_IN_CONTEXTIDR for PID
> tracing, so this patch uses the consistent condition for setting bit
> EL1_CX for PMSCR.

My own preference here would be to not bother with the new
enable/disable helpers, but just open code it right here. (Save a patch
and is the only user.) But I defer to the taste of arm64 maintainers. :)

-Kees

> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  drivers/perf/arm_spe_pmu.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> index d44bcc29d99c..935343cdcb39 100644
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c
> @@ -23,6 +23,7 @@
>  #include <linux/irq.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> +#include <linux/mmu_context.h>
>  #include <linux/module.h>
>  #include <linux/of_address.h>
>  #include <linux/of_device.h>
> @@ -272,7 +273,7 @@ static u64 arm_spe_event_to_pmscr(struct perf_event *event)
>  	if (!attr->exclude_kernel)
>  		reg |= BIT(SYS_PMSCR_EL1_E1SPE_SHIFT);
>  
> -	if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable())
> +	if (perfmon_capable() && (task_active_pid_ns(current) == &init_pid_ns))
>  		reg |= BIT(SYS_PMSCR_EL1_CX_SHIFT);
>  
>  	return reg;
> @@ -731,6 +732,13 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags)
>  	if (hwc->state)
>  		return;
>  
> +	/*
> +	 * Enable tracing PID to contextidr if profiling program runs in
> +	 * root PID namespace.
> +	 */
> +	if (perfmon_capable() && (task_active_pid_ns(current) == &init_pid_ns))
> +		contextidr_enable();
> +
>  	reg = arm_spe_event_to_pmsfcr(event);
>  	write_sysreg_s(reg, SYS_PMSFCR_EL1);
>  
> @@ -792,6 +800,9 @@ static void arm_spe_pmu_stop(struct perf_event *event, int flags)
>  	}
>  
>  	hwc->state |= PERF_HES_STOPPED;
> +
> +	if (perfmon_capable() && (task_active_pid_ns(current) == &init_pid_ns))
> +		contextidr_disable();
>  }
>  
>  static int arm_spe_pmu_add(struct perf_event *event, int flags)
> -- 
> 2.25.1
> 

-- 
Kees Cook

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