[PATCH 0/6] Support ETE decoding

[PATCH 0/6] Support ETE decoding

[James Clark]

Decoding ETE already works because it is a superset of
ETMv4, but if any new packet types are found then they will be
ignored by the decoder. This patchset creates an ETE decoder
which can output the new packets and saves a new register that
is required. No new packet types are handled by perf yet, as this
can be added in the future.

This set applies on top of "perf cs-etm: Support TRBE
(unformatted decoding)" on perf/core.

James Clark (6):
  perf cs-etm: Refactor initialisation of decoder params.
  perf cs-etm: Initialise architecture based on TRCIDR1
  perf cs-etm: Save TRCDEVARCH register
  perf cs-etm: Update OpenCSD decoder for ETE
  perf cs-etm: Create ETE decoder
  perf cs-etm: Print the decoder name

 tools/build/feature/test-libopencsd.c         |   4 +-
 tools/perf/arch/arm/util/cs-etm.c             |  13 +-
 .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 151 ++++++++----------
 .../perf/util/cs-etm-decoder/cs-etm-decoder.h |   8 +
 tools/perf/util/cs-etm.c                      |  54 ++++++-
 tools/perf/util/cs-etm.h                      |   6 +-
 6 files changed, 147 insertions(+), 89 deletions(-)

-- 
2.28.0

[PATCH 1/6] perf cs-etm: Refactor initialisation of decoder params.

[James Clark]

The initialisation of the decoder params is duplicated between
creation of the packet printer and packet decoder. Put them both
into one function so that future changes only need to be made in one
place.

Signed-off-by: James Clark <james.clark@arm.com>
---
 .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 99 +++++--------------
 1 file changed, 25 insertions(+), 74 deletions(-)

diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
index ed1f0326f859..30889a9d0165 100644
--- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
+++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
@@ -227,55 +227,6 @@ cs_etm_decoder__init_raw_frame_logging(
 }
 #endif
 
-static int cs_etm_decoder__create_packet_printer(struct cs_etm_decoder *decoder,
-						 const char *decoder_name,
-						 void *trace_config)
-{
-	u8 csid;
-
-	if (ocsd_dt_create_decoder(decoder->dcd_tree, decoder_name,
-				   OCSD_CREATE_FLG_PACKET_PROC,
-				   trace_config, &csid))
-		return -1;
-
-	if (ocsd_dt_set_pkt_protocol_printer(decoder->dcd_tree, csid, 0))
-		return -1;
-
-	return 0;
-}
-
-static int
-cs_etm_decoder__create_etm_packet_printer(struct cs_etm_trace_params *t_params,
-					  struct cs_etm_decoder *decoder)
-{
-	const char *decoder_name;
-	ocsd_etmv3_cfg config_etmv3;
-	ocsd_etmv4_cfg trace_config_etmv4;
-	void *trace_config;
-
-	switch (t_params->protocol) {
-	case CS_ETM_PROTO_ETMV3:
-	case CS_ETM_PROTO_PTM:
-		cs_etm_decoder__gen_etmv3_config(t_params, &config_etmv3);
-		decoder_name = (t_params->protocol == CS_ETM_PROTO_ETMV3) ?
-							OCSD_BUILTIN_DCD_ETMV3 :
-							OCSD_BUILTIN_DCD_PTM;
-		trace_config = &config_etmv3;
-		break;
-	case CS_ETM_PROTO_ETMV4i:
-		cs_etm_decoder__gen_etmv4_config(t_params, &trace_config_etmv4);
-		decoder_name = OCSD_BUILTIN_DCD_ETMV4I;
-		trace_config = &trace_config_etmv4;
-		break;
-	default:
-		return -1;
-	}
-
-	return cs_etm_decoder__create_packet_printer(decoder,
-						     decoder_name,
-						     trace_config);
-}
-
 static ocsd_datapath_resp_t
 cs_etm_decoder__do_soft_timestamp(struct cs_etm_queue *etmq,
 				  struct cs_etm_packet_queue *packet_queue,
@@ -629,9 +580,10 @@ static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer(
 	return resp;
 }
 
-static int cs_etm_decoder__create_etm_packet_decoder(
-					struct cs_etm_trace_params *t_params,
-					struct cs_etm_decoder *decoder)
+static int
+cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
+				   struct cs_etm_trace_params *t_params,
+				   struct cs_etm_decoder *decoder)
 {
 	const char *decoder_name;
 	ocsd_etmv3_cfg config_etmv3;
@@ -657,31 +609,30 @@ static int cs_etm_decoder__create_etm_packet_decoder(
 		return -1;
 	}
 
-	if (ocsd_dt_create_decoder(decoder->dcd_tree,
-				     decoder_name,
-				     OCSD_CREATE_FLG_FULL_DECODER,
-				     trace_config, &csid))
-		return -1;
+	if (d_params->operation == CS_ETM_OPERATION_DECODE) {
+		if (ocsd_dt_create_decoder(decoder->dcd_tree,
+					   decoder_name,
+					   OCSD_CREATE_FLG_FULL_DECODER,
+					   trace_config, &csid))
+			return -1;
 
-	if (ocsd_dt_set_gen_elem_outfn(decoder->dcd_tree,
-				       cs_etm_decoder__gen_trace_elem_printer,
-				       decoder))
-		return -1;
+		if (ocsd_dt_set_gen_elem_outfn(decoder->dcd_tree,
+					       cs_etm_decoder__gen_trace_elem_printer,
+					       decoder))
+			return -1;
 
-	return 0;
-}
+		return 0;
+	} else if (d_params->operation == CS_ETM_OPERATION_PRINT) {
+		if (ocsd_dt_create_decoder(decoder->dcd_tree, decoder_name,
+					   OCSD_CREATE_FLG_PACKET_PROC,
+					   trace_config, &csid))
+			return -1;
 
-static int
-cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
-				   struct cs_etm_trace_params *t_params,
-				   struct cs_etm_decoder *decoder)
-{
-	if (d_params->operation == CS_ETM_OPERATION_PRINT)
-		return cs_etm_decoder__create_etm_packet_printer(t_params,
-								 decoder);
-	else if (d_params->operation == CS_ETM_OPERATION_DECODE)
-		return cs_etm_decoder__create_etm_packet_decoder(t_params,
-								 decoder);
+		if (ocsd_dt_set_pkt_protocol_printer(decoder->dcd_tree, csid, 0))
+			return -1;
+
+		return 0;
+	}
 
 	return -1;
 }
-- 
2.28.0

Re: [PATCH 1/6] perf cs-etm: Refactor initialisation of decoder params.

[Leo Yan]

On Wed, Jul 21, 2021 at 10:07:00AM +0100, James Clark wrote:
> The initialisation of the decoder params is duplicated between
> creation of the packet printer and packet decoder. Put them both
> into one function so that future changes only need to be made in one
> place.
> 
> Signed-off-by: James Clark <james.clark@arm.com>

It's good to wait for Mathieu's review, I reviewed this patch and it's
good for me:

Reviewed-by: Leo Yan <leo.yan@linaro.org>

> ---
>  .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 99 +++++--------------
>  1 file changed, 25 insertions(+), 74 deletions(-)
> 
> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> index ed1f0326f859..30889a9d0165 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> @@ -227,55 +227,6 @@ cs_etm_decoder__init_raw_frame_logging(
>  }
>  #endif
>  
> -static int cs_etm_decoder__create_packet_printer(struct cs_etm_decoder *decoder,
> -						 const char *decoder_name,
> -						 void *trace_config)
> -{
> -	u8 csid;
> -
> -	if (ocsd_dt_create_decoder(decoder->dcd_tree, decoder_name,
> -				   OCSD_CREATE_FLG_PACKET_PROC,
> -				   trace_config, &csid))
> -		return -1;
> -
> -	if (ocsd_dt_set_pkt_protocol_printer(decoder->dcd_tree, csid, 0))
> -		return -1;
> -
> -	return 0;
> -}
> -
> -static int
> -cs_etm_decoder__create_etm_packet_printer(struct cs_etm_trace_params *t_params,
> -					  struct cs_etm_decoder *decoder)
> -{
> -	const char *decoder_name;
> -	ocsd_etmv3_cfg config_etmv3;
> -	ocsd_etmv4_cfg trace_config_etmv4;
> -	void *trace_config;
> -
> -	switch (t_params->protocol) {
> -	case CS_ETM_PROTO_ETMV3:
> -	case CS_ETM_PROTO_PTM:
> -		cs_etm_decoder__gen_etmv3_config(t_params, &config_etmv3);
> -		decoder_name = (t_params->protocol == CS_ETM_PROTO_ETMV3) ?
> -							OCSD_BUILTIN_DCD_ETMV3 :
> -							OCSD_BUILTIN_DCD_PTM;
> -		trace_config = &config_etmv3;
> -		break;
> -	case CS_ETM_PROTO_ETMV4i:
> -		cs_etm_decoder__gen_etmv4_config(t_params, &trace_config_etmv4);
> -		decoder_name = OCSD_BUILTIN_DCD_ETMV4I;
> -		trace_config = &trace_config_etmv4;
> -		break;
> -	default:
> -		return -1;
> -	}
> -
> -	return cs_etm_decoder__create_packet_printer(decoder,
> -						     decoder_name,
> -						     trace_config);
> -}
> -
>  static ocsd_datapath_resp_t
>  cs_etm_decoder__do_soft_timestamp(struct cs_etm_queue *etmq,
>  				  struct cs_etm_packet_queue *packet_queue,
> @@ -629,9 +580,10 @@ static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer(
>  	return resp;
>  }
>  
> -static int cs_etm_decoder__create_etm_packet_decoder(
> -					struct cs_etm_trace_params *t_params,
> -					struct cs_etm_decoder *decoder)
> +static int
> +cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
> +				   struct cs_etm_trace_params *t_params,
> +				   struct cs_etm_decoder *decoder)
>  {
>  	const char *decoder_name;
>  	ocsd_etmv3_cfg config_etmv3;
> @@ -657,31 +609,30 @@ static int cs_etm_decoder__create_etm_packet_decoder(
>  		return -1;
>  	}
>  
> -	if (ocsd_dt_create_decoder(decoder->dcd_tree,
> -				     decoder_name,
> -				     OCSD_CREATE_FLG_FULL_DECODER,
> -				     trace_config, &csid))
> -		return -1;
> +	if (d_params->operation == CS_ETM_OPERATION_DECODE) {
> +		if (ocsd_dt_create_decoder(decoder->dcd_tree,
> +					   decoder_name,
> +					   OCSD_CREATE_FLG_FULL_DECODER,
> +					   trace_config, &csid))
> +			return -1;
>  
> -	if (ocsd_dt_set_gen_elem_outfn(decoder->dcd_tree,
> -				       cs_etm_decoder__gen_trace_elem_printer,
> -				       decoder))
> -		return -1;
> +		if (ocsd_dt_set_gen_elem_outfn(decoder->dcd_tree,
> +					       cs_etm_decoder__gen_trace_elem_printer,
> +					       decoder))
> +			return -1;
>  
> -	return 0;
> -}
> +		return 0;
> +	} else if (d_params->operation == CS_ETM_OPERATION_PRINT) {
> +		if (ocsd_dt_create_decoder(decoder->dcd_tree, decoder_name,
> +					   OCSD_CREATE_FLG_PACKET_PROC,
> +					   trace_config, &csid))
> +			return -1;
>  
> -static int
> -cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
> -				   struct cs_etm_trace_params *t_params,
> -				   struct cs_etm_decoder *decoder)
> -{
> -	if (d_params->operation == CS_ETM_OPERATION_PRINT)
> -		return cs_etm_decoder__create_etm_packet_printer(t_params,
> -								 decoder);
> -	else if (d_params->operation == CS_ETM_OPERATION_DECODE)
> -		return cs_etm_decoder__create_etm_packet_decoder(t_params,
> -								 decoder);
> +		if (ocsd_dt_set_pkt_protocol_printer(decoder->dcd_tree, csid, 0))
> +			return -1;
> +
> +		return 0;
> +	}
>  
>  	return -1;
>  }
> -- 
> 2.28.0
> 

[PATCH 2/6] perf cs-etm: Initialise architecture based on TRCIDR1

[James Clark]

Currently the architecture is hard coded as ARCH_V8, but with the
introduction of ETE we want to pick ARCH_AA64. And this change is also
applicable to ETM v4.4 onwards as well.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
index 30889a9d0165..5972a8afcc6b 100644
--- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
+++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
@@ -126,6 +126,18 @@ static int cs_etm_decoder__gen_etmv3_config(struct cs_etm_trace_params *params,
 	return 0;
 }
 
+#define TRCIDR1_TRCARCHMIN_SHIFT 4
+#define TRCIDR1_TRCARCHMIN_MASK  GENMASK(7, 4)
+#define TRCIDR1_TRCARCHMIN(x)    (((x) & TRCIDR1_TRCARCHMIN_MASK) >> TRCIDR1_TRCARCHMIN_SHIFT)
+static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1)
+{
+	/*
+	 * If the ETM trace minor version is 4 or more then we can assume
+	 * the architecture is ARCH_AA64 rather than just V8
+	 */
+	return TRCIDR1_TRCARCHMIN(reg_idr1) >= 4 ? ARCH_AA64 : ARCH_V8;
+}
+
 static void cs_etm_decoder__gen_etmv4_config(struct cs_etm_trace_params *params,
 					     ocsd_etmv4_cfg *config)
 {
@@ -140,7 +152,7 @@ static void cs_etm_decoder__gen_etmv4_config(struct cs_etm_trace_params *params,
 	config->reg_idr11 = 0;
 	config->reg_idr12 = 0;
 	config->reg_idr13 = 0;
-	config->arch_ver = ARCH_V8;
+	config->arch_ver = cs_etm_decoder__get_arch_ver(params->etmv4.reg_idr1);
 	config->core_prof = profile_CortexA;
 }
 
-- 
2.28.0

Re: [PATCH 2/6] perf cs-etm: Initialise architecture based on TRCIDR1

[Mike Leach]

HI James

On Wed, 21 Jul 2021 at 10:07, James Clark <james.clark@arm.com> wrote:
>
> Currently the architecture is hard coded as ARCH_V8, but with the
> introduction of ETE we want to pick ARCH_AA64. And this change is also
> applicable to ETM v4.4 onwards as well.
>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> index 30889a9d0165..5972a8afcc6b 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> @@ -126,6 +126,18 @@ static int cs_etm_decoder__gen_etmv3_config(struct cs_etm_trace_params *params,
>         return 0;
>  }
>
> +#define TRCIDR1_TRCARCHMIN_SHIFT 4
> +#define TRCIDR1_TRCARCHMIN_MASK  GENMASK(7, 4)
> +#define TRCIDR1_TRCARCHMIN(x)    (((x) & TRCIDR1_TRCARCHMIN_MASK) >> TRCIDR1_TRCARCHMIN_SHIFT)
> +static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1)
> +{
> +       /*
> +        * If the ETM trace minor version is 4 or more then we can assume
> +        * the architecture is ARCH_AA64 rather than just V8
> +        */
> +       return TRCIDR1_TRCARCHMIN(reg_idr1) >= 4 ? ARCH_AA64 : ARCH_V8;
> +}

