Re: Some questions about using the perf tool in ARM-SPE

[James Clark <james.clark@arm.com>]

On 03/07/2023 10:39, Leo Yan wrote:
> Hi Suzuki, James,
> 
> On Mon, Jul 03, 2023 at 09:24:27AM +0100, James Clark wrote:
> 
> [...]
> 
>>>>> I have a new discovery that c2c seems to support only certain Arm
>>>>> Neoverse (N1/N2/V1) CPUs, I wonder if cortex-X4 could support it?
>>>>
>>>> Based on Cortex-X4 TRM [1], we can see Cortex-X4 has the same SPE data
>>>> source packet format with Neoverse CPUs.� In theory, we can add
>>>> Cortex-X4's MIDR into the neoverse_spe[] array in
>>>> tools/perf/util/arm-spe.c to support Cortex-X4.
>>>>
>>>> Linux master branch misses the definition for Cortex-X4's MIDR [2],
>>>> Mark.R / Suzuki / James, could you confirm if Arm has plan or already
>>>> has patches for enabling Cortex-X4's MIDR?
>>>
>>> Is there a particular reason why need this in the kernel ? Usual
>>> policy is, kernel needs to distinguish an MIDR, only if it ever
>>> needs to. e.g, a CPU erratum specific to the MIDR.
> 
> Just clarify a bit.  Since Arm SPE data source packet is
> implementation dependent, which means different CPU types can use
> different data source formats, or some CPU variants don't support data
> source packet at all; for this reason, perf tool checks CPU MIDR to
> decide if data source format can be supported or not.
> 
> Now perf tool directly includes kernel header
> arch/arm64/include/asm/cputype.h for CPU MIDR definitions, which is
> why I am asking if any one is working on adding MIDR for Cortex-X4.
> 
>> This is on the tool side rather than in the kernel. But yes, if the data
>> source encoding is the same as the existing ones please send a patch
>> adding X4's MIDR to the list.
> 
> I can do this; as elaborated above, I think we need two patches, one is
> a kernel patch for adding MIDR and another patch is for perf tool to
> consume the MIDR of X4.  I would like to know if this is expected or
> not from your side.
> 

Personally I would just add it on the tool side only. If it's not really
needed in the kernel then it doesn't make sense to add it.

But I suppose from a consistency point of view we could add it in both
places. I'm not too fussed either way.

> Thanks,
> Leo