Re: [PATCH v2 08/11] perf arm-spe: Fill branch operations and events to record

[James Clark <james.clark@linaro.org>]

On 14/02/2025 11:19 am, Leo Yan wrote:
> The new added branch operations and events are filled into record, the
> information will be consumed when synthesizing samples.
> 
> Reviewed-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>   .../util/arm-spe-decoder/arm-spe-decoder.c     | 18 ++++++++++++++++++
>   .../util/arm-spe-decoder/arm-spe-decoder.h     | 10 ++++++++--
>   2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> index ba807071d3c1..52bd0a4ea96d 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> @@ -207,6 +207,18 @@ static int arm_spe_read_record(struct arm_spe_decoder *decoder)
>   				break;
>   			case SPE_OP_PKT_HDR_CLASS_BR_ERET:
>   				decoder->record.op |= ARM_SPE_OP_BRANCH_ERET;
> +				if (payload & SPE_OP_PKT_COND)
> +					decoder->record.op |= ARM_SPE_OP_BR_COND;

I think this results in memory events being synthesised for these 
samples because of a bug in arm_spe__synth_data_source(). 
ARM_SPE_OP_BR_COND overlaps with bits from other packet types like 
ARM_SPE_OP_LD:

	if (record->op & ARM_SPE_OP_LD)
		data_src.mem_op = PERF_MEM_OP_LOAD;

arm_spe__synth_data_source() needs to only interpret that as a data 
source packet if ARM_SPE_OP_LDST is set. This was reported by Mike 
Williams but you have the privilege of hitting it for real first.

> +				if (payload & SPE_OP_PKT_INDIRECT_BRANCH)
> +					decoder->record.op |= ARM_SPE_OP_BR_INDIRECT;
> +				if (payload & SPE_OP_PKT_GCS)
> +					decoder->record.op |= ARM_SPE_OP_BR_GCS;
> +				if (SPE_OP_PKT_CR_BL(payload))
> +					decoder->record.op |= ARM_SPE_OP_BR_CR_BL;
> +				if (SPE_OP_PKT_CR_RET(payload))
> +					decoder->record.op |= ARM_SPE_OP_BR_CR_RET;
> +				if (SPE_OP_PKT_CR_NON_BL_RET(payload))
> +					decoder->record.op |= ARM_SPE_OP_BR_CR_NON_BL_RET;
>   				break;
>   			default:
>   				pr_err("Get packet error!\n");
> @@ -238,6 +250,12 @@ static int arm_spe_read_record(struct arm_spe_decoder *decoder)
>   			if (payload & BIT(EV_MISPRED))
>   				decoder->record.type |= ARM_SPE_BRANCH_MISS;
>   
> +			if (payload & BIT(EV_NOT_TAKEN))
> +				decoder->record.type |= ARM_SPE_BRANCH_NOT_TAKEN;
> +
> +			if (payload & BIT(EV_TRANSACTIONAL))
> +				decoder->record.type |= ARM_SPE_IN_TXN;
> +
>   			if (payload & BIT(EV_PARTIAL_PREDICATE))
>   				decoder->record.type |= ARM_SPE_SVE_PARTIAL_PRED;
>   
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
> index 4bcd627e859f..85b688a97436 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
> @@ -24,6 +24,8 @@ enum arm_spe_sample_type {
>   	ARM_SPE_REMOTE_ACCESS		= 1 << 7,
>   	ARM_SPE_SVE_PARTIAL_PRED	= 1 << 8,
>   	ARM_SPE_SVE_EMPTY_PRED		= 1 << 9,
> +	ARM_SPE_BRANCH_NOT_TAKEN	= 1 << 10,
> +	ARM_SPE_IN_TXN			= 1 << 11,
>   };
>   
>   enum arm_spe_op_type {
> @@ -52,8 +54,12 @@ enum arm_spe_op_type {
>   	ARM_SPE_OP_SVE_SG		= 1 << 27,
>   
>   	/* Second level operation type for BRANCH_ERET */
> -	ARM_SPE_OP_BR_COND	= 1 << 16,
> -	ARM_SPE_OP_BR_INDIRECT	= 1 << 17,
> +	ARM_SPE_OP_BR_COND		= 1 << 16,
> +	ARM_SPE_OP_BR_INDIRECT		= 1 << 17,
> +	ARM_SPE_OP_BR_GCS		= 1 << 18,
> +	ARM_SPE_OP_BR_CR_BL		= 1 << 19,
> +	ARM_SPE_OP_BR_CR_RET		= 1 << 20,
> +	ARM_SPE_OP_BR_CR_NON_BL_RET	= 1 << 21,
>   };
>   
>   enum arm_spe_common_data_source {