This is true for ETM4.x & ETE 1.x (arch 5.x) but not ETM 3.x
Probably need to beef up this comment or the function name to emphasise this.

Also only true because we don't currently support AArch32 builds of
the ETM4.x driver.

Regards

Mike

> +
>  static void cs_etm_decoder__gen_etmv4_config(struct cs_etm_trace_params *params,
>                                              ocsd_etmv4_cfg *config)
>  {
> @@ -140,7 +152,7 @@ static void cs_etm_decoder__gen_etmv4_config(struct cs_etm_trace_params *params,
>         config->reg_idr11 = 0;
>         config->reg_idr12 = 0;
>         config->reg_idr13 = 0;
> -       config->arch_ver = ARCH_V8;
> +       config->arch_ver = cs_etm_decoder__get_arch_ver(params->etmv4.reg_idr1);
>         config->core_prof = profile_CortexA;
>  }
>
> --
> 2.28.0
>


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH 2/6] perf cs-etm: Initialise architecture based on TRCIDR1

[Leo Yan]

On Thu, Jul 22, 2021 at 12:10:35PM +0100, Mike Leach wrote:
> HI James
> 
> On Wed, 21 Jul 2021 at 10:07, James Clark <james.clark@arm.com> wrote:
> >
> > Currently the architecture is hard coded as ARCH_V8, but with the
> > introduction of ETE we want to pick ARCH_AA64. And this change is also
> > applicable to ETM v4.4 onwards as well.
> >
> > Signed-off-by: James Clark <james.clark@arm.com>
> > ---
> >  tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > index 30889a9d0165..5972a8afcc6b 100644
> > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > @@ -126,6 +126,18 @@ static int cs_etm_decoder__gen_etmv3_config(struct cs_etm_trace_params *params,
> >         return 0;
> >  }
> >
> > +#define TRCIDR1_TRCARCHMIN_SHIFT 4
> > +#define TRCIDR1_TRCARCHMIN_MASK  GENMASK(7, 4)
> > +#define TRCIDR1_TRCARCHMIN(x)    (((x) & TRCIDR1_TRCARCHMIN_MASK) >> TRCIDR1_TRCARCHMIN_SHIFT)
> > +static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1)
> > +{
> > +       /*
> > +        * If the ETM trace minor version is 4 or more then we can assume
> > +        * the architecture is ARCH_AA64 rather than just V8
> > +        */
> > +       return TRCIDR1_TRCARCHMIN(reg_idr1) >= 4 ? ARCH_AA64 : ARCH_V8;
> > +}
> 
> This is true for ETM4.x & ETE 1.x (arch 5.x) but not ETM 3.x
> Probably need to beef up this comment or the function name to emphasise this.

Yeah, I think it's good to change the function name.  Eventually, this
function should only be used for ETM4.x and ETE.

Another minor comment is: can we refine the arch version number, e.g.
change the OpenCSD's macro "ARCH_AA64" to "ARCH_V8R4", (or
"ARCH_V8R3_AA64"), this can give more clear clue what's the ETM version.

And a nitpick: how about to change OpenCSD macro "ARCH_V8r3" to
"ARCH_V8R3" and assign it for ETMv4.3 IPs.

Thanks,
Leo

Re: [PATCH 2/6] perf cs-etm: Initialise architecture based on TRCIDR1

[Mike Leach]

Hi Leo,

On Sat, 31 Jul 2021 at 07:03, Leo Yan <leo.yan@linaro.org> wrote:
>
> On Thu, Jul 22, 2021 at 12:10:35PM +0100, Mike Leach wrote:
> > HI James
> >
> > On Wed, 21 Jul 2021 at 10:07, James Clark <james.clark@arm.com> wrote:
> > >
> > > Currently the architecture is hard coded as ARCH_V8, but with the
> > > introduction of ETE we want to pick ARCH_AA64. And this change is also
> > > applicable to ETM v4.4 onwards as well.
> > >
> > > Signed-off-by: James Clark <james.clark@arm.com>
> > > ---
> > >  tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 14 +++++++++++++-
> > >  1 file changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > > index 30889a9d0165..5972a8afcc6b 100644
> > > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> > > @@ -126,6 +126,18 @@ static int cs_etm_decoder__gen_etmv3_config(struct cs_etm_trace_params *params,
> > >         return 0;
> > >  }
> > >
> > > +#define TRCIDR1_TRCARCHMIN_SHIFT 4
> > > +#define TRCIDR1_TRCARCHMIN_MASK  GENMASK(7, 4)
> > > +#define TRCIDR1_TRCARCHMIN(x)    (((x) & TRCIDR1_TRCARCHMIN_MASK) >> TRCIDR1_TRCARCHMIN_SHIFT)
> > > +static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1)
> > > +{
> > > +       /*
> > > +        * If the ETM trace minor version is 4 or more then we can assume
> > > +        * the architecture is ARCH_AA64 rather than just V8
> > > +        */
> > > +       return TRCIDR1_TRCARCHMIN(reg_idr1) >= 4 ? ARCH_AA64 : ARCH_V8;
> > > +}
> >
> > This is true for ETM4.x & ETE 1.x (arch 5.x) but not ETM 3.x
> > Probably need to beef up this comment or the function name to emphasise this.
>
> Yeah, I think it's good to change the function name.  Eventually, this
> function should only be used for ETM4.x and ETE.
>
> Another minor comment is: can we refine the arch version number, e.g.
> change the OpenCSD's macro "ARCH_AA64" to "ARCH_V8R4", (or
> "ARCH_V8R3_AA64"), this can give more clear clue what's the ETM version.
>

The purpose of these macros is to inform the decoder of the
architecture of the PE - not the version of the ETM.

These OpenCSD macros are defined by the library headers
(ocsd_if_types.h) and not the perf headers.
These have been published as the API / ABI for OpenCSD and as such
changing them affects all OpenCSD clients, not just perf.

This PE architecture version is used along with the core profile to
determine which instructions are valid waypoint instructions to
associate with atom elements when walking the program image during
trace decode.

From v8.3  onwards we moved away from filtering on specific
architecture versions. This was due to two factors:-
1. The architectural rules now allow architectural features for one
increment e.g. Arch 8.4, to be backported into  the previous increment
- e,g, 8.3, which made this filtering more difficult to track.
2. After discussion with the PE architects it was clear that
instructions in a later architect version would not re-use older
opcodes from a previous one and  be nop / invalid in the earlier
architectures. (certainly in the scope of AA64). Therefore
the policy in the decoder is to check for all the instructions we know
about for the latest version of architecture, even if we could be
decoding an earlier architecture version. This means we may check for
a few more opcodes than necessary for earlier version of the
architecture, but the overall decode is more robust and easier to
maintain.

Therefore for any AA64 core beyond v8.3 - it is safe to use the
ARCH_AA64 PE architecture version and the decoder will handle it.

Regards

Mike

> And a nitpick: how about to change OpenCSD macro "ARCH_V8r3" to
> "ARCH_V8R3" and assign it for ETMv4.3 IPs.
>
> Thanks,
> Leo



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH 2/6] perf cs-etm: Initialise architecture based on TRCIDR1

[Leo Yan]

Hi Mike,

On Mon, Aug 02, 2021 at 03:04:14PM +0100, Mike Leach wrote:

[...]

> > > > +#define TRCIDR1_TRCARCHMIN_SHIFT 4
> > > > +#define TRCIDR1_TRCARCHMIN_MASK  GENMASK(7, 4)
> > > > +#define TRCIDR1_TRCARCHMIN(x)    (((x) & TRCIDR1_TRCARCHMIN_MASK) >> TRCIDR1_TRCARCHMIN_SHIFT)
> > > > +static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1)
> > > > +{
> > > > +       /*
> > > > +        * If the ETM trace minor version is 4 or more then we can assume
> > > > +        * the architecture is ARCH_AA64 rather than just V8
> > > > +        */
> > > > +       return TRCIDR1_TRCARCHMIN(reg_idr1) >= 4 ? ARCH_AA64 : ARCH_V8;
> > > > +}
> > >
> > > This is true for ETM4.x & ETE 1.x (arch 5.x) but not ETM 3.x
> > > Probably need to beef up this comment or the function name to emphasise this.
> >
> > Yeah, I think it's good to change the function name.  Eventually, this
> > function should only be used for ETM4.x and ETE.
> >
> > Another minor comment is: can we refine the arch version number, e.g.
> > change the OpenCSD's macro "ARCH_AA64" to "ARCH_V8R4", (or
> > "ARCH_V8R3_AA64"), this can give more clear clue what's the ETM version.
> >
> 
> The purpose of these macros is to inform the decoder of the
> architecture of the PE - not the version of the ETM.
> 
> These OpenCSD macros are defined by the library headers
> (ocsd_if_types.h) and not the perf headers.
> These have been published as the API / ABI for OpenCSD and as such
> changing them affects all OpenCSD clients, not just perf.

I understand these macros are defined in OpenCSD lib as APIs, since I
saw these macros have not been widely used in perf tool (e.g.
ARCH_AA64), so this is why I think it's good to take chance to refine
the naming conventions.

> This PE architecture version is used along with the core profile to
> determine which instructions are valid waypoint instructions to
> associate with atom elements when walking the program image during
> trace decode.
> 
> From v8.3  onwards we moved away from filtering on specific
> architecture versions. This was due to two factors:-
> 1. The architectural rules now allow architectural features for one
> increment e.g. Arch 8.4, to be backported into  the previous increment
> - e,g, 8.3, which made this filtering more difficult to track.
> 2. After discussion with the PE architects it was clear that
> instructions in a later architect version would not re-use older
> opcodes from a previous one and  be nop / invalid in the earlier
> architectures. (certainly in the scope of AA64). Therefore
> the policy in the decoder is to check for all the instructions we know
> about for the latest version of architecture, even if we could be
> decoding an earlier architecture version. This means we may check for
> a few more opcodes than necessary for earlier version of the
> architecture, but the overall decode is more robust and easier to
> maintain.
> 
> Therefore for any AA64 core beyond v8.3 - it is safe to use the
> ARCH_AA64 PE architecture version and the decoder will handle it.

I have no objection for current approach; but two things can cause
confusions and it might be difficult for maintenance:

- The first thing is now we base on the bit fields TRCIDR1::TRCARCHMIN
  to decide the PE architecture version.  In the ETMv4 spec,
  TRCIDR1::TRCARCHMIN is defined as the trace unit minor version,
  so essentially it's a minor version number for tracer (ETM) but not
  the PE architecture number.  But now we are using it to decide the
  PE architecture number (8.3, 8.4, etc...).

- The second thing is the macros' naming convention.
  E.g. "AA64" gives me an impression it is a general naming "Arm Arch 64"
  for all Arm 64-bit CPUs, it's something like an abbreviation for
  "aarch64"; so seems to me it doesn't show any meaningful info for PE's
  architecture version number.  This is why I proposed to use more
  explict macro definition for architectures (e.g. ARCH_V8R3, ARCH_V8R4,
  ARCH_V9R0, etc).

If we really want to use ARCH_AA64, it's better to give some comments in
the code.

Thanks a lot for shared the background info.

Leo

Re: [PATCH 2/6] perf cs-etm: Initialise architecture based on TRCIDR1

[Mike Leach]

Hi Leo,

On Mon, 2 Aug 2021 at 16:04, Leo Yan <leo.yan@linaro.org> wrote:
>
> Hi Mike,
>
> On Mon, Aug 02, 2021 at 03:04:14PM +0100, Mike Leach wrote:
>
> [...]
>
> > > > > +#define TRCIDR1_TRCARCHMIN_SHIFT 4
> > > > > +#define TRCIDR1_TRCARCHMIN_MASK  GENMASK(7, 4)
> > > > > +#define TRCIDR1_TRCARCHMIN(x)    (((x) & TRCIDR1_TRCARCHMIN_MASK) >> TRCIDR1_TRCARCHMIN_SHIFT)
> > > > > +static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1)
> > > > > +{
> > > > > +       /*
> > > > > +        * If the ETM trace minor version is 4 or more then we can assume
> > > > > +        * the architecture is ARCH_AA64 rather than just V8
> > > > > +        */
> > > > > +       return TRCIDR1_TRCARCHMIN(reg_idr1) >= 4 ? ARCH_AA64 : ARCH_V8;
> > > > > +}
> > > >
> > > > This is true for ETM4.x & ETE 1.x (arch 5.x) but not ETM 3.x
> > > > Probably need to beef up this comment or the function name to emphasise this.
> > >
> > > Yeah, I think it's good to change the function name.  Eventually, this
> > > function should only be used for ETM4.x and ETE.
> > >
> > > Another minor comment is: can we refine the arch version number, e.g.
> > > change the OpenCSD's macro "ARCH_AA64" to "ARCH_V8R4", (or
> > > "ARCH_V8R3_AA64"), this can give more clear clue what's the ETM version.
> > >
> >
> > The purpose of these macros is to inform the decoder of the
> > architecture of the PE - not the version of the ETM.
> >
> > These OpenCSD macros are defined by the library headers
> > (ocsd_if_types.h) and not the perf headers.
> > These have been published as the API / ABI for OpenCSD and as such
> > changing them affects all OpenCSD clients, not just perf.
>
> I understand these macros are defined in OpenCSD lib as APIs, since I
> saw these macros have not been widely used in perf tool (e.g.
> ARCH_AA64), so this is why I think it's good to take chance to refine
> the naming conventions.
>

The macros are used in other tools - so changing now affects those
too. Not something I am prepared to do without good reason.

> > This PE architecture version is used along with the core profile to
> > determine which instructions are valid waypoint instructions to
> > associate with atom elements when walking the program image during
> > trace decode.
> >
> > From v8.3  onwards we moved away from filtering on specific
> > architecture versions. This was due to two factors:-
> > 1. The architectural rules now allow architectural features for one
> > increment e.g. Arch 8.4, to be backported into  the previous increment
> > - e,g, 8.3, which made this filtering more difficult to track.
> > 2. After discussion with the PE architects it was clear that
> > instructions in a later architect version would not re-use older
> > opcodes from a previous one and  be nop / invalid in the earlier
> > architectures. (certainly in the scope of AA64). Therefore
> > the policy in the decoder is to check for all the instructions we know
> > about for the latest version of architecture, even if we could be
> > decoding an earlier architecture version. This means we may check for
> > a few more opcodes than necessary for earlier version of the
> > architecture, but the overall decode is more robust and easier to
> > maintain.
> >
> > Therefore for any AA64 core beyond v8.3 - it is safe to use the
> > ARCH_AA64 PE architecture version and the decoder will handle it.
>
> I have no objection for current approach; but two things can cause
> confusions and it might be difficult for maintenance:
>
> - The first thing is now we base on the bit fields TRCIDR1::TRCARCHMIN
>   to decide the PE architecture version.  In the ETMv4 spec,
>   TRCIDR1::TRCARCHMIN is defined as the trace unit minor version,
>   so essentially it's a minor version number for tracer (ETM) but not
>   the PE architecture number.  But now we are using it to decide the
>   PE architecture number (8.3, 8.4, etc...).
>

