Re: [PATCH v1 2/7] perf cs_etm: Use pid tracing explicitly instead of contextid

From: Mathieu Poirier <mathieu.poirier@linaro.org>
To: Leo Yan <leo.yan@linaro.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Mike Leach <mike.leach@linaro.org>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	John Garry <john.garry@huawei.com>, Will Deacon <will@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Mark Rutland <mark.rutland@arm.com>, Jiri Olsa <jolsa@redhat.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Daniel Kiss <Daniel.Kiss@arm.com>,
	Denis Nikitin <denik@chromium.org>,
	coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Al Grant <al.grant@arm.com>
Subject: Re: [PATCH v1 2/7] perf cs_etm: Use pid tracing explicitly instead of contextid
Date: Fri, 15 Jan 2021 15:44:16 -0700	[thread overview]
Message-ID: <20210115224416.GB375055@xps15> (raw)
In-Reply-To: <20210109074435.626855-3-leo.yan@linaro.org>

On Sat, Jan 09, 2021 at 03:44:30PM +0800, Leo Yan wrote:
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
> If the kernel is running at EL2, the pid of the task is exposed
> via VMID instead of the CONTEXTID. Add support for this in the
> perf tool.
> 
> By default the perf tool requests contextid and timestamp for
> task bound events. Instead of hard coding contextid, switch
> to "pid" config exposed by the kernel. While at it, define new
> independent macros (rather than using the "config" bits) for
> requesting the "pid" and "timestamp" for cs_etm_set_option(),
> since the PID config is now dynamic depending on the kernel
> exception level.
> 
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Al Grant <al.grant@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/include/linux/coresight-pmu.h | 11 +++--
>  tools/perf/arch/arm/util/cs-etm.c   | 68 ++++++++++++++++++++++-------
>  2 files changed, 59 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h
> index b0e35eec6499..927c6285ce5d 100644
> --- a/tools/include/linux/coresight-pmu.h
> +++ b/tools/include/linux/coresight-pmu.h
> @@ -11,16 +11,19 @@
>  #define CORESIGHT_ETM_PMU_SEED  0x10
>  
>  /* ETMv3.5/PTM's ETMCR config bit */
> -#define ETM_OPT_CYCACC  12
> -#define ETM_OPT_CTXTID	14
> -#define ETM_OPT_TS      28
> -#define ETM_OPT_RETSTK	29
> +#define ETM_OPT_CYCACC		12
> +#define ETM_OPT_CTXTID		14
> +#define ETM_OPT_CTXTID_IN_VMID	15
> +#define ETM_OPT_TS		28
> +#define ETM_OPT_RETSTK		29
>  
>  /* ETMv4 CONFIGR programming bits for the ETM OPTs */
>  #define ETM4_CFG_BIT_CYCACC	4
>  #define ETM4_CFG_BIT_CTXTID	6
> +#define ETM4_CFG_BIT_VMID	7
>  #define ETM4_CFG_BIT_TS		11
>  #define ETM4_CFG_BIT_RETSTK	12
> +#define ETM4_CFG_BIT_VMID_OPT	15

Wasn't this done in the previous patch?

