Re: [PATCH V3 6/6] tools/perf: Add perf tool support for processing powerpc HTM AUXTRACE records

[Athira Rajeev <atrajeev@linux.ibm.com>]

> On 25 Jul 2026, at 12:52 PM, sashiko-bot@kernel.org wrote:
> 
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [Medium] The implementation is missing the HTM_MAX_SAFE_TARGETS (1024) cap for num_events that was explicitly promised in the commit message.
> - [Medium] htm_config_for_cpu conflates a lookup failure with a valid 0 PMU configuration, causing valid auxtrace events to be dropped.
> - [Low] Assigning write() return value to a size_t variable masks the actual error code and produces misleading log messages.
> - [Low] Missing explicit inclusion of POSIX standard headers required for musl libc compatibility.
> --
> 
> --- Patch [6]: [PATCH V3 6/6] tools/perf: Add perf tool support for processing powerpc HTM AUXTRACE records ---
> commit b319ae58a675d493de8d8d9853fc6d781771f6b8
> Author: Athira Rajeev <atrajeev@linux.ibm.com>
> 
> tools/perf: Add perf tool support for processing powerpc HTM AUXTRACE records
> 
> powerpc_htm_process_auxtrace_info() reads the (cpu, attr.config) pairs
> stored by htm_info_fill() at record time and builds a cpu_configs[]
> table.  This lets process_auxtrace_event() map each AUX buffer, which
> carries event->auxtrace.cpu, to the correct (node, chip, core) target
> and write the raw trace data to htm.bin.nX.pX.cX immediately.
> 
> [ ... ]
>> - Cap num_events at HTM_MAX_SAFE_TARGETS (1024) to prevent size_t
>> truncation on 32-bit platforms when passed to calloc().
> 
> [Severity: Medium]
> This isn't a bug, but the commit message mentions capping num_events at 1024
> (HTM_MAX_SAFE_TARGETS).
> 
> Looking at powerpc_htm_process_auxtrace_info(), the arrays are allocated
> using num_events directly:
> 
> tools/perf/util/powerpc-htm.c:powerpc_htm_process_auxtrace_info() {
>    ...
>    htm->nr_targets   = (int)num_events;
>    htm->cpu_configs  = calloc((size_t)num_events, sizeof(*htm->cpu_configs));
>    ...
> }
> 
> Has the 1024 maximum cap and truncation prevention logic been omitted here?
Thanks for all the comments

I will update commit message
> 
>> diff --git a/tools/perf/util/powerpc-htm.c b/tools/perf/util/powerpc-htm.c
>> index 952ed32997d9a..1d1b4302e2e4a 100644
>> --- a/tools/perf/util/powerpc-htm.c
>> +++ b/tools/perf/util/powerpc-htm.c
>> @@ -12,9 +12,14 @@
>> #include "util/session.h"
>> #include "color.h"
>> #include "powerpc-htm.h"
>> +#include <errno.h>
>> #include "debug.h"
>> #include "sample.h"
>> 
>> +#include <linux/unaligned.h>
>> +
>> +struct perf_session;
> 
> [Severity: Low]
> Are the standard POSIX headers missing here?
> 
> The code uses functions like open(), write(), lseek(), snprintf(), strerror(),
> calloc(), and free().
> 
> Without explicitly including fcntl.h, unistd.h, stdio.h, string.h, and
> stdlib.h, could this fail to compile on systems using musl libc due to
> relying on implicit header inclusions?
These are added patch 5 already
> 
> [ ... ]
>> +/*
>> + * Look up attr.config by the CPU number carried in event->auxtrace.cpu.
>> + * Returns 0 if not found.
>> + */
>> +static u64 htm_config_for_cpu(struct powerpc_htm *htm, int cpu)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < htm->nr_cpu_configs; i++) {
>> + if (htm->cpu_configs[i].cpu == cpu)
>> + return htm->cpu_configs[i].config;
>> + }
>> + return 0;
>> +}
> 
> [Severity: Medium]
> Can a valid configuration evaluate to 0?
> 
> If the HTM PMU is configured with node=0, chip=0, core=0 and no other flags,
> attr.config could legitimately be 0.
> 
> If it returns 0, the caller powerpc_htm_process_auxtrace_event() checks:
> 
> tools/perf/util/powerpc-htm.c:powerpc_htm_process_auxtrace_event() {
>    ...
>    ev_config = htm_config_for_cpu(htm, (int)event->auxtrace.cpu);
>    if (!ev_config) {
>        ...
>        return 0;
>    }
>    ...
> }
> 
> Would this cause valid auxtrace events to be dropped by conflating a lookup
> failure with a valid 0 PMU configuration?
Will fix this logic in V4

Thanks
Athira
> 
> [ ... ]
>> +static int write_htm(struct powerpc_htm *htm, void *data, size_t size,
>> +     u32 node, u32 chip, u32 core, int mem_maps)
>> +{
>> + u32 target_key = htm_pack_target(node, chip, core);
>> + char target_file[128];
>> + size_t written;
>> + int flags;
>> + int fd;
> [ ... ]
>> + written = write(fd, data, payload);
>> + close(fd);
>> +
>> + if (written != payload) {
>> + pr_err("Failed to write memory config: expected %zu bytes, wrote %zu\n",
>> +       payload, written);
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> + }
>> +
>> + /* AUX bus-trace path */
>> + snprintf(target_file, sizeof(target_file),
>> + "htm.bin.n%d.p%d.c%d", node, chip, core);
>> + flags |= htm_target_seen(htm->htm_bin_targets,
>> + &htm->nr_htm_bin_targets,
>> + htm->nr_targets,
>> + target_key) ? O_APPEND : O_TRUNC;
>> + fd = open(target_file, flags, 0644);
>> + if (fd == -1) {
>> + pr_err("Failed to open %s: %s\n", target_file, strerror(errno));
>> + return -errno;
>> + }
>> +
>> + written = write(fd, data, size);
>> + close(fd);
>> +
>> + if (written != size) {
>> + pr_err("Failed to write htm trace data: expected %zu bytes, wrote %zu\n",
>> +       size, written);
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> +}
> 
> [Severity: Low]
> Does this code assign the signed return value of write() to a size_t variable?
> 
> If write() fails and returns -1, it will be assigned to size_t written,
> wrapping it to SIZE_MAX.
> 
> While the error is still caught by the written != payload and written != size
> checks, will this result in logging an impossibly large value for bytes written
> and mask the actual error code?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260725070747.81435-1-atrajeev@linux.ibm.com?part=6