This is a slight weakness in the implementation of perf. Ideally one
does need to establish the architecture version of the PE - but perf
/cs-etm is using an assumption regarding the profile and version of
the core, according to the ETM / ETE versiom.
That said - the ETM / ETE version numbers do have a strong
relationship with PE architecture version numbers, so this assumption
holds for the current supported devices.

> - The second thing is the macros' naming convention.
>   E.g. "AA64" gives me an impression it is a general naming "Arm Arch 64"
>   for all Arm 64-bit CPUs, it's something like an abbreviation for
>   "aarch64"; so seems to me it doesn't show any meaningful info for PE's
>   architecture version number.  This is why I proposed to use more
>   explict macro definition for architectures (e.g. ARCH_V8R3, ARCH_V8R4,
>   ARCH_V9R0, etc).
>

For modern cores it is sufficient for the decoder to know the profile
and that it is aarch 64 - so yes the macro is simply saying this a
general AA64 core.
The macros for earlier versions are a little more specific as certain
filtering is used according to the version of the PE.

ARCH_V8R4,  ARCH_V9R0 etc would have no significance to the decoder
and would not be useful. If we get to the stage where we need more
specific PE architecture versions - then these can be added as
required.
Using the ARCH_AA64 macro means that we do not have to update the API
for every version update of the architecture, and there are no changes
required to the perf / cs-etm handling.

> If we really want to use ARCH_AA64, it's better to give some comments in
> the code.
>

There are comments in the OpenCSD headers, though additional ones in
the perf  / cs-etm handling soruce code could be added.

Regards

Mike


> Thanks a lot for shared the background info.
>
> Leo



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

[PATCH 3/6] perf cs-etm: Save TRCDEVARCH register

[James Clark]

Now that the metadata has a length field we can add extra registers
without breaking any previous versions of perf.

Save the TRCDEVARCH register so that it can be used to configure the ETE
decoder in the next commit. If the sysfs file doesn't exist then 0 will
be saved which is an impossible register value and can also be used to
signify that the file couldn't be read.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/arch/arm/util/cs-etm.c | 13 ++++++++++++-
 tools/perf/util/cs-etm.c          |  1 +
 tools/perf/util/cs-etm.h          |  5 +++--
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index 85168d87b2d7..65a863bdf5cc 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -53,6 +53,7 @@ static const char *metadata_etmv4_ro[CS_ETMV4_PRIV_MAX] = {
 	[CS_ETMV4_TRCIDR2]		= "trcidr/trcidr2",
 	[CS_ETMV4_TRCIDR8]		= "trcidr/trcidr8",
 	[CS_ETMV4_TRCAUTHSTATUS]	= "mgmt/trcauthstatus",
+	[CS_ETE_TRCDEVARCH]		= "mgmt/trcdevarch"
 };
 
 static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu);
@@ -73,7 +74,7 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr,
 	if (!cs_etm_is_etmv4(itr, cpu))
 		goto out;
 
-	/* Get a handle on TRCIRD2 */
+	/* Get a handle on TRCIDR2 */
 	snprintf(path, PATH_MAX, "cpu%d/%s",
 		 cpu, metadata_etmv4_ro[CS_ETMV4_TRCIDR2]);
 	err = perf_pmu__scan_file(cs_etm_pmu, path, "%x", &val);
