Re: [PATCH 3/4] perf arm_spe: Decode Arm N1 IMPDEF events

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

On 07/04/2026 7:26 pm, Ian Rogers wrote:
> On Tue, Apr 7, 2026 at 5:35 AM James Clark <james.clark@linaro.org> wrote:
>>
>>
>>
>> On 02/04/2026 4:26 pm, Ian Rogers wrote:
>>> On Wed, Apr 1, 2026 at 7:26 AM James Clark <james.clark@linaro.org> wrote:
>>>>
>>>>   From the TRM [1], N1 has one IMPDEF event which isn't covered by the
>>>> common list. Add a framework so that more cores can be added in the
>>>> future and that the N1 IMPDEF event can be decoded. Also increase the
>>>> size of the buffer because we're adding more strings and if it gets
>>>> truncated it falls back to a hex dump only.
>>>>
>>>> [1]: https://developer.arm.com/documentation/100616/0401/Statistical-Profiling-Extension/implementation-defined-features-of-SPE
>>>> Suggested-by: Al Grant <al.grant@arm.com>
>>>> Signed-off-by: James Clark <james.clark@linaro.org>
>>>> ---
>>>>    tools/perf/util/arm-spe-decoder/Build              |  2 +
>>>>    .../util/arm-spe-decoder/arm-spe-pkt-decoder.c     | 45 ++++++++++++++++++++--
>>>>    .../util/arm-spe-decoder/arm-spe-pkt-decoder.h     |  5 ++-
>>>>    tools/perf/util/arm-spe.c                          | 13 ++++---
>>>>    4 files changed, 54 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/tools/perf/util/arm-spe-decoder/Build b/tools/perf/util/arm-spe-decoder/Build
>>>> index ab500e0efe24..97a298d1e279 100644
>>>> --- a/tools/perf/util/arm-spe-decoder/Build
>>>> +++ b/tools/perf/util/arm-spe-decoder/Build
>>>> @@ -1 +1,3 @@
>>>>    perf-util-y += arm-spe-pkt-decoder.o arm-spe-decoder.o
>>>> +
>>>> +CFLAGS_arm-spe-pkt-decoder.o += -I$(srctree)/tools/arch/arm64/include/ -I$(OUTPUT)arch/arm64/include/generated/
>>>> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
>>>> index c880b0dec3a1..42a7501d4dfe 100644
>>>> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
>>>> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
>>>> @@ -15,6 +15,8 @@
>>>>
>>>>    #include "arm-spe-pkt-decoder.h"
>>>>
>>>> +#include "../../arm64/include/asm/cputype.h"
>>>
>>> Sashiko spotted:
>>> https://sashiko.dev/#/patchset/20260401-james-spe-impdef-decode-v1-0-ad0d372c220c%40linaro.org
>>> """
>>> This isn't a bug, but does this include directive rely on accidental
>>> path normalization?
>>>
>>> The relative path ../../arm64/include/asm/cputype.h does not exist relative
>>> to arm-spe-pkt-decoder.c. It only compiles because the Build file adds
>>> -I$(srctree)/tools/arch/arm64/include/ to CFLAGS.
>>>
>>> Would it be cleaner to use #include <asm/cputype.h> to explicitly rely on
>>> the include path?
>>> [ ... ]
>>> """
>>> I wouldn't use <asm/cputype.h> due to cross-compilation and the like,
>>> instead just add the extra "../" into the include path.
>>>
>>
>> Do you mean change the #include to this?
>>
>>     #include "../../../arm64/include/asm/cputype.h"
>>
>> I still need to add:
>>
>>     CFLAGS_arm-spe-pkt-decoder.o += -I$(srctree)/tools/arch/arm64/include/
>>
>> To make the this include in cputype.h work:
>>
>>     #include <asm/sysreg.h>
>>
>> Which probably only works because there isn't a sysreg.h on other
>> architectures. But I'm not sure what the significance of ../../ vs
>> ../../../ is if either compile? arm-spe.c already does it with ../../
>> which is what I copied.
> 
> Hmm.. maybe the path should be
> "../../../arch/arm64/include/asm/cputype.h". The include preference is
> for a path relative to the source file and
> ../../arm64/include/asm/cputype.h doesn't exist. It is kind of horrid

Up 3 dirs from arm-spe-pkt-decoder.c would be 
"tools/arm64/include/asm/cputype.h" which doesn't exist either unless 
I'm missing something?

If get the compiler to print the path it uses with 3 then it would use 
the x86 uapi include path which doesn't seem any less weird to me:

  ...
  In file included from util/arm-spe-decoder/arm-spe-pkt-decoder.c:19:

  linux/tools/arch/x86/include/uapi/../../../arm64/include/asm/cputype.h:254:10:


> to add an include path and then use a relative path to escape into a
> higher-level directory. arm-spe.c is a little different as it is one
> directory higher in the directory layout.
> 

It is one folder higher, but arm-spe.c still relies on adding a special 
include path to CFLAGS_arm-spe.o to make it work. It's not including a 
real path relative to the source either.

Yeah it's a bit horrible but I don't think the asm/ thing combined with 
copying headers from the kernel to tools expected to handle the case 
where we would want to use asm/ stuff for a different arch than the 
compile one. It might not be normal to use relative include paths to 
escape to higher directories, but it's not a normal situation either. I 
think it's a special case for a special scenario. I'm not sure of a 
better way, but this is working for now.

> Thanks,
> Ian
> 
>>>> +
>>>>    static const char * const arm_spe_packet_name[] = {
>>>>           [ARM_SPE_PAD]           = "PAD",
>>>>           [ARM_SPE_END]           = "END",
>>>> @@ -307,6 +309,11 @@ static const struct ev_string common_ev_strings[] = {
>>>>           { .event = 0, .desc = NULL },
>>>>    };
>>>>
>>>> +static const struct ev_string n1_event_strings[] = {
>>>> +       { .event = 12, .desc = "LATE-PREFETCH" },
>>>> +       { .event = 0, .desc = NULL },
>>>> +};
>>>> +
>>>>    static u64 print_event_list(int *err, char **buf, size_t *buf_len,
>>>>                               const struct ev_string *ev_strings, u64 payload)
>>>>    {
>>>> @@ -318,14 +325,44 @@ static u64 print_event_list(int *err, char **buf, size_t *buf_len,
>>>>           return payload;
>>>>    }
>>>>
>>>> +struct event_print_handle {
>>>> +       const struct midr_range *midr_ranges;
>>>> +       const struct ev_string *ev_strings;
>>>> +};
>>>> +
>>>> +#define EV_PRINT(range, strings)                       \
>>>> +       {                                       \
>>>> +               .midr_ranges = range,           \
>>>> +               .ev_strings = strings,  \
>>>> +       }
>>>> +
>>>> +static const struct midr_range n1_event_encoding_cpus[] = {
>>>> +       MIDR_ALL_VERSIONS(MIDR_NEOVERSE_N1),
>>>> +       {},
>>>> +};
>>>> +
>>>> +static const struct event_print_handle event_print_handles[] = {
>>>> +       EV_PRINT(n1_event_encoding_cpus, n1_event_strings),
>>>> +};
>>>> +
>>>>    static int arm_spe_pkt_desc_event(const struct arm_spe_pkt *packet,
>>>> -                                 char *buf, size_t buf_len)
>>>> +                                 char *buf, size_t buf_len, u64 midr)
>>>>    {
>>>>           u64 payload = packet->payload;
>>>>           int err = 0;
>>>>
>>>>           arm_spe_pkt_out_string(&err, &buf, &buf_len, "EV");
>>>> -       print_event_list(&err, &buf, &buf_len, common_ev_strings, payload);
>>>> +       payload = print_event_list(&err, &buf, &buf_len, common_ev_strings,
>>>> +                                  payload);
>>>> +
>>>> +       /* Try to decode IMPDEF bits for known CPUs */
>>>> +       for (unsigned int i = 0; i < ARRAY_SIZE(event_print_handles); i++) {
>>>> +               if (is_midr_in_range_list(midr,
>>>> +                                         event_print_handles[i].midr_ranges))
>>>> +                       payload = print_event_list(&err, &buf, &buf_len,
>>>> +                                                  event_print_handles[i].ev_strings,
>>>> +                                                  payload);
>>>> +       }
>>>>
>>>>           return err;
>>>>    }
>>>> @@ -506,7 +543,7 @@ static int arm_spe_pkt_desc_counter(const struct arm_spe_pkt *packet,
>>>>    }
>>>>
>>>>    int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>>>> -                    size_t buf_len)
>>>> +                    size_t buf_len, u64 midr)
>>>>    {
>>>>           int idx = packet->index;
>>>>           unsigned long long payload = packet->payload;
>>>> @@ -522,7 +559,7 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>>>>                   arm_spe_pkt_out_string(&err, &buf, &blen, "%s", name);
>>>>                   break;
>>>>           case ARM_SPE_EVENTS:
>>>> -               err = arm_spe_pkt_desc_event(packet, buf, buf_len);
>>>> +               err = arm_spe_pkt_desc_event(packet, buf, buf_len, midr);
>>>>                   break;
>>>>           case ARM_SPE_OP_TYPE:
>>>>                   err = arm_spe_pkt_desc_op_type(packet, buf, buf_len);
>>>> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
>>>> index adf4cde320aa..17b067fe3c87 100644
>>>> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
>>>> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
>>>> @@ -11,7 +11,7 @@
>>>>    #include <stddef.h>
>>>>    #include <stdint.h>
>>>>
>>>> -#define ARM_SPE_PKT_DESC_MAX           256
>>>> +#define ARM_SPE_PKT_DESC_MAX           512
>>>>
>>>>    #define ARM_SPE_NEED_MORE_BYTES                -1
>>>>    #define ARM_SPE_BAD_PACKET             -2
>>>> @@ -186,5 +186,6 @@ const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
>>>>    int arm_spe_get_packet(const unsigned char *buf, size_t len,
>>>>                          struct arm_spe_pkt *packet);
>>>>
>>>> -int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf, size_t len);
>>>> +int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf, size_t len,
>>>> +                    u64 midr);
>>>>    #endif
>>>> diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
>>>> index 7447b000f9cd..46f0309c092b 100644
>>>> --- a/tools/perf/util/arm-spe.c
>>>> +++ b/tools/perf/util/arm-spe.c
>>>> @@ -135,7 +135,7 @@ struct data_source_handle {
>>>>           }
>>>>
>>>>    static void arm_spe_dump(struct arm_spe *spe __maybe_unused,
>>>> -                        unsigned char *buf, size_t len)
>>>> +                        unsigned char *buf, size_t len, u64 midr)
>>>>    {
>>>>           struct arm_spe_pkt packet;
>>>>           size_t pos = 0;
>>>> @@ -161,7 +161,7 @@ static void arm_spe_dump(struct arm_spe *spe __maybe_unused,
>>>>                           color_fprintf(stdout, color, "   ");
>>>>                   if (ret > 0) {
>>>>                           ret = arm_spe_pkt_desc(&packet, desc,
>>>> -                                              ARM_SPE_PKT_DESC_MAX);
>>>> +                                              ARM_SPE_PKT_DESC_MAX, midr);
>>>>                           if (!ret)
>>>>                                   color_fprintf(stdout, color, " %s\n", desc);
>>>>                   } else {
>>>> @@ -174,10 +174,10 @@ static void arm_spe_dump(struct arm_spe *spe __maybe_unused,
>>>>    }
>>>>
>>>>    static void arm_spe_dump_event(struct arm_spe *spe, unsigned char *buf,
>>>> -                              size_t len)
>>>> +                              size_t len, u64 midr)
>>>>    {
>>>>           printf(".\n");
>>>> -       arm_spe_dump(spe, buf, len);
>>>> +       arm_spe_dump(spe, buf, len, midr);
>>>>    }
>>>>
>>>>    static int arm_spe_get_trace(struct arm_spe_buffer *b, void *data)
>>>> @@ -1469,8 +1469,11 @@ static int arm_spe_process_auxtrace_event(struct perf_session *session,
>>>>                   /* Dump here now we have copied a piped trace out of the pipe */
>>>>                   if (dump_trace) {
>>>>                           if (auxtrace_buffer__get_data(buffer, fd)) {
>>>> +                               u64 midr = 0;
>>>> +
>>>> +                               arm_spe__get_midr(spe, buffer->cpu.cpu, &midr);
>>>
>>> Sashiko claims to have spotted an issue here:
>>> """
>>> Is it possible for arm_spe__get_midr() to cause a segmentation fault here?
>>>
>>> If the trace is from an older recording (metadata version 1) and the
>>> environment lacks a CPUID string (such as during cross-architecture
>>> analysis), perf_env__cpuid() returns NULL.
>>>
>>> It appears arm_spe__get_midr() then passes this NULL pointer to
>>> strtol(cpuid, NULL, 16), which leads to undefined behavior.
>>> """
>>>
>>> But this feels like, if this happens you're already having a bad time
>>> and these changes aren't necessarily making things worse.
>>>
>>> Thanks,
>>> Ian
>>>
>>
>> Yeah I think it might be possible so I can add an error instead of a
>> segfault. I'll check the rest of the Sashiko comments too.
>>
>>>>                                   arm_spe_dump_event(spe, buffer->data,
>>>> -                                               buffer->size);
>>>> +                                               buffer->size, midr);
>>>>                                   auxtrace_buffer__put_data(buffer);
>>>>                           }
>>>>                   }
>>>>
>>>> --
>>>> 2.34.1
>>>>
>>