Re: [PATCH] perf arm-spe: Add support for SPE Data Source packet on AmpereOne

[Ilkka Koskinen]

Hi James,

On Fri, 25 Oct 2024, James Clark wrote:
> On 25/10/2024 12:30 am, Ilkka Koskinen wrote:
> > Decode SPE Data Source packets on AmpereOne. The field is IMPDEF.
> >
> > Signed-off-by: Ilkka Koskinen <ilkka@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >   .../util/arm-spe-decoder/arm-spe-decoder.h    |  9 +++
> >   tools/perf/util/arm-spe.c                     | 61 +++++++++++++++++++
> >   2 files changed, 70 insertions(+)
> >
> > 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 1443c28545a9..e4115b1e92b2 100644
> > --- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
> > +++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
> > @@ -67,6 +67,15 @@ enum arm_spe_neoverse_data_source {
> >   	ARM_SPE_NV_DRAM		 = 0xe,
> >   };
> >   +enum arm_spe_ampereone_data_source {
> > +	ARM_SPE_AMPEREONE_LOCAL_CHIP_CACHE_OR_DEVICE	= 0x0,
> > +	ARM_SPE_AMPEREONE_SLC				= 0x3,
> > +	ARM_SPE_AMPEREONE_REMOTE_CHIP_CACHE		= 0x5,
> > +	ARM_SPE_AMPEREONE_DDR				= 0x7,
> > +	ARM_SPE_AMPEREONE_L1D				= 0x8,
> > +	ARM_SPE_AMPEREONE_L2D				= 0x9,
> > +};
> > +
> >   struct arm_spe_record {
> >   	enum arm_spe_sample_type type;
> >   	int err;
> > diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
> > index 138ffc71b32d..04bd21ad7ea8 100644
> > --- a/tools/perf/util/arm-spe.c
> > +++ b/tools/perf/util/arm-spe.c
> > @@ -515,10 +515,69 @@ static void arm_spe__synth_data_source_generic(const 
> > struct arm_spe_record *reco
> >   		data_src->mem_lvl |= PERF_MEM_LVL_REM_CCE1;
> >   }
> >   +static const struct midr_range ampereone_source_spe[] = {
> > +	MIDR_ALL_VERSIONS(MIDR_AMPERE1A),
> > +	{},
> > +};
> > +
> > +static void arm_spe__synth_data_source_ampereone(const struct 
> > arm_spe_record *record,
> > +						 union perf_mem_data_src 
> > *data_src,
> > +						 u64 midr)
> > +{
> > +	if (!is_midr_in_range_list(midr, ampereone_source_spe)) {
> > +		arm_spe__synth_data_source_generic(record, data_src);
> > +		return;
> > +	}
> [...]
> >   static u64 arm_spe__synth_data_source(const struct arm_spe_record 
> > *record, u64 midr)
> >   {
> >   	union perf_mem_data_src	data_src = { .mem_op = PERF_MEM_OP_NA };
> >   	bool is_neoverse = is_midr_in_range_list(midr, neoverse_spe);
> > +	bool is_ampereone = (read_cpuid_implementor() == ARM_CPU_IMP_AMPERE);
>
> Hi Ilkka,
>
> I think this read_cpuid_implementor() is for the device that's running 
> report, rather than record. You need to use the midr that's saved into the 
> file.

Uh, that didn't come to my mind at all.

> But it looks like you've done that for is_midr_in_range_list(midr, 
> ampereone_source_spe) above. Is it possible to just do that and not 
> read_cpuid_implementor() and then it's done the same way as neoverse and also 
> works off target?

You're right, it's wrong and there's no even need for separate implementor 
id check. I fix it.

Cheers, Ilkka

> Thanks
>
> James