@@ -643,6 +644,16 @@ static void cs_etm_get_metadata(int cpu, u32 *offset,
 			cs_etm_get_ro(cs_etm_pmu, cpu,
 				      metadata_etmv4_ro
 				      [CS_ETMV4_TRCAUTHSTATUS]);
+		/*
+		 * ETE uses the same registers as ETMv4 plus TRCDEVARCH. It's also backwards
+		 * compatible, so don't change the magic number otherwise that will reduce the
+		 * number of versions of perf that can open it. Just append TRCDEVARCH to the end of
+		 * the register block and allow newer versions of perf to make use. cs_etm_get_ro()
+		 * returns 0 if it couldn't be read.
+		 */
+		info->priv[*offset + CS_ETE_TRCDEVARCH] =
+			cs_etm_get_ro(cs_etm_pmu, cpu,
+				      metadata_etmv4_ro[CS_ETE_TRCDEVARCH]);
 
 		/* How much space was used */
 		increment = CS_ETMV4_PRIV_MAX;
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 62769a84a53f..68978f6707a8 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -2508,6 +2508,7 @@ static const char * const cs_etmv4_priv_fmts[] = {
 	[CS_ETMV4_TRCIDR2]	= "	TRCIDR2			       %llx\n",
 	[CS_ETMV4_TRCIDR8]	= "	TRCIDR8			       %llx\n",
 	[CS_ETMV4_TRCAUTHSTATUS] = "	TRCAUTHSTATUS		       %llx\n",
+	[CS_ETE_TRCDEVARCH]	= "	TRCDEVARCH                     %llx\n"
 };
 
 static const char * const param_unk_fmt =
diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h
index d65c7b19407d..52d82dce9d59 100644
--- a/tools/perf/util/cs-etm.h
+++ b/tools/perf/util/cs-etm.h
@@ -59,7 +59,7 @@ enum {
 /* define fixed version 0 length - allow new format reader to read old files. */
 #define CS_ETM_NR_TRC_PARAMS_V0 (CS_ETM_ETMIDR - CS_ETM_ETMCR + 1)
 
-/* ETMv4 metadata */
+/* ETMv4 + ETE metadata */
 enum {
 	/* Dynamic, configurable parameters */
 	CS_ETMV4_TRCCONFIGR = CS_ETM_COMMON_BLK_MAX_V1,
@@ -70,7 +70,8 @@ enum {
 	CS_ETMV4_TRCIDR2,
 	CS_ETMV4_TRCIDR8,
 	CS_ETMV4_TRCAUTHSTATUS,
-	CS_ETMV4_PRIV_MAX,
+	CS_ETE_TRCDEVARCH,
+	CS_ETMV4_PRIV_MAX
 };
 
 /* define fixed version 0 length - allow new format reader to read old files. */
-- 
2.28.0

Re: [PATCH 3/6] perf cs-etm: Save TRCDEVARCH register

[Mike Leach]

HI James,

On Wed, 21 Jul 2021 at 10:07, James Clark <james.clark@arm.com> wrote:
>
> Now that the metadata has a length field we can add extra registers
> without breaking any previous versions of perf.
>
> Save the TRCDEVARCH register so that it can be used to configure the ETE
> decoder in the next commit. If the sysfs file doesn't exist then 0 will
> be saved which is an impossible register value and can also be used to
> signify that the file couldn't be read.
>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/arch/arm/util/cs-etm.c | 13 ++++++++++++-
>  tools/perf/util/cs-etm.c          |  1 +
>  tools/perf/util/cs-etm.h          |  5 +++--
>  3 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
> index 85168d87b2d7..65a863bdf5cc 100644
> --- a/tools/perf/arch/arm/util/cs-etm.c
> +++ b/tools/perf/arch/arm/util/cs-etm.c
> @@ -53,6 +53,7 @@ static const char *metadata_etmv4_ro[CS_ETMV4_PRIV_MAX] = {
>         [CS_ETMV4_TRCIDR2]              = "trcidr/trcidr2",
>         [CS_ETMV4_TRCIDR8]              = "trcidr/trcidr8",
>         [CS_ETMV4_TRCAUTHSTATUS]        = "mgmt/trcauthstatus",
> +       [CS_ETE_TRCDEVARCH]             = "mgmt/trcdevarch"
>  };
>
>  static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu);
> @@ -73,7 +74,7 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr,
>         if (!cs_etm_is_etmv4(itr, cpu))
>                 goto out;
>
> -       /* Get a handle on TRCIRD2 */
> +       /* Get a handle on TRCIDR2 */
>         snprintf(path, PATH_MAX, "cpu%d/%s",
>                  cpu, metadata_etmv4_ro[CS_ETMV4_TRCIDR2]);
>         err = perf_pmu__scan_file(cs_etm_pmu, path, "%x", &val);
> @@ -643,6 +644,16 @@ static void cs_etm_get_metadata(int cpu, u32 *offset,
>                         cs_etm_get_ro(cs_etm_pmu, cpu,
>                                       metadata_etmv4_ro
>                                       [CS_ETMV4_TRCAUTHSTATUS]);
> +               /*
> +                * ETE uses the same registers as ETMv4 plus TRCDEVARCH. It's also backwards
> +                * compatible, so don't change the magic number otherwise that will reduce the
> +                * number of versions of perf that can open it. Just append TRCDEVARCH to the end of
> +                * the register block and allow newer versions of perf to make use. cs_etm_get_ro()
> +                * returns 0 if it couldn't be read.
> +                */

ETE is a superset of ETMv4, but an old perf that only knows ETMv4
cannot be guaranteed to decode all ETE due to new packet types.
Therefore do we want to allow old perfs to decode only some ETE,
possibly with errors?

I think it would be better to add in a new magic number for the new
decoder rather than have some grey overlap area were an "older" perf
might work intermittently dependent on the packets generated in a
particular trace run.

Regards

Mike

> +               info->priv[*offset + CS_ETE_TRCDEVARCH] =
> +                       cs_etm_get_ro(cs_etm_pmu, cpu,
> +                                     metadata_etmv4_ro[CS_ETE_TRCDEVARCH]);
>
>                 /* How much space was used */
>                 increment = CS_ETMV4_PRIV_MAX;
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 62769a84a53f..68978f6707a8 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -2508,6 +2508,7 @@ static const char * const cs_etmv4_priv_fmts[] = {
>         [CS_ETMV4_TRCIDR2]      = "     TRCIDR2                        %llx\n",
>         [CS_ETMV4_TRCIDR8]      = "     TRCIDR8                        %llx\n",
>         [CS_ETMV4_TRCAUTHSTATUS] = "    TRCAUTHSTATUS                  %llx\n",
> +       [CS_ETE_TRCDEVARCH]     = "     TRCDEVARCH                     %llx\n"
>  };
>
>  static const char * const param_unk_fmt =
> diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h
> index d65c7b19407d..52d82dce9d59 100644
> --- a/tools/perf/util/cs-etm.h
> +++ b/tools/perf/util/cs-etm.h
> @@ -59,7 +59,7 @@ enum {
>  /* define fixed version 0 length - allow new format reader to read old files. */
>  #define CS_ETM_NR_TRC_PARAMS_V0 (CS_ETM_ETMIDR - CS_ETM_ETMCR + 1)
>
> -/* ETMv4 metadata */
> +/* ETMv4 + ETE metadata */
>  enum {
>         /* Dynamic, configurable parameters */
>         CS_ETMV4_TRCCONFIGR = CS_ETM_COMMON_BLK_MAX_V1,
> @@ -70,7 +70,8 @@ enum {
>         CS_ETMV4_TRCIDR2,
>         CS_ETMV4_TRCIDR8,
>         CS_ETMV4_TRCAUTHSTATUS,
> -       CS_ETMV4_PRIV_MAX,
> +       CS_ETE_TRCDEVARCH,
> +       CS_ETMV4_PRIV_MAX
>  };
>
>  /* define fixed version 0 length - allow new format reader to read old files. */
> --
> 2.28.0
>


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH 3/6] perf cs-etm: Save TRCDEVARCH register

[James Clark]

On 21/07/2021 10:48, Mike Leach wrote:
> HI James,
> 
> On Wed, 21 Jul 2021 at 10:07, James Clark <james.clark@arm.com> wrote:
>>
>> Now that the metadata has a length field we can add extra registers
>> without breaking any previous versions of perf.
>>
>> Save the TRCDEVARCH register so that it can be used to configure the ETE
>> decoder in the next commit. If the sysfs file doesn't exist then 0 will
>> be saved which is an impossible register value and can also be used to
>> signify that the file couldn't be read.
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>>  tools/perf/arch/arm/util/cs-etm.c | 13 ++++++++++++-
>>  tools/perf/util/cs-etm.c          |  1 +
>>  tools/perf/util/cs-etm.h          |  5 +++--
>>  3 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
>> index 85168d87b2d7..65a863bdf5cc 100644
>> --- a/tools/perf/arch/arm/util/cs-etm.c
>> +++ b/tools/perf/arch/arm/util/cs-etm.c
>> @@ -53,6 +53,7 @@ static const char *metadata_etmv4_ro[CS_ETMV4_PRIV_MAX] = {
>>         [CS_ETMV4_TRCIDR2]              = "trcidr/trcidr2",
>>         [CS_ETMV4_TRCIDR8]              = "trcidr/trcidr8",
>>         [CS_ETMV4_TRCAUTHSTATUS]        = "mgmt/trcauthstatus",
>> +       [CS_ETE_TRCDEVARCH]             = "mgmt/trcdevarch"
>>  };
>>
>>  static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu);
>> @@ -73,7 +74,7 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr,
>>         if (!cs_etm_is_etmv4(itr, cpu))
>>                 goto out;
>>
>> -       /* Get a handle on TRCIRD2 */
>> +       /* Get a handle on TRCIDR2 */
>>         snprintf(path, PATH_MAX, "cpu%d/%s",
>>                  cpu, metadata_etmv4_ro[CS_ETMV4_TRCIDR2]);
>>         err = perf_pmu__scan_file(cs_etm_pmu, path, "%x", &val);
>> @@ -643,6 +644,16 @@ static void cs_etm_get_metadata(int cpu, u32 *offset,
>>                         cs_etm_get_ro(cs_etm_pmu, cpu,
>>                                       metadata_etmv4_ro
>>                                       [CS_ETMV4_TRCAUTHSTATUS]);
>> +               /*
>> +                * ETE uses the same registers as ETMv4 plus TRCDEVARCH. It's also backwards
>> +                * compatible, so don't change the magic number otherwise that will reduce the
>> +                * number of versions of perf that can open it. Just append TRCDEVARCH to the end of
>> +                * the register block and allow newer versions of perf to make use. cs_etm_get_ro()
>> +                * returns 0 if it couldn't be read.
>> +                */
> 
> ETE is a superset of ETMv4, but an old perf that only knows ETMv4
> cannot be guaranteed to decode all ETE due to new packet types.
> Therefore do we want to allow old perfs to decode only some ETE,
> possibly with errors?
> 
> I think it would be better to add in a new magic number for the new
> decoder rather than have some grey overlap area were an "older" perf
> might work intermittently dependent on the packets generated in a
> particular trace run.

I did think about doing it this way and I'm also ok with adding a new
magic number. I will wait till Al's back to get his opinion before
making the change.

Thanks
James

> 
> Regards
> 
> Mike
> 
>> +               info->priv[*offset + CS_ETE_TRCDEVARCH] =
>> +                       cs_etm_get_ro(cs_etm_pmu, cpu,
>> +                                     metadata_etmv4_ro[CS_ETE_TRCDEVARCH]);
>>
>>                 /* How much space was used */
>>                 increment = CS_ETMV4_PRIV_MAX;
>> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
>> index 62769a84a53f..68978f6707a8 100644
>> --- a/tools/perf/util/cs-etm.c
>> +++ b/tools/perf/util/cs-etm.c
>> @@ -2508,6 +2508,7 @@ static const char * const cs_etmv4_priv_fmts[] = {
>>         [CS_ETMV4_TRCIDR2]      = "     TRCIDR2                        %llx\n",
>>         [CS_ETMV4_TRCIDR8]      = "     TRCIDR8                        %llx\n",
>>         [CS_ETMV4_TRCAUTHSTATUS] = "    TRCAUTHSTATUS                  %llx\n",
>> +       [CS_ETE_TRCDEVARCH]     = "     TRCDEVARCH                     %llx\n"
>>  };
>>
>>  static const char * const param_unk_fmt =
>> diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h
>> index d65c7b19407d..52d82dce9d59 100644
>> --- a/tools/perf/util/cs-etm.h
>> +++ b/tools/perf/util/cs-etm.h
>> @@ -59,7 +59,7 @@ enum {
>>  /* define fixed version 0 length - allow new format reader to read old files. */
>>  #define CS_ETM_NR_TRC_PARAMS_V0 (CS_ETM_ETMIDR - CS_ETM_ETMCR + 1)
>>
>> -/* ETMv4 metadata */
>> +/* ETMv4 + ETE metadata */
>>  enum {
>>         /* Dynamic, configurable parameters */
>>         CS_ETMV4_TRCCONFIGR = CS_ETM_COMMON_BLK_MAX_V1,
>> @@ -70,7 +70,8 @@ enum {
>>         CS_ETMV4_TRCIDR2,
>>         CS_ETMV4_TRCIDR8,
>>         CS_ETMV4_TRCAUTHSTATUS,
>> -       CS_ETMV4_PRIV_MAX,
>> +       CS_ETE_TRCDEVARCH,
>> +       CS_ETMV4_PRIV_MAX
>>  };
>>
>>  /* define fixed version 0 length - allow new format reader to read old files. */
>> --
>> 2.28.0
>>
> 
> 

Re: [PATCH 3/6] perf cs-etm: Save TRCDEVARCH register

[Leo Yan]

On Wed, Jul 21, 2021 at 10:48:25AM +0100, Mike Leach wrote:
> HI James,
> 
> On Wed, 21 Jul 2021 at 10:07, James Clark <james.clark@arm.com> wrote:
> >
> > Now that the metadata has a length field we can add extra registers
> > without breaking any previous versions of perf.
> >
> > Save the TRCDEVARCH register so that it can be used to configure the ETE
> > decoder in the next commit. If the sysfs file doesn't exist then 0 will
> > be saved which is an impossible register value and can also be used to
> > signify that the file couldn't be read.
> >
> > Signed-off-by: James Clark <james.clark@arm.com>
> > ---
> >  tools/perf/arch/arm/util/cs-etm.c | 13 ++++++++++++-
> >  tools/perf/util/cs-etm.c          |  1 +
> >  tools/perf/util/cs-etm.h          |  5 +++--
> >  3 files changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
> > index 85168d87b2d7..65a863bdf5cc 100644
> > --- a/tools/perf/arch/arm/util/cs-etm.c
> > +++ b/tools/perf/arch/arm/util/cs-etm.c
> > @@ -53,6 +53,7 @@ static const char *metadata_etmv4_ro[CS_ETMV4_PRIV_MAX] = {
> >         [CS_ETMV4_TRCIDR2]              = "trcidr/trcidr2",
> >         [CS_ETMV4_TRCIDR8]              = "trcidr/trcidr8",
> >         [CS_ETMV4_TRCAUTHSTATUS]        = "mgmt/trcauthstatus",
> > +       [CS_ETE_TRCDEVARCH]             = "mgmt/trcdevarch"

ETMv4 supports TRCDEVARCH, so I think it's good to use the naming
"CS_ETMV4_TRCDEVARCH"?

> >  };
> >
> >  static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu);
> > @@ -73,7 +74,7 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr,
> >         if (!cs_etm_is_etmv4(itr, cpu))
> >                 goto out;
> >
> > -       /* Get a handle on TRCIRD2 */
> > +       /* Get a handle on TRCIDR2 */

This is typo fixing; it's irrelevant to the topic in this patch, it's
good to use a separate patch for the typo fixing.

> >         snprintf(path, PATH_MAX, "cpu%d/%s",
> >                  cpu, metadata_etmv4_ro[CS_ETMV4_TRCIDR2]);
> >         err = perf_pmu__scan_file(cs_etm_pmu, path, "%x", &val);
> > @@ -643,6 +644,16 @@ static void cs_etm_get_metadata(int cpu, u32 *offset,
> >                         cs_etm_get_ro(cs_etm_pmu, cpu,
> >                                       metadata_etmv4_ro
> >                                       [CS_ETMV4_TRCAUTHSTATUS]);
> > +               /*
> > +                * ETE uses the same registers as ETMv4 plus TRCDEVARCH. It's also backwards
> > +                * compatible, so don't change the magic number otherwise that will reduce the
> > +                * number of versions of perf that can open it. Just append TRCDEVARCH to the end of
> > +                * the register block and allow newer versions of perf to make use. cs_etm_get_ro()
> > +                * returns 0 if it couldn't be read.
> > +                */
> 
> ETE is a superset of ETMv4, but an old perf that only knows ETMv4
> cannot be guaranteed to decode all ETE due to new packet types.
> Therefore do we want to allow old perfs to decode only some ETE,
> possibly with errors?
> 
> I think it would be better to add in a new magic number for the new
> decoder rather than have some grey overlap area were an "older" perf
> might work intermittently dependent on the packets generated in a
> particular trace run.

I checked ETMv4.3 and ETMv4.4 spec (ARM IHI0064E for ETMv4.3 and ARM
IHI0064F for ETMv4.4), both clarify ETMv4 has the register TRCDEVARCH;
thus TRCDEVARCH is not a new register introduced by ETE.

For this case, it's good to directly add a new field in the metadata
array for recording register TRCDEVARCH.

If there have any new registers are introduced by ETE, then Mike's
suggestion for using new magic number is the right thing to do.

> > +               info->priv[*offset + CS_ETE_TRCDEVARCH] =
> > +                       cs_etm_get_ro(cs_etm_pmu, cpu,
> > +                                     metadata_etmv4_ro[CS_ETE_TRCDEVARCH]);
> >
> >                 /* How much space was used */
> >                 increment = CS_ETMV4_PRIV_MAX;
> > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> > index 62769a84a53f..68978f6707a8 100644
> > --- a/tools/perf/util/cs-etm.c
> > +++ b/tools/perf/util/cs-etm.c
> > @@ -2508,6 +2508,7 @@ static const char * const cs_etmv4_priv_fmts[] = {
> >         [CS_ETMV4_TRCIDR2]      = "     TRCIDR2                        %llx\n",
> >         [CS_ETMV4_TRCIDR8]      = "     TRCIDR8                        %llx\n",
> >         [CS_ETMV4_TRCAUTHSTATUS] = "    TRCAUTHSTATUS                  %llx\n",
> > +       [CS_ETE_TRCDEVARCH]     = "     TRCDEVARCH                     %llx\n"
> >  };
> >
> >  static const char * const param_unk_fmt =
> > diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h
> > index d65c7b19407d..52d82dce9d59 100644
> > --- a/tools/perf/util/cs-etm.h
> > +++ b/tools/perf/util/cs-etm.h
> > @@ -59,7 +59,7 @@ enum {
> >  /* define fixed version 0 length - allow new format reader to read old files. */
> >  #define CS_ETM_NR_TRC_PARAMS_V0 (CS_ETM_ETMIDR - CS_ETM_ETMCR + 1)
> >
> > -/* ETMv4 metadata */
> > +/* ETMv4 + ETE metadata */
> >  enum {
> >         /* Dynamic, configurable parameters */
> >         CS_ETMV4_TRCCONFIGR = CS_ETM_COMMON_BLK_MAX_V1,
> > @@ -70,7 +70,8 @@ enum {
> >         CS_ETMV4_TRCIDR2,
> >         CS_ETMV4_TRCIDR8,
> >         CS_ETMV4_TRCAUTHSTATUS,
> > -       CS_ETMV4_PRIV_MAX,
> > +       CS_ETE_TRCDEVARCH,
> > +       CS_ETMV4_PRIV_MAX

Spurious change for "CS_ETMV4_PRIV_MAX"?

Thanks,
Leo

Re: [PATCH 3/6] perf cs-etm: Save TRCDEVARCH register

[James Clark]

On 31/07/2021 07:37, Leo Yan wrote:
> I checked ETMv4.3 and ETMv4.4 spec (ARM IHI0064E for ETMv4.3 and ARM
> IHI0064F for ETMv4.4), both clarify ETMv4 has the register TRCDEVARCH;
> thus TRCDEVARCH is not a new register introduced by ETE.
> 
> For this case, it's good to directly add a new field in the metadata
> array for recording register TRCDEVARCH.

This might be true, but the OpenCSD library doesn't take TRCDEVARCH as a config
parameter for ETMv4 so it couldn't be used. This is the struct:

        typedef struct _ocsd_etmv4_cfg 
        {
        uint32_t                reg_idr0;    /**< ID0 register */
        uint32_t                reg_idr1;    /**< ID1 register */
        uint32_t                reg_idr2;    /**< ID2 register */
        uint32_t                reg_idr8;
        uint32_t                reg_idr9;   
        uint32_t                reg_idr10;
        uint32_t                reg_idr11;
        uint32_t                reg_idr12;
        uint32_t                reg_idr13;
        uint32_t                reg_configr;  /**< Config Register */
        uint32_t                reg_traceidr;  /**< Trace Stream ID register */
        ocsd_arch_version_t    arch_ver;   /**< Architecture version */
        ocsd_core_profile_t    core_prof;  /**< Core Profile */
        } ocsd_etmv4_cfg;

And this is ETE where TRCDEVARCH is used:

        typedef struct _ocsd_ete_cfg
        {
        uint32_t                reg_idr0;       /**< ID0 register */
        uint32_t                reg_idr1;       /**< ID1 register */
        uint32_t                reg_idr2;       /**< ID2 register */
        uint32_t                reg_idr8;       /**< ID8 - maxspec */
        uint32_t                reg_devarch;    /**< DevArch register */
        uint32_t                reg_configr;    /**< Config Register */
        uint32_t                reg_traceidr;   /**< Trace Stream ID register */
        ocsd_arch_version_t    arch_ver;        /**< Architecture version */
        ocsd_core_profile_t    core_prof;       /**< Core Profile */
        } ocsd_ete_cfg;

Thanks
James

Re: [PATCH 3/6] perf cs-etm: Save TRCDEVARCH register

[James Clark]

On 31/07/2021 07:37, Leo Yan wrote:
>>> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
>>> index 85168d87b2d7..65a863bdf5cc 100644
>>> --- a/tools/perf/arch/arm/util/cs-etm.c
>>> +++ b/tools/perf/arch/arm/util/cs-etm.c
>>> @@ -53,6 +53,7 @@ static const char *metadata_etmv4_ro[CS_ETMV4_PRIV_MAX] = {
>>>         [CS_ETMV4_TRCIDR2]              = "trcidr/trcidr2",
>>>         [CS_ETMV4_TRCIDR8]              = "trcidr/trcidr8",
>>>         [CS_ETMV4_TRCAUTHSTATUS]        = "mgmt/trcauthstatus",
>>> +       [CS_ETE_TRCDEVARCH]             = "mgmt/trcdevarch"
> ETMv4 supports TRCDEVARCH, so I think it's good to use the naming
> "CS_ETMV4_TRCDEVARCH"?
> 

Based on the other discussions do you still think I should do this?

As part of the new magic number I moved it into a new enum so it
might be clearer now?

James

Re: [PATCH 3/6] perf cs-etm: Save TRCDEVARCH register

[Leo Yan]

On Tue, Aug 03, 2021 at 01:34:36PM +0100, James Clark wrote:
> 
> On 31/07/2021 07:37, Leo Yan wrote:
> >>> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
> >>> index 85168d87b2d7..65a863bdf5cc 100644
> >>> --- a/tools/perf/arch/arm/util/cs-etm.c
> >>> +++ b/tools/perf/arch/arm/util/cs-etm.c
> >>> @@ -53,6 +53,7 @@ static const char *metadata_etmv4_ro[CS_ETMV4_PRIV_MAX] = {
> >>>         [CS_ETMV4_TRCIDR2]              = "trcidr/trcidr2",
> >>>         [CS_ETMV4_TRCIDR8]              = "trcidr/trcidr8",
> >>>         [CS_ETMV4_TRCAUTHSTATUS]        = "mgmt/trcauthstatus",
> >>> +       [CS_ETE_TRCDEVARCH]             = "mgmt/trcdevarch"
> > ETMv4 supports TRCDEVARCH, so I think it's good to use the naming
> > "CS_ETMV4_TRCDEVARCH"?
> > 
> 
> Based on the other discussions do you still think I should do this?

No, don't need to add TRCDEVARCH for ETMv4 anymore.

> As part of the new magic number I moved it into a new enum so it
> might be clearer now?

Regard ETMv4 and ETE use the different decoder types from OpenCSD, you
could add a new magic number and new enum for ETE.  It would be easier
for later extension for ETMv4 and ETE separately if decoder requires
to pass more config info.

Thanks,
Leo

Re: [PATCH 3/6] perf cs-etm: Save TRCDEVARCH register

[James Clark]

On 31/07/2021 07:37, Leo Yan wrote:
>>> diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h
>>> index d65c7b19407d..52d82dce9d59 100644
>>> --- a/tools/perf/util/cs-etm.h
>>> +++ b/tools/perf/util/cs-etm.h
>>> @@ -59,7 +59,7 @@ enum {
>>>  /* define fixed version 0 length - allow new format reader to read old files. */
>>>  #define CS_ETM_NR_TRC_PARAMS_V0 (CS_ETM_ETMIDR - CS_ETM_ETMCR + 1)
>>>
>>> -/* ETMv4 metadata */
>>> +/* ETMv4 + ETE metadata */
>>>  enum {
>>>         /* Dynamic, configurable parameters */
>>>         CS_ETMV4_TRCCONFIGR = CS_ETM_COMMON_BLK_MAX_V1,
>>> @@ -70,7 +70,8 @@ enum {
>>>         CS_ETMV4_TRCIDR2,
>>>         CS_ETMV4_TRCIDR8,
>>>         CS_ETMV4_TRCAUTHSTATUS,
>>> -       CS_ETMV4_PRIV_MAX,
>>> +       CS_ETE_TRCDEVARCH,
>>> +       CS_ETMV4_PRIV_MAX
> Spurious change for "CS_ETMV4_PRIV_MAX"?

This change will be removed in the next version due to moving it into
a new enum. But it wasn't a mistake, CS_ETMV4_PRIV_MAX is used to define the length
of the header so it always needs to be at the end if another item is saved.

James

Re: [PATCH 3/6] perf cs-etm: Save TRCDEVARCH register

[Leo Yan]

On Wed, Jul 21, 2021 at 10:07:02AM +0100, James Clark wrote:
> Now that the metadata has a length field we can add extra registers
> without breaking any previous versions of perf.
> 
> Save the TRCDEVARCH register so that it can be used to configure the ETE
> decoder in the next commit. If the sysfs file doesn't exist then 0 will
> be saved which is an impossible register value and can also be used to
> signify that the file couldn't be read.

After reviewed the whole patch set, come back to highlight one thing:
seems to me ETE is only a feature introduced by new ETMv4 revisions; in
other words, if we support ETMv4.5 or any later revisions, it will
support ETE feature.

Here I think the right thing to do is to support newer revisions for
ETMv4, and then based on the revision it creates a decoder with
supporting ETE feature.  For a more neat solution, if the perf tool
passes the "correct" revision number to the OpenCSD decoder, it should
can decode trace data with ETE packets.  In this way, the ETE decoding
can be transparent for perf cs-etm code.

How about you think for this?  Sorry if I introduce noise due to my
lack knowledge (and platform) for ETE.

Thanks,
Leo

Re: [PATCH 3/6] perf cs-etm: Save TRCDEVARCH register

[Mike Leach]

Hi Leo.

On Sat, 31 Jul 2021 at 08:43, Leo Yan <leo.yan@linaro.org> wrote:
>
> On Wed, Jul 21, 2021 at 10:07:02AM +0100, James Clark wrote:
> > Now that the metadata has a length field we can add extra registers
> > without breaking any previous versions of perf.
> >
> > Save the TRCDEVARCH register so that it can be used to configure the ETE
> > decoder in the next commit. If the sysfs file doesn't exist then 0 will
> > be saved which is an impossible register value and can also be used to
> > signify that the file couldn't be read.
>
> After reviewed the whole patch set, come back to highlight one thing:
> seems to me ETE is only a feature introduced by new ETMv4 revisions; in
> other words, if we support ETMv4.5 or any later revisions, it will
> support ETE feature.
>

The ETE hardware and trace protocol is introduced to support new
architectural features in the PE - principally those associated with
v9 architecture and beyond. It has has additional packet types that
will never appear in any ETM4.x trace sequence.
ETE (and TRBE) are defined as architectural features of the PE - i.e.
FEAT_ETE and FEAT_TRBE read from feature registers on the core.

You are correct in saying that some features in ETM v4.x beyond ETM
4.5 will also appear in ETE, but the reverse is not true - ETE is a
superset of ETMv4.

> Here I think the right thing to do is to support newer revisions for
> ETMv4, and then based on the revision it creates a decoder with
> supporting ETE feature.  For a more neat solution, if the perf tool
> passes the "correct" revision number to the OpenCSD decoder, it should
> can decode trace data with ETE packets.  In this way, the ETE decoding
> can be transparent for perf cs-etm code.
>

The OpenCSD decoder separates the ETMv4 decoder from the ETE decoder -
for the reasons given above.

Additionally the ETE decoder and the ETMv4 decoder required different
sets of ID registers to correctly set up the decoder.  For example,
for ETMv4 the version is extracted form TRCIDR1, for ETE the version
in TRCIDR1 is set 0xFF, and thus needs TRCDEVARCH to extract the
revision. It is likely that later updates to ETE will require an
additional TRCIDR register to be saved.

Choosing the base type of decoder in this way is how the library can
support ETMv3, EMTv4, ETE, STM, PTM etc - and while some of those
protocols use TRCIDR1 and TRCDEVARCH - not all do.

It would in theory be possible to have the OpenCSD library
"autodetect" the type of decoder needed based purely on a set of ID
registers. But this set of ID registers would be far larger than the
ones currently used, and would require modifcation to a lot of the
existing device drivers to ensure they were accessible via sysfs. This
register set includes the ID registers that are currently used to
identify the component on the AMBA bus and match to the correct
driver, plus additional CoreSight management registers. This would
also create a dependency between decoder creation and ID numbers - in
the same way that each new ETM4.x part number has to be added to the
ETM4.x device driver.

Such a system would require a significant update to the OpenCSD
infrastructure, and is not planned at this time.

Regards

Mike


> How about you think for this?  Sorry if I introduce noise due to my
> lack knowledge (and platform) for ETE.
>
> Thanks,
> Leo



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH 3/6] perf cs-etm: Save TRCDEVARCH register

[Leo Yan]

Hi Mike,

On Mon, Aug 02, 2021 at 12:21:31PM +0100, Mike Leach wrote:

[...]

> > Here I think the right thing to do is to support newer revisions for
> > ETMv4, and then based on the revision it creates a decoder with
> > supporting ETE feature.  For a more neat solution, if the perf tool
> > passes the "correct" revision number to the OpenCSD decoder, it should
> > can decode trace data with ETE packets.  In this way, the ETE decoding
> > can be transparent for perf cs-etm code.
> >
> 
> The OpenCSD decoder separates the ETMv4 decoder from the ETE decoder -
> for the reasons given above.

Thanks for explanation.

> Additionally the ETE decoder and the ETMv4 decoder required different
> sets of ID registers to correctly set up the decoder.  For example,
> for ETMv4 the version is extracted form TRCIDR1, for ETE the version
> in TRCIDR1 is set 0xFF, and thus needs TRCDEVARCH to extract the
> revision. It is likely that later updates to ETE will require an
> additional TRCIDR register to be saved.

Okay, for ETMv4.x and ETE, finally I think we need to rely on TRCDEVARCH to
decide the tracer version based on the architecture number (arch 4 or 5)
and revision number.

> Choosing the base type of decoder in this way is how the library can
> support ETMv3, EMTv4, ETE, STM, PTM etc - and while some of those
> protocols use TRCIDR1 and TRCDEVARCH - not all do.
> 
> It would in theory be possible to have the OpenCSD library
> "autodetect" the type of decoder needed based purely on a set of ID
> registers. But this set of ID registers would be far larger than the
> ones currently used, and would require modifcation to a lot of the
> existing device drivers to ensure they were accessible via sysfs. This
> register set includes the ID registers that are currently used to
> identify the component on the AMBA bus and match to the correct
> driver, plus additional CoreSight management registers. This would
> also create a dependency between decoder creation and ID numbers - in
> the same way that each new ETM4.x part number has to be added to the
> ETM4.x device driver.
> 
> Such a system would require a significant update to the OpenCSD
> infrastructure, and is not planned at this time.

It's fine for me not introducing significant change in OpenCSD.

If so, I understand your suggestion in another email to add a new magic
number and a new protocol (this patch set has added the new protocol
CS_ETM_PROTO_ETE) for creating ETE decoder.

Just confirm one thing which is a bit confused me: for ETMv4.5 or any
newer ETM IPs, should the perf tool keep the existed way to create the
ETMv4 decoder?  Or there have updating is required for decoder to
support the extended packets?

Thanks,
Leo

Re: [PATCH 3/6] perf cs-etm: Save TRCDEVARCH register

[Mike Leach]

Hi Leo,

On Mon, 2 Aug 2021 at 13:05, Leo Yan <leo.yan@linaro.org> wrote:
>
> Hi Mike,
>
> On Mon, Aug 02, 2021 at 12:21:31PM +0100, Mike Leach wrote:
>
> [...]
>
> > > Here I think the right thing to do is to support newer revisions for
> > > ETMv4, and then based on the revision it creates a decoder with
> > > supporting ETE feature.  For a more neat solution, if the perf tool
> > > passes the "correct" revision number to the OpenCSD decoder, it should
> > > can decode trace data with ETE packets.  In this way, the ETE decoding
> > > can be transparent for perf cs-etm code.
> > >
> >
> > The OpenCSD decoder separates the ETMv4 decoder from the ETE decoder -
> > for the reasons given above.
>
> Thanks for explanation.
>
> > Additionally the ETE decoder and the ETMv4 decoder required different
> > sets of ID registers to correctly set up the decoder.  For example,
> > for ETMv4 the version is extracted form TRCIDR1, for ETE the version
> > in TRCIDR1 is set 0xFF, and thus needs TRCDEVARCH to extract the
> > revision. It is likely that later updates to ETE will require an
> > additional TRCIDR register to be saved.
>
> Okay, for ETMv4.x and ETE, finally I think we need to rely on TRCDEVARCH to
> decide the tracer version based on the architecture number (arch 4 or 5)
> and revision number.
>
> > Choosing the base type of decoder in this way is how the library can
> > support ETMv3, EMTv4, ETE, STM, PTM etc - and while some of those
> > protocols use TRCIDR1 and TRCDEVARCH - not all do.
> >
> > It would in theory be possible to have the OpenCSD library
> > "autodetect" the type of decoder needed based purely on a set of ID
> > registers. But this set of ID registers would be far larger than the
> > ones currently used, and would require modifcation to a lot of the
> > existing device drivers to ensure they were accessible via sysfs. This
> > register set includes the ID registers that are currently used to
> > identify the component on the AMBA bus and match to the correct
> > driver, plus additional CoreSight management registers. This would
> > also create a dependency between decoder creation and ID numbers - in
> > the same way that each new ETM4.x part number has to be added to the
> > ETM4.x device driver.
> >
> > Such a system would require a significant update to the OpenCSD
> > infrastructure, and is not planned at this time.
>
> It's fine for me not introducing significant change in OpenCSD.
>
> If so, I understand your suggestion in another email to add a new magic
> number and a new protocol (this patch set has added the new protocol
> CS_ETM_PROTO_ETE) for creating ETE decoder.
>
> Just confirm one thing which is a bit confused me: for ETMv4.5 or any
> newer ETM IPs, should the perf tool keep the existed way to create the
> ETMv4 decoder?  Or there have updating is required for decoder to
> support the extended packets?
>

Where the trace device is identified as an ETMv4.x, then perf will
continue to create an ETMv4.x decoder as it does now. The OpenCSD
ETM4.x decoder will track any needed updates for the ETM4.x series.

Only where the trace device is ETE, there will be the new magic number
and different ID registers be used - so the call to OpenCSD will
request an ETE decoder to be created.

The register value structures supplied to OpenCSD on decoder creation,
differ between ETM4.x and ETE.

Regards

Mike


> Thanks,
> Leo



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH 3/6] perf cs-etm: Save TRCDEVARCH register

[James Clark]

On 02/08/2021 13:05, Leo Yan wrote:
> If so, I understand your suggestion in another email to add a new magic
> number and a new protocol (this patch set has added the new protocol
> CS_ETM_PROTO_ETE) for creating ETE decoder.

Yes this is what I'm working on now. I think that it's actually
cleaner than the previous change where there was some half way
solution that old versions of perf wouldn't properly decode ETE
but would try to anyway.

James

[PATCH 4/6] perf cs-etm: Update OpenCSD decoder for ETE

[James Clark]

OpenCSD v1.1.1 has a bug fix for the installation of the ETE decoder
headers. This also means that including headers separately for each
decoder is unnecessary so remove these.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/build/feature/test-libopencsd.c           | 4 ++--
 tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 2 --
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/build/feature/test-libopencsd.c b/tools/build/feature/test-libopencsd.c
index 52c790b0317b..eb6303ff446e 100644
--- a/tools/build/feature/test-libopencsd.c
+++ b/tools/build/feature/test-libopencsd.c
@@ -4,9 +4,9 @@
 /*
  * Check OpenCSD library version is sufficient to provide required features
  */
-#define OCSD_MIN_VER ((1 << 16) | (0 << 8) | (0))
+#define OCSD_MIN_VER ((1 << 16) | (1 << 8) | (1))
 #if !defined(OCSD_VER_NUM) || (OCSD_VER_NUM < OCSD_MIN_VER)
-#error "OpenCSD >= 1.0.0 is required"
+#error "OpenCSD >= 1.1.1 is required"
 #endif
 
 int main(void)
diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
index 5972a8afcc6b..60147c908425 100644
--- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
+++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
@@ -13,8 +13,6 @@
 #include <linux/zalloc.h>
 #include <stdlib.h>
 #include <opencsd/c_api/opencsd_c_api.h>
-#include <opencsd/etmv4/trc_pkt_types_etmv4.h>
-#include <opencsd/ocsd_if_types.h>
 
 #include "cs-etm.h"
 #include "cs-etm-decoder.h"
-- 
2.28.0

Re: [PATCH 4/6] perf cs-etm: Update OpenCSD decoder for ETE

[Leo Yan]

On Wed, Jul 21, 2021 at 10:07:03AM +0100, James Clark wrote:
> OpenCSD v1.1.1 has a bug fix for the installation of the ETE decoder
> headers. This also means that including headers separately for each
> decoder is unnecessary so remove these.
> 
> Signed-off-by: James Clark <james.clark@arm.com>

IIUC, only the later patches in this patch set are dependent on OpenCSD
v1.1.1.  After I checked OpenCSD latest header, this change LGTM:

Reviewed-by: Leo Yan <leo.yan@linaro.org>

> ---
>  tools/build/feature/test-libopencsd.c           | 4 ++--
>  tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 2 --
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/build/feature/test-libopencsd.c b/tools/build/feature/test-libopencsd.c
> index 52c790b0317b..eb6303ff446e 100644
> --- a/tools/build/feature/test-libopencsd.c
> +++ b/tools/build/feature/test-libopencsd.c
> @@ -4,9 +4,9 @@
>  /*
>   * Check OpenCSD library version is sufficient to provide required features
>   */
> -#define OCSD_MIN_VER ((1 << 16) | (0 << 8) | (0))
> +#define OCSD_MIN_VER ((1 << 16) | (1 << 8) | (1))
>  #if !defined(OCSD_VER_NUM) || (OCSD_VER_NUM < OCSD_MIN_VER)
> -#error "OpenCSD >= 1.0.0 is required"
> +#error "OpenCSD >= 1.1.1 is required"
>  #endif
>  
>  int main(void)
> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> index 5972a8afcc6b..60147c908425 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> @@ -13,8 +13,6 @@
>  #include <linux/zalloc.h>
>  #include <stdlib.h>
>  #include <opencsd/c_api/opencsd_c_api.h>
> -#include <opencsd/etmv4/trc_pkt_types_etmv4.h>
> -#include <opencsd/ocsd_if_types.h>
>  
>  #include "cs-etm.h"
>  #include "cs-etm-decoder.h"
> -- 
> 2.28.0
> 

[PATCH 5/6] perf cs-etm: Create ETE decoder

[James Clark]

If the TRCDEVARCH register was saved, and it shows that ETE is present,
then instantiate an OCSD_BUILTIN_DCD_ETE decoder instead of
OCSD_BUILTIN_DCD_ETMV4I. ETE is the new trace feature for Armv9.

Testing performed
=================

* Old files with v0 headers still open correctly
* Old files with v1 headers with no TRCDEVARCH saved still open
* New files with TRCDEVARCH open using an old version of perf that
  supports v1 headers
* Coresight decoding results in the same output if there are no new ETE
  packet types

Signed-off-by: James Clark <james.clark@arm.com>
---
 .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 29 ++++++++++-
 .../perf/util/cs-etm-decoder/cs-etm-decoder.h |  7 +++
 tools/perf/util/cs-etm.c                      | 49 ++++++++++++++++++-
 tools/perf/util/cs-etm.h                      |  1 +
 4 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
index 60147c908425..37bc9d6a7677 100644
--- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
+++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
@@ -127,8 +127,12 @@ static int cs_etm_decoder__gen_etmv3_config(struct cs_etm_trace_params *params,
 #define TRCIDR1_TRCARCHMIN_SHIFT 4
 #define TRCIDR1_TRCARCHMIN_MASK  GENMASK(7, 4)
 #define TRCIDR1_TRCARCHMIN(x)    (((x) & TRCIDR1_TRCARCHMIN_MASK) >> TRCIDR1_TRCARCHMIN_SHIFT)
-static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1)
+static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1, u32 reg_devarch)
 {
+	/* ETE has to be v9 so set arch version to v8.3+ (ARCH__AA64) */
+	if (cs_etm__is_ete(reg_devarch))
+		return ARCH_AA64;
+
 	/*
 	 * If the ETM trace minor version is 4 or more then we can assume
 	 * the architecture is ARCH_AA64 rather than just V8
@@ -150,7 +154,22 @@ static void cs_etm_decoder__gen_etmv4_config(struct cs_etm_trace_params *params,
 	config->reg_idr11 = 0;
 	config->reg_idr12 = 0;
 	config->reg_idr13 = 0;
-	config->arch_ver = cs_etm_decoder__get_arch_ver(params->etmv4.reg_idr1);
+	config->arch_ver = cs_etm_decoder__get_arch_ver(params->etmv4.reg_idr1, 0);
+	config->core_prof = profile_CortexA;
+}
+
+static void cs_etm_decoder__gen_ete_config(struct cs_etm_trace_params *params,
+					   ocsd_ete_cfg *config)
+{
+	config->reg_configr = params->ete.base_params.reg_configr;
+	config->reg_traceidr = params->ete.base_params.reg_traceidr;
+	config->reg_idr0 = params->ete.base_params.reg_idr0;
+	config->reg_idr1 = params->ete.base_params.reg_idr1;
+	config->reg_idr2 = params->ete.base_params.reg_idr2;
+	config->reg_idr8 = params->ete.base_params.reg_idr8;
+	config->reg_devarch = params->ete.reg_devarch;
+	config->arch_ver = cs_etm_decoder__get_arch_ver(params->ete.base_params.reg_idr1,
+							params->ete.reg_devarch);
 	config->core_prof = profile_CortexA;
 }
 
@@ -598,6 +617,7 @@ cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
 	const char *decoder_name;
 	ocsd_etmv3_cfg config_etmv3;
 	ocsd_etmv4_cfg trace_config_etmv4;
+	ocsd_ete_cfg trace_config_ete;
 	void *trace_config;
 	u8 csid;
 
@@ -615,6 +635,11 @@ cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
 		decoder_name = OCSD_BUILTIN_DCD_ETMV4I;
 		trace_config = &trace_config_etmv4;
 		break;
+	case CS_ETM_PROTO_ETE:
+		cs_etm_decoder__gen_ete_config(t_params, &trace_config_ete);
+		decoder_name = OCSD_BUILTIN_DCD_ETE;
+		trace_config = &trace_config_ete;
+		break;
 	default:
 		return -1;
 	}
diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
index 11f3391d06f2..9137796fe3c5 100644
--- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
+++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
@@ -37,11 +37,17 @@ struct cs_etmv4_trace_params {
 	u32 reg_traceidr;
 };
 
+struct cs_ete_trace_params {
+	struct cs_etmv4_trace_params base_params;
+	u32 reg_devarch;
+};
+
 struct cs_etm_trace_params {
 	int protocol;
 	union {
 		struct cs_etmv3_trace_params etmv3;
 		struct cs_etmv4_trace_params etmv4;
+		struct cs_ete_trace_params ete;
 	};
 };
 
@@ -65,6 +71,7 @@ enum {
 	CS_ETM_PROTO_ETMV4i,
 	CS_ETM_PROTO_ETMV4d,
 	CS_ETM_PROTO_PTM,
+	CS_ETM_PROTO_ETE
 };
 
 enum cs_etm_decoder_operation {
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 68978f6707a8..870073bce670 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -460,11 +460,44 @@ static void cs_etm__set_trace_param_etmv4(struct cs_etm_trace_params *t_params,
 	t_params[idx].etmv4.reg_traceidr = metadata[idx][CS_ETMV4_TRCTRACEIDR];
 }
 
+static void cs_etm__set_trace_param_ete(struct cs_etm_trace_params *t_params,
+					  struct cs_etm_auxtrace *etm, int idx)
+{
+	u64 **metadata = etm->metadata;
+
+	t_params[idx].protocol = CS_ETM_PROTO_ETE;
+	t_params[idx].ete.base_params.reg_idr0 = metadata[idx][CS_ETMV4_TRCIDR0];
+	t_params[idx].ete.base_params.reg_idr1 = metadata[idx][CS_ETMV4_TRCIDR1];
+	t_params[idx].ete.base_params.reg_idr2 = metadata[idx][CS_ETMV4_TRCIDR2];
+	t_params[idx].ete.base_params.reg_idr8 = metadata[idx][CS_ETMV4_TRCIDR8];
+	t_params[idx].ete.base_params.reg_configr = metadata[idx][CS_ETMV4_TRCCONFIGR];
+	t_params[idx].ete.base_params.reg_traceidr = metadata[idx][CS_ETMV4_TRCTRACEIDR];
+	t_params[idx].ete.reg_devarch = metadata[idx][CS_ETE_TRCDEVARCH];
+}
+
+#define TRCDEVARCH_ARCHPART_SHIFT 0
+#define TRCDEVARCH_ARCHPART_MASK  GENMASK(11, 0)
+#define TRCDEVARCH_ARCHPART(x)    (((x) & TRCDEVARCH_ARCHPART_MASK) >> TRCDEVARCH_ARCHPART_SHIFT)
+
+#define TRCDEVARCH_ARCHVER_SHIFT 12
+#define TRCDEVARCH_ARCHVER_MASK  GENMASK(15, 12)
+#define TRCDEVARCH_ARCHVER(x)    (((x) & TRCDEVARCH_ARCHVER_MASK) >> TRCDEVARCH_ARCHVER_SHIFT)
+
+bool cs_etm__is_ete(u32 trcdevarch)
+{
+	/*
+	 * ETE if ARCHVER is 5 (ARCHVER is 4 for ETM) and ARCHPART is 0xA13.
+	 * See ETM_DEVARCH_ETE_ARCH in coresight-etm4x.h
+	 */
+	return TRCDEVARCH_ARCHVER(trcdevarch) == 5 && TRCDEVARCH_ARCHPART(trcdevarch) == 0xA13;
+}
+
+
 static int cs_etm__init_trace_params(struct cs_etm_trace_params *t_params,
 				     struct cs_etm_auxtrace *etm,
 				     int decoders_per_cpu)
 {
-	int i;
+	int i, num_params;
 	u32 etmidr;
 	u64 architecture;
 
@@ -477,7 +510,19 @@ static int cs_etm__init_trace_params(struct cs_etm_trace_params *t_params,
 			cs_etm__set_trace_param_etmv3(t_params, etm, i, etmidr);
 			break;
 		case __perf_cs_etmv4_magic:
-			cs_etm__set_trace_param_etmv4(t_params, etm, i);
+			/*
+			 * If devarch was saved and shows ETE, initialise ETE decoder. Otherwise
+			 * ETM decoder will still be able to decode a subset of the data. The total
+			 * number of params is number of params saved + common block size for v1,
+			 * see cs_etm_get_metadata().
+			 */
+			num_params = etm->metadata[i][CS_ETM_NR_TRC_PARAMS] +
+					CS_ETM_COMMON_BLK_MAX_V1;
+			if (num_params > CS_ETE_TRCDEVARCH &&
+			    cs_etm__is_ete(etm->metadata[i][CS_ETE_TRCDEVARCH]))
+				cs_etm__set_trace_param_ete(t_params, etm, i);
+			else
+				cs_etm__set_trace_param_etmv4(t_params, etm, i);
 			break;
 		default:
 			return -EINVAL;
diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h
index 52d82dce9d59..514083819657 100644
--- a/tools/perf/util/cs-etm.h
+++ b/tools/perf/util/cs-etm.h
@@ -203,6 +203,7 @@ void cs_etm__etmq_set_traceid_queue_timestamp(struct cs_etm_queue *etmq,
 					      u8 trace_chan_id);
 struct cs_etm_packet_queue
 *cs_etm__etmq_get_packet_queue(struct cs_etm_queue *etmq, u8 trace_chan_id);
+bool cs_etm__is_ete(u32 trcdevarch);
 #else
 static inline int
 cs_etm__process_auxtrace_info(union perf_event *event __maybe_unused,
-- 
2.28.0

Re: [PATCH 5/6] perf cs-etm: Create ETE decoder

[Leo Yan]

On Wed, Jul 21, 2021 at 10:07:04AM +0100, James Clark wrote:
> If the TRCDEVARCH register was saved, and it shows that ETE is present,
> then instantiate an OCSD_BUILTIN_DCD_ETE decoder instead of
> OCSD_BUILTIN_DCD_ETMV4I. ETE is the new trace feature for Armv9.
> 
> Testing performed
> =================
> 
> * Old files with v0 headers still open correctly
> * Old files with v1 headers with no TRCDEVARCH saved still open
> * New files with TRCDEVARCH open using an old version of perf that
>   supports v1 headers
> * Coresight decoding results in the same output if there are no new ETE
>   packet types
> 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 29 ++++++++++-
>  .../perf/util/cs-etm-decoder/cs-etm-decoder.h |  7 +++
>  tools/perf/util/cs-etm.c                      | 49 ++++++++++++++++++-
>  tools/perf/util/cs-etm.h                      |  1 +
>  4 files changed, 82 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> index 60147c908425..37bc9d6a7677 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> @@ -127,8 +127,12 @@ static int cs_etm_decoder__gen_etmv3_config(struct cs_etm_trace_params *params,
>  #define TRCIDR1_TRCARCHMIN_SHIFT 4
>  #define TRCIDR1_TRCARCHMIN_MASK  GENMASK(7, 4)
>  #define TRCIDR1_TRCARCHMIN(x)    (((x) & TRCIDR1_TRCARCHMIN_MASK) >> TRCIDR1_TRCARCHMIN_SHIFT)
> -static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1)
> +static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1, u32 reg_devarch)
>  {
> +	/* ETE has to be v9 so set arch version to v8.3+ (ARCH__AA64) */
> +	if (cs_etm__is_ete(reg_devarch))
> +		return ARCH_AA64;
> +

Based on values used in below change, I think we can unify the ETM
versio number like:

  ARCH_V8R3 : REVISION, bits[19:16] is 0x3
  ARCH_V8R4 : REVISION, bits[19:16] is 0x4
  ARCH_V8R5 : REVISION, bits[19:16] is 0x5

>  	/*
>  	 * If the ETM trace minor version is 4 or more then we can assume
>  	 * the architecture is ARCH_AA64 rather than just V8
> @@ -150,7 +154,22 @@ static void cs_etm_decoder__gen_etmv4_config(struct cs_etm_trace_params *params,
>  	config->reg_idr11 = 0;
>  	config->reg_idr12 = 0;
>  	config->reg_idr13 = 0;
> -	config->arch_ver = cs_etm_decoder__get_arch_ver(params->etmv4.reg_idr1);
> +	config->arch_ver = cs_etm_decoder__get_arch_ver(params->etmv4.reg_idr1, 0);

Can we always pass the value "params->ete.reg_devarch" rather than
directly pass "0"?

> +	config->core_prof = profile_CortexA;
> +}
> +
> +static void cs_etm_decoder__gen_ete_config(struct cs_etm_trace_params *params,
> +					   ocsd_ete_cfg *config)
> +{
> +	config->reg_configr = params->ete.base_params.reg_configr;
> +	config->reg_traceidr = params->ete.base_params.reg_traceidr;
> +	config->reg_idr0 = params->ete.base_params.reg_idr0;
> +	config->reg_idr1 = params->ete.base_params.reg_idr1;
> +	config->reg_idr2 = params->ete.base_params.reg_idr2;
> +	config->reg_idr8 = params->ete.base_params.reg_idr8;
> +	config->reg_devarch = params->ete.reg_devarch;
> +	config->arch_ver = cs_etm_decoder__get_arch_ver(params->ete.base_params.reg_idr1,
> +							params->ete.reg_devarch);
>  	config->core_prof = profile_CortexA;
>  }
>  
> @@ -598,6 +617,7 @@ cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
>  	const char *decoder_name;
>  	ocsd_etmv3_cfg config_etmv3;
>  	ocsd_etmv4_cfg trace_config_etmv4;
> +	ocsd_ete_cfg trace_config_ete;
>  	void *trace_config;
>  	u8 csid;
>  
> @@ -615,6 +635,11 @@ cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
>  		decoder_name = OCSD_BUILTIN_DCD_ETMV4I;
>  		trace_config = &trace_config_etmv4;
>  		break;
> +	case CS_ETM_PROTO_ETE:
> +		cs_etm_decoder__gen_ete_config(t_params, &trace_config_ete);
> +		decoder_name = OCSD_BUILTIN_DCD_ETE;
> +		trace_config = &trace_config_ete;
> +		break;
>  	default:
>  		return -1;
>  	}
> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
> index 11f3391d06f2..9137796fe3c5 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
> @@ -37,11 +37,17 @@ struct cs_etmv4_trace_params {
>  	u32 reg_traceidr;
>  };
>  
> +struct cs_ete_trace_params {
> +	struct cs_etmv4_trace_params base_params;
> +	u32 reg_devarch;

As we have said, can we directly support ETMv4.5, so that it can
smoothly support ETE features?  If so, we don't need to add a new
structure "cs_ete_trace_params" at here.

> +};
> +
>  struct cs_etm_trace_params {
>  	int protocol;
>  	union {
>  		struct cs_etmv3_trace_params etmv3;
>  		struct cs_etmv4_trace_params etmv4;
> +		struct cs_ete_trace_params ete;
>  	};
>  };
>  
> @@ -65,6 +71,7 @@ enum {
>  	CS_ETM_PROTO_ETMV4i,
>  	CS_ETM_PROTO_ETMV4d,
>  	CS_ETM_PROTO_PTM,
> +	CS_ETM_PROTO_ETE
>  };
>  
>  enum cs_etm_decoder_operation {
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 68978f6707a8..870073bce670 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -460,11 +460,44 @@ static void cs_etm__set_trace_param_etmv4(struct cs_etm_trace_params *t_params,
>  	t_params[idx].etmv4.reg_traceidr = metadata[idx][CS_ETMV4_TRCTRACEIDR];
>  }
>  
> +static void cs_etm__set_trace_param_ete(struct cs_etm_trace_params *t_params,
> +					  struct cs_etm_auxtrace *etm, int idx)
> +{
> +	u64 **metadata = etm->metadata;
> +
> +	t_params[idx].protocol = CS_ETM_PROTO_ETE;
> +	t_params[idx].ete.base_params.reg_idr0 = metadata[idx][CS_ETMV4_TRCIDR0];
> +	t_params[idx].ete.base_params.reg_idr1 = metadata[idx][CS_ETMV4_TRCIDR1];
> +	t_params[idx].ete.base_params.reg_idr2 = metadata[idx][CS_ETMV4_TRCIDR2];
> +	t_params[idx].ete.base_params.reg_idr8 = metadata[idx][CS_ETMV4_TRCIDR8];
> +	t_params[idx].ete.base_params.reg_configr = metadata[idx][CS_ETMV4_TRCCONFIGR];
> +	t_params[idx].ete.base_params.reg_traceidr = metadata[idx][CS_ETMV4_TRCTRACEIDR];
> +	t_params[idx].ete.reg_devarch = metadata[idx][CS_ETE_TRCDEVARCH];
> +}
> +
> +#define TRCDEVARCH_ARCHPART_SHIFT 0
> +#define TRCDEVARCH_ARCHPART_MASK  GENMASK(11, 0)
> +#define TRCDEVARCH_ARCHPART(x)    (((x) & TRCDEVARCH_ARCHPART_MASK) >> TRCDEVARCH_ARCHPART_SHIFT)
> +
> +#define TRCDEVARCH_ARCHVER_SHIFT 12
> +#define TRCDEVARCH_ARCHVER_MASK  GENMASK(15, 12)
> +#define TRCDEVARCH_ARCHVER(x)    (((x) & TRCDEVARCH_ARCHVER_MASK) >> TRCDEVARCH_ARCHVER_SHIFT)
> +
> +bool cs_etm__is_ete(u32 trcdevarch)
> +{
> +	/*
> +	 * ETE if ARCHVER is 5 (ARCHVER is 4 for ETM) and ARCHPART is 0xA13.
> +	 * See ETM_DEVARCH_ETE_ARCH in coresight-etm4x.h
> +	 */
> +	return TRCDEVARCH_ARCHVER(trcdevarch) == 5 && TRCDEVARCH_ARCHPART(trcdevarch) == 0xA13;

I think this is incorrect.

Here should check the bit field "REVISION, bits[19:16]".  If it's
field value is >= 5, then we can say it supports ETE.  I checked the
spec for ETMv4.4 and ETMv4.6, both use the same values for the
Bits[15:12] = 0x4, so the architecture ID is same for ETMv4.x IPs.

Thanks,
Leo

> +}
> +
> +
>  static int cs_etm__init_trace_params(struct cs_etm_trace_params *t_params,
>  				     struct cs_etm_auxtrace *etm,
>  				     int decoders_per_cpu)
>  {
> -	int i;
> +	int i, num_params;
>  	u32 etmidr;
>  	u64 architecture;
>  
> @@ -477,7 +510,19 @@ static int cs_etm__init_trace_params(struct cs_etm_trace_params *t_params,
>  			cs_etm__set_trace_param_etmv3(t_params, etm, i, etmidr);
>  			break;
>  		case __perf_cs_etmv4_magic:
> -			cs_etm__set_trace_param_etmv4(t_params, etm, i);
> +			/*
> +			 * If devarch was saved and shows ETE, initialise ETE decoder. Otherwise
> +			 * ETM decoder will still be able to decode a subset of the data. The total
> +			 * number of params is number of params saved + common block size for v1,
> +			 * see cs_etm_get_metadata().
> +			 */
> +			num_params = etm->metadata[i][CS_ETM_NR_TRC_PARAMS] +
> +					CS_ETM_COMMON_BLK_MAX_V1;
> +			if (num_params > CS_ETE_TRCDEVARCH &&
> +			    cs_etm__is_ete(etm->metadata[i][CS_ETE_TRCDEVARCH]))
> +				cs_etm__set_trace_param_ete(t_params, etm, i);
> +			else
> +				cs_etm__set_trace_param_etmv4(t_params, etm, i);
>  			break;
>  		default:
>  			return -EINVAL;
> diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h
> index 52d82dce9d59..514083819657 100644
> --- a/tools/perf/util/cs-etm.h
> +++ b/tools/perf/util/cs-etm.h
> @@ -203,6 +203,7 @@ void cs_etm__etmq_set_traceid_queue_timestamp(struct cs_etm_queue *etmq,
>  					      u8 trace_chan_id);
>  struct cs_etm_packet_queue
>  *cs_etm__etmq_get_packet_queue(struct cs_etm_queue *etmq, u8 trace_chan_id);
> +bool cs_etm__is_ete(u32 trcdevarch);
>  #else
>  static inline int
>  cs_etm__process_auxtrace_info(union perf_event *event __maybe_unused,
> -- 
> 2.28.0
> 

Re: [PATCH 5/6] perf cs-etm: Create ETE decoder

[James Clark]

On 31/07/2021 08:23, Leo Yan wrote:
> On Wed, Jul 21, 2021 at 10:07:04AM +0100, James Clark wrote:
>> If the TRCDEVARCH register was saved, and it shows that ETE is present,
>> then instantiate an OCSD_BUILTIN_DCD_ETE decoder instead of
>> OCSD_BUILTIN_DCD_ETMV4I. ETE is the new trace feature for Armv9.
>>
>> Testing performed
>> =================
>>
>> * Old files with v0 headers still open correctly
>> * Old files with v1 headers with no TRCDEVARCH saved still open
>> * New files with TRCDEVARCH open using an old version of perf that
>>   supports v1 headers
>> * Coresight decoding results in the same output if there are no new ETE
>>   packet types
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>>  .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 29 ++++++++++-
>>  .../perf/util/cs-etm-decoder/cs-etm-decoder.h |  7 +++
>>  tools/perf/util/cs-etm.c                      | 49 ++++++++++++++++++-
>>  tools/perf/util/cs-etm.h                      |  1 +
>>  4 files changed, 82 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
>> index 60147c908425..37bc9d6a7677 100644
>> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
>> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
>> @@ -127,8 +127,12 @@ static int cs_etm_decoder__gen_etmv3_config(struct cs_etm_trace_params *params,
>>  #define TRCIDR1_TRCARCHMIN_SHIFT 4
>>  #define TRCIDR1_TRCARCHMIN_MASK  GENMASK(7, 4)
>>  #define TRCIDR1_TRCARCHMIN(x)    (((x) & TRCIDR1_TRCARCHMIN_MASK) >> TRCIDR1_TRCARCHMIN_SHIFT)
>> -static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1)
>> +static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1, u32 reg_devarch)
>>  {
>> +	/* ETE has to be v9 so set arch version to v8.3+ (ARCH__AA64) */
>> +	if (cs_etm__is_ete(reg_devarch))
>> +		return ARCH_AA64;
>> +
> 
> Based on values used in below change, I think we can unify the ETM
> versio number like:
> 
>   ARCH_V8R3 : REVISION, bits[19:16] is 0x3
>   ARCH_V8R4 : REVISION, bits[19:16] is 0x4
>   ARCH_V8R5 : REVISION, bits[19:16] is 0x5

Do you mean make this change in OpenCSD? At the moment it understands these
values so I'm not sure if the extra ones would be useful:

	/** Core Architecture Version */
	typedef enum _ocsd_arch_version {
	    ARCH_UNKNOWN = 0x0000,   /**< unknown architecture */
	    ARCH_CUSTOM = 0x0001,    /**< None ARM, custom architecture */
	    ARCH_V7 = 0x0700,        /**< V7 architecture */
	    ARCH_V8 = 0x0800,        /**< V8 architecture */
	    ARCH_V8r3 = 0x0803,      /**< V8.3 architecture */
	    ARCH_AA64 = 0x0864,      /**< Min v8r3 plus additional AA64 PE features */
	    ARCH_V8_max = ARCH_AA64,
	} ocsd_arch_version_t;

[...]

>> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
>> index 11f3391d06f2..9137796fe3c5 100644
>> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
>> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
>> @@ -37,11 +37,17 @@ struct cs_etmv4_trace_params {
>>  	u32 reg_traceidr;
>>  };
>>  
>> +struct cs_ete_trace_params {
>> +	struct cs_etmv4_trace_params base_params;
>> +	u32 reg_devarch;
> 
> As we have said, can we directly support ETMv4.5, so that it can
> smoothly support ETE features?  If so, we don't need to add a new
> structure "cs_ete_trace_params" at here.
> 

I think with the new magic number change this is more likely to stay,
what are your thoughts?

[...]

>> +
>> +#define TRCDEVARCH_ARCHPART_SHIFT 0
>> +#define TRCDEVARCH_ARCHPART_MASK  GENMASK(11, 0)
>> +#define TRCDEVARCH_ARCHPART(x)    (((x) & TRCDEVARCH_ARCHPART_MASK) >> TRCDEVARCH_ARCHPART_SHIFT)
>> +
>> +#define TRCDEVARCH_ARCHVER_SHIFT 12
>> +#define TRCDEVARCH_ARCHVER_MASK  GENMASK(15, 12)
>> +#define TRCDEVARCH_ARCHVER(x)    (((x) & TRCDEVARCH_ARCHVER_MASK) >> TRCDEVARCH_ARCHVER_SHIFT)
>> +
>> +bool cs_etm__is_ete(u32 trcdevarch)
>> +{
>> +	/*
>> +	 * ETE if ARCHVER is 5 (ARCHVER is 4 for ETM) and ARCHPART is 0xA13.
>> +	 * See ETM_DEVARCH_ETE_ARCH in coresight-etm4x.h
>> +	 */
>> +	return TRCDEVARCH_ARCHVER(trcdevarch) == 5 && TRCDEVARCH_ARCHPART(trcdevarch) == 0xA13;
> 
> I think this is incorrect.
> 
> Here should check the bit field "REVISION, bits[19:16]".  If it's
> field value is >= 5, then we can say it supports ETE.  I checked the
> spec for ETMv4.4 and ETMv4.6, both use the same values for the
> Bits[15:12] = 0x4, so the architecture ID is same for ETMv4.x IPs.
> 

I tried to copy this as closely as possible from the ETE driver. See in coresight-etm4x.h

	#define ETM_DEVARCH_ETE_ARCH						\
		(ETM_DEVARCH_ARCHITECT_ARM | ETM_DEVARCH_ARCHID_ETE | ETM_DEVARCH_PRESENT) 

Where ETM_DEVARCH_ARCHID_ETE is ARCHVER == 5 and ARCHPART == 0xA13. I didn't check 
ETM_DEVARCH_ARCHITECT_ARM because I thought that wouldn't be necessary. If we want to make
the change do detect >= 5 then I think this should be made in the driver first. @Suzuki,
what do you think?

Thanks
James

Re: [PATCH 5/6] perf cs-etm: Create ETE decoder

[Leo Yan]

On Tue, Aug 03, 2021 at 02:09:38PM +0100, James Clark wrote:

[...]

> >> -static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1)
> >> +static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1, u32 reg_devarch)
> >>  {
> >> +	/* ETE has to be v9 so set arch version to v8.3+ (ARCH__AA64) */
> >> +	if (cs_etm__is_ete(reg_devarch))
> >> +		return ARCH_AA64;
> >> +
> > 
> > Based on values used in below change, I think we can unify the ETM
> > versio number like:
> > 
> >   ARCH_V8R3 : REVISION, bits[19:16] is 0x3
> >   ARCH_V8R4 : REVISION, bits[19:16] is 0x4
> >   ARCH_V8R5 : REVISION, bits[19:16] is 0x5
> 
> Do you mean make this change in OpenCSD? At the moment it understands these
> values so I'm not sure if the extra ones would be useful:

Yes.  As Mike said, these new macros will cause big changes in OpenCSD,
so I don't have strong opinion to add more macros for tracer versions.

> >> +struct cs_ete_trace_params {
> >> +	struct cs_etmv4_trace_params base_params;
> >> +	u32 reg_devarch;
> > 
> > As we have said, can we directly support ETMv4.5, so that it can
> > smoothly support ETE features?  If so, we don't need to add a new
> > structure "cs_ete_trace_params" at here.
> > 
> 
> I think with the new magic number change this is more likely to stay,
> what are your thoughts?

Agreed.  Just wander if need to define the struct cs_ete_trace_params
as below?

  struct cs_ete_trace_params {
          u32 reg_idr0;
          u32 reg_idr1;
          u32 reg_idr2;
          u32 reg_idr8;
          u32 reg_configr;
          u32 reg_traceidr;
          u32 reg_devarch;
  }

> >> +
> >> +#define TRCDEVARCH_ARCHPART_SHIFT 0
> >> +#define TRCDEVARCH_ARCHPART_MASK  GENMASK(11, 0)
> >> +#define TRCDEVARCH_ARCHPART(x)    (((x) & TRCDEVARCH_ARCHPART_MASK) >> TRCDEVARCH_ARCHPART_SHIFT)
> >> +
> >> +#define TRCDEVARCH_ARCHVER_SHIFT 12
> >> +#define TRCDEVARCH_ARCHVER_MASK  GENMASK(15, 12)
> >> +#define TRCDEVARCH_ARCHVER(x)    (((x) & TRCDEVARCH_ARCHVER_MASK) >> TRCDEVARCH_ARCHVER_SHIFT)
> >> +
> >> +bool cs_etm__is_ete(u32 trcdevarch)
> >> +{
> >> +	/*
> >> +	 * ETE if ARCHVER is 5 (ARCHVER is 4 for ETM) and ARCHPART is 0xA13.
> >> +	 * See ETM_DEVARCH_ETE_ARCH in coresight-etm4x.h
> >> +	 */
> >> +	return TRCDEVARCH_ARCHVER(trcdevarch) == 5 && TRCDEVARCH_ARCHPART(trcdevarch) == 0xA13;
> > 
> > I think this is incorrect.
> > 
> > Here should check the bit field "REVISION, bits[19:16]".  If it's
> > field value is >= 5, then we can say it supports ETE.  I checked the
> > spec for ETMv4.4 and ETMv4.6, both use the same values for the
> > Bits[15:12] = 0x4, so the architecture ID is same for ETMv4.x IPs.
> > 
> 
> I tried to copy this as closely as possible from the ETE driver. See in coresight-etm4x.h
> 
> 	#define ETM_DEVARCH_ETE_ARCH						\
> 		(ETM_DEVARCH_ARCHITECT_ARM | ETM_DEVARCH_ARCHID_ETE | ETM_DEVARCH_PRESENT) 
> 
> Where ETM_DEVARCH_ARCHID_ETE is ARCHVER == 5 and ARCHPART == 0xA13. I didn't check 
> ETM_DEVARCH_ARCHITECT_ARM because I thought that wouldn't be necessary. If we want to make
> the change do detect >= 5 then I think this should be made in the driver first. @Suzuki,
> what do you think?

The tracer has two fields:

- ARCHID bits[15:12]
- REVISION, bits[19:16]

For ETE its ARCHID[15:12] is 0x5 and ETMv4.x's ARCHID[15:12] is 0x4.
So checking ARCHID[15:12] is the right way to distinguish if the
tracer is ETE and creates corresponding decoder for it.

When reviewed this patch I assumed we also need to create ETE decoder
if ETMv4.x has supported packet extension.  As Mike confirmed, all
ETMv4.x tracers keep to use existed way to create decoder; so it's not
necessary to check REVISION bit field.

So please ignore my this comment.

Thanks,
Leo

[PATCH 6/6] perf cs-etm: Print the decoder name

[James Clark]

Use the real name of the decoder instead of hard-coding "ETM" to avoid
confusion when the trace is ETE. This also now distinguishes between
ETMv3 and ETMv4.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 17 +++++++++++------
 tools/perf/util/cs-etm-decoder/cs-etm-decoder.h |  1 +
 tools/perf/util/cs-etm.c                        |  4 ++--
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
index 37bc9d6a7677..177c08f5be8d 100644
--- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
+++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
@@ -37,6 +37,7 @@ struct cs_etm_decoder {
 	dcd_tree_handle_t dcd_tree;
 	cs_etm_mem_cb_type mem_access;
 	ocsd_datapath_resp_t prev_return;
+	const char *decoder_name;
 };
 
 static u32
@@ -614,7 +615,6 @@ cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
 				   struct cs_etm_trace_params *t_params,
 				   struct cs_etm_decoder *decoder)
 {
-	const char *decoder_name;
 	ocsd_etmv3_cfg config_etmv3;
 	ocsd_etmv4_cfg trace_config_etmv4;
 	ocsd_ete_cfg trace_config_ete;
@@ -625,19 +625,19 @@ cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
 	case CS_ETM_PROTO_ETMV3:
 	case CS_ETM_PROTO_PTM:
 		cs_etm_decoder__gen_etmv3_config(t_params, &config_etmv3);
-		decoder_name = (t_params->protocol == CS_ETM_PROTO_ETMV3) ?
+		decoder->decoder_name = (t_params->protocol == CS_ETM_PROTO_ETMV3) ?
 							OCSD_BUILTIN_DCD_ETMV3 :
 							OCSD_BUILTIN_DCD_PTM;
 		trace_config = &config_etmv3;
 		break;
 	case CS_ETM_PROTO_ETMV4i:
 		cs_etm_decoder__gen_etmv4_config(t_params, &trace_config_etmv4);
-		decoder_name = OCSD_BUILTIN_DCD_ETMV4I;
+		decoder->decoder_name = OCSD_BUILTIN_DCD_ETMV4I;
 		trace_config = &trace_config_etmv4;
 		break;
 	case CS_ETM_PROTO_ETE:
 		cs_etm_decoder__gen_ete_config(t_params, &trace_config_ete);
-		decoder_name = OCSD_BUILTIN_DCD_ETE;
+		decoder->decoder_name = OCSD_BUILTIN_DCD_ETE;
 		trace_config = &trace_config_ete;
 		break;
 	default:
@@ -646,7 +646,7 @@ cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
 
 	if (d_params->operation == CS_ETM_OPERATION_DECODE) {
 		if (ocsd_dt_create_decoder(decoder->dcd_tree,
-					   decoder_name,
+					   decoder->decoder_name,
 					   OCSD_CREATE_FLG_FULL_DECODER,
 					   trace_config, &csid))
 			return -1;
@@ -658,7 +658,7 @@ cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
 
 		return 0;
 	} else if (d_params->operation == CS_ETM_OPERATION_PRINT) {
-		if (ocsd_dt_create_decoder(decoder->dcd_tree, decoder_name,
+		if (ocsd_dt_create_decoder(decoder->dcd_tree, decoder->decoder_name,
 					   OCSD_CREATE_FLG_PACKET_PROC,
 					   trace_config, &csid))
 			return -1;
@@ -790,3 +790,8 @@ void cs_etm_decoder__free(struct cs_etm_decoder *decoder)
 	decoder->dcd_tree = NULL;
 	free(decoder);
 }
+
+const char *cs_etm_decoder__get_name(struct cs_etm_decoder *decoder)
+{
+	return decoder->decoder_name;
+}
diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
index 9137796fe3c5..fbd6786baf99 100644
--- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
+++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
@@ -99,5 +99,6 @@ int cs_etm_decoder__get_packet(struct cs_etm_packet_queue *packet_queue,
 			       struct cs_etm_packet *packet);
 
 int cs_etm_decoder__reset(struct cs_etm_decoder *decoder);
+const char *cs_etm_decoder__get_name(struct cs_etm_decoder *decoder);
 
 #endif /* INCLUDE__CS_ETM_DECODER_H__ */
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 870073bce670..bb7957afd9cb 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -564,8 +564,8 @@ static void cs_etm__dump_event(struct cs_etm_queue *etmq,
 
 	fprintf(stdout, "\n");
 	color_fprintf(stdout, color,
-		     ". ... CoreSight ETM Trace data: size %zu bytes\n",
-		     buffer->size);
+		     ". ... CoreSight %s Trace data: size %zu bytes\n",
+		     cs_etm_decoder__get_name(etmq->decoder), buffer->size);
 
 	do {
 		size_t consumed;
-- 
2.28.0

Re: [PATCH 6/6] perf cs-etm: Print the decoder name

[Leo Yan]

On Wed, Jul 21, 2021 at 10:07:05AM +0100, James Clark wrote:
> Use the real name of the decoder instead of hard-coding "ETM" to avoid
> confusion when the trace is ETE. This also now distinguishes between
> ETMv3 and ETMv4.
> 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 17 +++++++++++------
>  tools/perf/util/cs-etm-decoder/cs-etm-decoder.h |  1 +
>  tools/perf/util/cs-etm.c                        |  4 ++--
>  3 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> index 37bc9d6a7677..177c08f5be8d 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> @@ -37,6 +37,7 @@ struct cs_etm_decoder {
>  	dcd_tree_handle_t dcd_tree;
>  	cs_etm_mem_cb_type mem_access;
>  	ocsd_datapath_resp_t prev_return;
> +	const char *decoder_name;
>  };
>  
>  static u32
> @@ -614,7 +615,6 @@ cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
>  				   struct cs_etm_trace_params *t_params,
>  				   struct cs_etm_decoder *decoder)
>  {
> -	const char *decoder_name;
>  	ocsd_etmv3_cfg config_etmv3;
>  	ocsd_etmv4_cfg trace_config_etmv4;
>  	ocsd_ete_cfg trace_config_ete;
> @@ -625,19 +625,19 @@ cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
>  	case CS_ETM_PROTO_ETMV3:
>  	case CS_ETM_PROTO_PTM:
>  		cs_etm_decoder__gen_etmv3_config(t_params, &config_etmv3);
> -		decoder_name = (t_params->protocol == CS_ETM_PROTO_ETMV3) ?
> +		decoder->decoder_name = (t_params->protocol == CS_ETM_PROTO_ETMV3) ?
>  							OCSD_BUILTIN_DCD_ETMV3 :
>  							OCSD_BUILTIN_DCD_PTM;
>  		trace_config = &config_etmv3;
>  		break;
>  	case CS_ETM_PROTO_ETMV4i:
>  		cs_etm_decoder__gen_etmv4_config(t_params, &trace_config_etmv4);
> -		decoder_name = OCSD_BUILTIN_DCD_ETMV4I;
> +		decoder->decoder_name = OCSD_BUILTIN_DCD_ETMV4I;
>  		trace_config = &trace_config_etmv4;
>  		break;
>  	case CS_ETM_PROTO_ETE:
>  		cs_etm_decoder__gen_ete_config(t_params, &trace_config_ete);
> -		decoder_name = OCSD_BUILTIN_DCD_ETE;
> +		decoder->decoder_name = OCSD_BUILTIN_DCD_ETE;
>  		trace_config = &trace_config_ete;
>  		break;
>  	default:
> @@ -646,7 +646,7 @@ cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
>  
>  	if (d_params->operation == CS_ETM_OPERATION_DECODE) {
>  		if (ocsd_dt_create_decoder(decoder->dcd_tree,
> -					   decoder_name,
> +					   decoder->decoder_name,
>  					   OCSD_CREATE_FLG_FULL_DECODER,
>  					   trace_config, &csid))
>  			return -1;
> @@ -658,7 +658,7 @@ cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
>  
>  		return 0;
>  	} else if (d_params->operation == CS_ETM_OPERATION_PRINT) {
> -		if (ocsd_dt_create_decoder(decoder->dcd_tree, decoder_name,
> +		if (ocsd_dt_create_decoder(decoder->dcd_tree, decoder->decoder_name,
>  					   OCSD_CREATE_FLG_PACKET_PROC,
>  					   trace_config, &csid))
>  			return -1;
> @@ -790,3 +790,8 @@ void cs_etm_decoder__free(struct cs_etm_decoder *decoder)
>  	decoder->dcd_tree = NULL;
>  	free(decoder);
>  }
> +
> +const char *cs_etm_decoder__get_name(struct cs_etm_decoder *decoder)
> +{
> +	return decoder->decoder_name;
> +}

Maybe can consider to place this function into the header file with
"static inline" specifier.

Either way, this patch looks good to me:

Reviewed-by: Leo Yan <leo.yan@linaro.org>

> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
> index 9137796fe3c5..fbd6786baf99 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h
> @@ -99,5 +99,6 @@ int cs_etm_decoder__get_packet(struct cs_etm_packet_queue *packet_queue,
>  			       struct cs_etm_packet *packet);
>  
>  int cs_etm_decoder__reset(struct cs_etm_decoder *decoder);
> +const char *cs_etm_decoder__get_name(struct cs_etm_decoder *decoder);
>  
>  #endif /* INCLUDE__CS_ETM_DECODER_H__ */
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 870073bce670..bb7957afd9cb 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -564,8 +564,8 @@ static void cs_etm__dump_event(struct cs_etm_queue *etmq,
>  
>  	fprintf(stdout, "\n");
>  	color_fprintf(stdout, color,
> -		     ". ... CoreSight ETM Trace data: size %zu bytes\n",
> -		     buffer->size);
> +		     ". ... CoreSight %s Trace data: size %zu bytes\n",
> +		     cs_etm_decoder__get_name(etmq->decoder), buffer->size);
>  
>  	do {
>  		size_t consumed;
> -- 
> 2.28.0
> 

Re: [PATCH 6/6] perf cs-etm: Print the decoder name

[James Clark]

On 31/07/2021 08:30, Leo Yan wrote:
>> @@ -658,7 +658,7 @@ cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
>>  
>>  		return 0;
>>  	} else if (d_params->operation == CS_ETM_OPERATION_PRINT) {
>> -		if (ocsd_dt_create_decoder(decoder->dcd_tree, decoder_name,
>> +		if (ocsd_dt_create_decoder(decoder->dcd_tree, decoder->decoder_name,
>>  					   OCSD_CREATE_FLG_PACKET_PROC,
>>  					   trace_config, &csid))
>>  			return -1;
>> @@ -790,3 +790,8 @@ void cs_etm_decoder__free(struct cs_etm_decoder *decoder)
>>  	decoder->dcd_tree = NULL;
>>  	free(decoder);
>>  }
>> +
>> +const char *cs_etm_decoder__get_name(struct cs_etm_decoder *decoder)
>> +{
>> +	return decoder->decoder_name;
>> +}
> Maybe can consider to place this function into the header file with
> "static inline" specifier.

I tried this, but because the struct is defined in the .c file it can't
be done without moving the struct to the header. It's also only used
for the --dump-raw-trace path so performance isn't critical anyway.

James

> 
> Either way, this patch looks good to me:
> 
> Reviewed-by: Leo Yan <leo.yan@linaro.org>
> 

Re: [PATCH 6/6] perf cs-etm: Print the decoder name

[Leo Yan]

On Fri, Aug 06, 2021 at 10:43:25AM +0100, James Clark wrote:
> On 31/07/2021 08:30, Leo Yan wrote:
> >> @@ -658,7 +658,7 @@ cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
> >>  
> >>  		return 0;
> >>  	} else if (d_params->operation == CS_ETM_OPERATION_PRINT) {
> >> -		if (ocsd_dt_create_decoder(decoder->dcd_tree, decoder_name,
> >> +		if (ocsd_dt_create_decoder(decoder->dcd_tree, decoder->decoder_name,
> >>  					   OCSD_CREATE_FLG_PACKET_PROC,
> >>  					   trace_config, &csid))
> >>  			return -1;
> >> @@ -790,3 +790,8 @@ void cs_etm_decoder__free(struct cs_etm_decoder *decoder)
> >>  	decoder->dcd_tree = NULL;
> >>  	free(decoder);
> >>  }
> >> +
> >> +const char *cs_etm_decoder__get_name(struct cs_etm_decoder *decoder)
> >> +{
> >> +	return decoder->decoder_name;
> >> +}
> > Maybe can consider to place this function into the header file with
> > "static inline" specifier.
> 
> I tried this, but because the struct is defined in the .c file it can't
> be done without moving the struct to the header. It's also only used
> for the --dump-raw-trace path so performance isn't critical anyway.

It's fine to keep current patch, and thanks for trying.

Re: [PATCH 0/6] Support ETE decoding

[Mathieu Poirier]

Hi James,

I have received this set but won't be able to look at it for quite some time.

Thanks,
Mathieu

On Wed, 21 Jul 2021 at 03:07, James Clark <james.clark@arm.com> wrote:
>
> Decoding ETE already works because it is a superset of
> ETMv4, but if any new packet types are found then they will be
> ignored by the decoder. This patchset creates an ETE decoder
> which can output the new packets and saves a new register that
> is required. No new packet types are handled by perf yet, as this
> can be added in the future.
>
> This set applies on top of "perf cs-etm: Support TRBE
> (unformatted decoding)" on perf/core.
>
> James Clark (6):
>   perf cs-etm: Refactor initialisation of decoder params.
>   perf cs-etm: Initialise architecture based on TRCIDR1
>   perf cs-etm: Save TRCDEVARCH register
>   perf cs-etm: Update OpenCSD decoder for ETE
>   perf cs-etm: Create ETE decoder
>   perf cs-etm: Print the decoder name
>
>  tools/build/feature/test-libopencsd.c         |   4 +-
>  tools/perf/arch/arm/util/cs-etm.c             |  13 +-
>  .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 151 ++++++++----------
>  .../perf/util/cs-etm-decoder/cs-etm-decoder.h |   8 +
>  tools/perf/util/cs-etm.c                      |  54 ++++++-
>  tools/perf/util/cs-etm.h                      |   6 +-
>  6 files changed, 147 insertions(+), 89 deletions(-)
>
> --
> 2.28.0
>