>  
>  static inline int coresight_get_trace_id(int cpu)
>  {
> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
> index cad7bf783413..fad7b6e13ccc 100644
> --- a/tools/perf/arch/arm/util/cs-etm.c
> +++ b/tools/perf/arch/arm/util/cs-etm.c
> @@ -59,14 +59,15 @@ static const char *metadata_etmv4_ro[CS_ETMV4_PRIV_MAX] = {
>  
>  static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu);
>  
> -static int cs_etm_set_context_id(struct auxtrace_record *itr,
> -				 struct evsel *evsel, int cpu)
> +static int cs_etm_set_pid(struct auxtrace_record *itr,
> +			  struct evsel *evsel, int cpu)
>  {
>  	struct cs_etm_recording *ptr;
>  	struct perf_pmu *cs_etm_pmu;
>  	char path[PATH_MAX];
>  	int err = -EINVAL;
>  	u32 val;
> +	u64 pid_fmt;
>  
>  	ptr = container_of(itr, struct cs_etm_recording, itr);
>  	cs_etm_pmu = ptr->cs_etm_pmu;
> @@ -86,21 +87,50 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr,
>  		goto out;
>  	}
>  
> +	pid_fmt = perf_pmu__format_bits(&cs_etm_pmu->format, "pid");

If we go with my suggestion from patch 1 this would become "contextid".

I think what follows below is all good.

>  	/*
> -	 * TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID tracing
> -	 * is supported:
> -	 *  0b00000 Context ID tracing is not supported.
> -	 *  0b00100 Maximum of 32-bit Context ID size.
> -	 *  All other values are reserved.
> +	 * If the kernel doesn't support the "pid" format (older kernel),
> +	 * fall back to using the CTXTID.
>  	 */
> -	val = BMVAL(val, 5, 9);
> -	if (!val || val != 0x4) {
> +	if (!pid_fmt)
> +		pid_fmt = 1ULL << ETM_OPT_CTXTID;
> +
> +	switch (pid_fmt) {
> +	case (1ULL << ETM_OPT_CTXTID):
> +		/*
> +		 * TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID
> +		 * tracing is supported:
> +		 *  0b00000 Context ID tracing is not supported.
> +		 *  0b00100 Maximum of 32-bit Context ID size.
> +		 *  All other values are reserved.
> +		 */
> +		val = BMVAL(val, 5, 9);
> +		if (!val || val != 0x4) {
> +			err = -EINVAL;
> +			goto out;
> +		}
> +		break;
> +	case (1ULL << ETM_OPT_CTXTID_IN_VMID):
> +		/*
> +		 * TRCIDR2.VMIDOPT[30:29] != 0 and
> +		 * TRCIDR2.VMIDSIZE[14:10] == 0b00100 (32bit virtual contextid)
> +		 * We can't support CONTEXTIDR in VMID if the size of the
> +		 * virtual context id is < 32bit.
> +		 * Any value of VMIDSIZE >= 4 (i.e, > 32bit) is fine for us.
> +		 */
> +		if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) < 4) {
> +			err = -EINVAL;
> +			goto out;
> +		}
> +		break;
> +	default:
>  		err = -EINVAL;
>  		goto out;
>  	}
>  
> +
>  	/* All good, let the kernel know */
> -	evsel->core.attr.config |= (1 << ETM_OPT_CTXTID);
> +	evsel->core.attr.config |= pid_fmt;
>  	err = 0;
>  
>  out:
> @@ -156,6 +186,10 @@ static int cs_etm_set_timestamp(struct auxtrace_record *itr,
>  	return err;
>  }
>  
> +#define ETM_SET_OPT_PID		(1 << 0)
> +#define ETM_SET_OPT_TS		(1 << 1)
> +#define ETM_SET_OPT_MASK	(ETM_SET_OPT_PID | ETM_SET_OPT_TS)
> +

I don't think we need this as part of the current patchset.  Back in the days I
thought it would be a good idea to use the ETMv3.5/PTM configuration bits but in
hindsight it wasn't.  That's what happens when your crystal ball isn't working.

That being said we can start treating the perf_event_attr::config bits
arbitrarily.  Someone pointed out that it might get confusing but I don't think
it is anything that can't be overcomed with good in-line documentation.   

>  static int cs_etm_set_option(struct auxtrace_record *itr,
>  			     struct evsel *evsel, u32 option)
>  {
> @@ -169,17 +203,17 @@ static int cs_etm_set_option(struct auxtrace_record *itr,
>  		    !cpu_map__has(online_cpus, i))
>  			continue;
>  
> -		if (option & ETM_OPT_CTXTID) {
> -			err = cs_etm_set_context_id(itr, evsel, i);
> +		if (option & ETM_SET_OPT_PID) {
> +			err = cs_etm_set_pid(itr, evsel, i);
>  			if (err)
>  				goto out;
>  		}
> -		if (option & ETM_OPT_TS) {
> +		if (option & ETM_SET_OPT_TS) {
>  			err = cs_etm_set_timestamp(itr, evsel, i);
>  			if (err)
>  				goto out;
>  		}
> -		if (option & ~(ETM_OPT_CTXTID | ETM_OPT_TS))
> +		if (option & ~(ETM_SET_OPT_MASK))
>  			/* Nothing else is currently supported */
>  			goto out;
>  	}
> @@ -406,7 +440,7 @@ static int cs_etm_recording_options(struct auxtrace_record *itr,
>  		evsel__set_sample_bit(cs_etm_evsel, CPU);
>  
>  		err = cs_etm_set_option(itr, cs_etm_evsel,
> -					ETM_OPT_CTXTID | ETM_OPT_TS);
> +					ETM_SET_OPT_PID | ETM_SET_OPT_TS);
>  		if (err)
>  			goto out;
>  	}
> @@ -485,7 +519,9 @@ static u64 cs_etmv4_get_config(struct auxtrace_record *itr)
>  		config |= BIT(ETM4_CFG_BIT_TS);
>  	if (config_opts & BIT(ETM_OPT_RETSTK))
>  		config |= BIT(ETM4_CFG_BIT_RETSTK);
> -
> +	if (config_opts & BIT(ETM_OPT_CTXTID_IN_VMID))
> +		config |= BIT(ETM4_CFG_BIT_VMID) |
> +			  BIT(ETM4_CFG_BIT_VMID_OPT);
>  	return config;
>  }
>  
> -- 
> 2.25.1
> 