Re: [PATCH V4 3/6] powerpc/perf: Add AUX buffer management to capture HTM trace data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Nested calls to `perf_aux_output_begin` from NMI context corrupt the outer context's shared per-CPU `struct perf_output_handle`.
- [High] `htm_dump_sample_data()` permanently stops the hardware trace buffer during a read without restarting it, breaking continuous streaming.
- [High] Applying `PAGE_MASK` to `chunk_size` zeroes the size when available ring buffer space is under `PAGE_SIZE`, causing a permanent stall and 100% CPU hog in userspace.
--

--- Patch [3]: [PATCH V4 3/6] powerpc/perf: Add AUX buffer management to capture HTM trace data ---
commit 8304e887ab454be629979799f609224f8a64f9ec
Author: Athira Rajeev <atrajeev@linux.ibm.com>

powerpc/perf: Add AUX buffer management to capture HTM trace data

This patch implements support for auxiliary (AUX) ring buffers in the HTM
PMU driver. It introduces core lifecycle hooks and data structures to stream
trace data into perf AUX buffers.

>     htm_pmu_ctx embeds a struct perf_output_handle as a per-CPU variable,
>     following the established upstream pattern.  Reentrancy is handled by
>     perf_aux_output_begin() itself via rb->aux_nest: when a nested caller
>     (e.g. from NMI context) detects that aux_nest is already non-zero, it
>     falls through to the err path, which sets the *nested* handle->event to
>     NULL and returns NULL to the nested caller.  The outer handle is never
>     touched; its handle->event remains valid throughout the outer
>     perf_aux_output_end() call.

[Severity: High]
Is this description accurate regarding the outer handle?

Since htm_pmu_ctx is defined as a static per-CPU variable, the nested context
uses the exact same perf_output_handle object as the outer context. When the
nested perf_aux_output_begin() call hits the error path, it clears handle->event
on that shared object.

Would this clear the event pointer of the interrupted outer transaction, causing
the outer perf_aux_output_end() to return early without decrementing rb->aux_nest?

> diff --git a/arch/powerpc/perf/htm-perf.c b/arch/powerpc/perf/htm-perf.c
> index 1879125bc4102..0b954a7ed4dd9 100644
> --- a/arch/powerpc/perf/htm-perf.c
> +++ b/arch/powerpc/perf/htm-perf.c

[ ... ]

> @@ -95,6 +95,22 @@ static inline void parse_htm_config(u64 config, struct htm_config *cfg)
>  	cfg->coreindexonchip = (config >> 20) & 0xff;
>  }
>  
> +struct htm_pmu_buf {
> +	int     nr_pages;
> +	bool    snapshot;
> +	void    *base;
> +	void	**pages;
> +	u64	head;
> +	u64     size;
> +	int     collect_htm_trace;
> +};
> +
> +struct htm_pmu_ctx {
> +	struct perf_output_handle handle;
> +};
> +
> +static DEFINE_PER_CPU(struct htm_pmu_ctx, htm_pmu_ctx);

[Severity: High]
Is it safe to share a single perf_output_handle per-CPU?

If an NMI interrupts an active AUX transaction on the same CPU, the nested
execution passes this exact same handle pointer to perf_aux_output_begin().
Could we define the handle as a stack variable in htm_dump_sample_data() and
htm_event_start() instead to prevent reentrancy corruption?

[ ... ]

> @@ -465,8 +504,254 @@ static void htm_event_del(struct perf_event *event, int flags)
>  	/* pmu_private freed by event->destroy = reset_htm_active */
>  }
>  
> +static ssize_t htm_dump_sample_data(struct perf_event *event)
> +{
> +	struct htm_pmu_ctx *htm_ctx = this_cpu_ptr(&htm_pmu_ctx);

[ ... ]

> +	if (target->tracing_active == HTM_TRACING_ACTIVE) {
> +		htm_event_stop(event, 0);
> +		if (target->tracing_active == HTM_TRACING_ACTIVE) {
> +			/*
> +			 * H_HTM_OP_STOP failed.
> +			 * Cannot dump data while the trace is still running.
> +			 * Return -EIO to signal a non-retriable hardware failure;
> +			 * htm_event_read() will set event->count=0 and the drain
> +			 * loop will stop cleanly.
> +			 */
> +			perf_aux_output_end(&htm_ctx->handle, 0);
> +			return -EIO;
> +		}
> +	}

[Severity: High]
Does this permanently halt tracing when data is read?

When htm_dump_sample_data() is called (via pmu->read), it calls htm_event_stop()
to halt the hardware trace before dumping. After the dump finishes, the trace
is never restarted. Will this cause the hardware trace to silently stop
collecting further data the first time userspace reads from the event?

[ ... ]

> +	/*
> +	 * Assess constraints regarding space remaining across the mapping
> +	 * context boundary
> +	 */
> +	chunk_size = htm_ctx->handle.size;
> +	chunk_size &= PAGE_MASK;

[Severity: High]
Can this truncation cause a permanent stall in userspace?

If the available free space (handle.size) is less than a full page, this applies
PAGE_MASK and truncates chunk_size to 0.

Later in this function:

	if (!chunk_size) {
		/* ... */
		perf_aux_output_end(&htm_ctx->handle, 0);
		return -ENOSPC;
	}

Returning -ENOSPC sets event->count = 1, signaling userspace to drain the
buffer and retry. However, because 0 bytes were written, userspace has nothing
new to drain, the free space remains < PAGE_SIZE, and the next read will again
truncate to 0 and return -ENOSPC. Does this create an infinite retry loop
consuming 100% CPU?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729123752.63010-1-atrajeev@linux.ibm.com?part=3