Re: [PATCH] perf arm-spe: Use SPE data source for neoverse cores

[James Clark]

On 21/01/2022 18:24, Ali Saidi wrote:
> When synthesizing data from SPE, augment the type with source information
> for Arm Neoverse cores. The field is IMPLDEF but the Neoverse cores all use
> the same encoding. I can't find encoding information for any other SPE
> implementations to unify their choices with Arm's thus that is left for future
> work.
> 
> This changes enables the expected behavior of perf c2c on a system with SPE where
> lines that are shared among multiple cores show up in perf c2c output. 
> 
> Signed-off-by: Ali Saidi <alisaidi@xxxxxxxxxx> 
> ---
>  .../util/arm-spe-decoder/arm-spe-decoder.c    |  1 +
>  .../util/arm-spe-decoder/arm-spe-decoder.h    | 12 +++++
>  tools/perf/util/arm-spe.c                     | 48 ++++++++++++++-----
>  3 files changed, 49 insertions(+), 12 deletions(-)
> 
[...]
> +static u64 arm_spe__synth_data_source(const struct arm_spe_record *record, u64 midr)
>  {
>  	union perf_mem_data_src	data_src = { 0 };
> +	bool is_neoverse = is_midr_in_range(midr, neoverse_spe);
>  
>  	if (record->op == ARM_SPE_LD)
>  		data_src.mem_op = PERF_MEM_OP_LOAD;
> @@ -409,19 +418,30 @@ static u64 arm_spe__synth_data_source(const struct arm_spe_record *record)
>  		data_src.mem_op = PERF_MEM_OP_STORE;
>  
>  	if (record->type & (ARM_SPE_LLC_ACCESS | ARM_SPE_LLC_MISS)) {
> -		data_src.mem_lvl = PERF_MEM_LVL_L3;
> +		if (is_neoverse && record->source == ARM_SPE_NV_DRAM) {
> +			data_src.mem_lvl = PERF_MEM_LVL_LOC_RAM | PERF_MEM_LVL_HIT;
> +		} else if (is_neoverse && record->source == ARM_SPE_NV_PEER_CLSTR) {
> +			data_src.mem_snoop = PERF_MEM_SNOOP_HITM;

I'm not following how LLC_ACCESS | LLC_MISS ends up as HITM in this case (ARM_SPE_NV_PEER_CLSTR)?
I thought there was no way to determine a HITM from SPE. Wouldn't one of the other values
like PERF_MEM_SNOOP_MISS be more accurate?

> +			data_src.mem_lvl = PERF_MEM_LVL_L3 | PERF_MEM_LVL_HIT;

This one also adds PERF_MEM_LVL_HIT even though the check of "if (record->type & ARM_SPE_LLC_MISS)"
hasn't happened yet. Maybe some comments would make it a bit clearer, but at the moment it's
not obvious how the result is derived because there are some things that don't add up like
ARM_SPE_LLC_MISS == PERF_MEM_LVL_HIT.

> +		} else {
> +			data_src.mem_lvl = PERF_MEM_LVL_L3;>  
> -		if (record->type & ARM_SPE_LLC_MISS)
> -			data_src.mem_lvl |= PERF_MEM_LVL_MISS;
> -		else
> -			data_src.mem_lvl |= PERF_MEM_LVL_HIT;
> +			if (record->type & ARM_SPE_LLC_MISS)
> +				data_src.mem_lvl |= PERF_MEM_LVL_MISS;
> +			else
> +				data_src.mem_lvl |= PERF_MEM_LVL_HIT;
> +		}
>  	} else if (record->type & (ARM_SPE_L1D_ACCESS | ARM_SPE_L1D_MISS)) {
> -		data_src.mem_lvl = PERF_MEM_LVL_L1;
> +		if (is_neoverse && record->source == ARM_SPE_NV_L2) {
> +			data_src.mem_lvl = PERF_MEM_LVL_L2 | PERF_MEM_LVL_HIT;
> +		} else {
> +			data_src.mem_lvl = PERF_MEM_LVL_L1;
>  
> -		if (record->type & ARM_SPE_L1D_MISS)
> -			data_src.mem_lvl |= PERF_MEM_LVL_MISS;
> -		else
> -			data_src.mem_lvl |= PERF_MEM_LVL_HIT;
> +			if (record->type & ARM_SPE_L1D_MISS)
> +				data_src.mem_lvl |= PERF_MEM_LVL_MISS;
> +			else
> +				data_src.mem_lvl |= PERF_MEM_LVL_HIT;
> +		}
>  	}
>  
>  	if (record->type & ARM_SPE_REMOTE_ACCESS)
> @@ -446,7 +466,7 @@ static int arm_spe_sample(struct arm_spe_queue *speq)
>  	u64 data_src;
>  	int err;
>  
> -	data_src = arm_spe__synth_data_source(record);
> +	data_src = arm_spe__synth_data_source(record, spe->midr);
>  
>  	if (spe->sample_flc) {
>  		if (record->type & ARM_SPE_L1D_MISS) {
> @@ -796,6 +816,10 @@ static int arm_spe_process_event(struct perf_session *session,
>  	u64 timestamp;
>  	struct arm_spe *spe = container_of(session->auxtrace,
>  			struct arm_spe, auxtrace);
> +	const char *cpuid = perf_env__cpuid(session->evlist->env);
> +	u64 midr = strtol(cpuid, NULL, 16);
> +
> +	spe->midr = midr;
>  
>  	if (dump_trace)
>  		return 0;
>