Re: [PATCH 3/3] perf: arm_spe: Add support for a 'software' discard mode

On 05/08/2026 16:58, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Writes to control-plane system registers (`SYS_PMBPTR_EL1` and `SYS_PMBLIMITR_EL1`) lack an immediate `isb()` synchronization barrier, violating ARM64 architectural invariants.
> - [Medium] User-triggerable allocations for the software discard buffer lack `__GFP_ACCOUNT`, allowing a user to bypass memory cgroup limits.
> --
> 
> --- Patch [3]: [PATCH 3/3] perf: arm_spe: Add support for a 'software' discard mode ---
> commit af3311701234ae1a3b87d9c00cb9f6f3748862b7
> Author: James Clark <[email protected]>
> 
> perf: arm_spe: Add support for a 'software' discard mode
> 
> This patch implements a software discard mode for ARM SPE. It falls back
> to writing to a single driver-allocated page and not emitting aux records
> when hardware discard support is unavailable.
> 
>> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
>> --- a/drivers/perf/arm_spe_pmu.c
>> +++ b/drivers/perf/arm_spe_pmu.c
> 
> [ ... ]
> 
>> +static int arm_spe_pmu_alloc_discard_buf(struct perf_event *event)
>> +{
>> +	int node = (event->cpu == -1) ? -1 : cpu_to_node(event->cpu);
>> +	int nr_pages = ARM_SPE_DISCARD_BUF_SIZE / PAGE_SIZE;
>> +	struct arm_spe_pmu_sw_discard_buf *buf;
>> +	struct page **pglist;
>> +	int i;
>> +
>> +	if (event->parent) {
> 
> [ ... ]
> 
>> +		return 0;
>> +	}
>> +
>> +	buf = kzalloc_node(sizeof(*buf), GFP_KERNEL, node);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	buf->page = alloc_pages_node(node, GFP_KERNEL, 0);
>> +	if (!buf->page)
>> +		goto out_free_buf;
>> +
>> +	pglist = kvmalloc_array(nr_pages, sizeof(*pglist), GFP_KERNEL);
> 
> [Severity: Medium]
> Should these GFP_KERNEL allocations include __GFP_ACCOUNT?
> 
> Since this memory is allocated on behalf of a user process creating an
> SPE event, omitting __GFP_ACCOUNT could allow a container to allocate
> kernel memory without being charged, potentially bypassing memory limits.
> 

I can't find any examples of __GFP_ACCOUNT being used in a similar way. 
Even the existing arm_spe_pmu_setup_aux() doesn't use it. So I'm not 
sure what the precedent is.

>> +	if (!pglist)
>> +		goto out_free_page;
> 
> [ ... ]
> 
>> @@ -617,10 +698,26 @@ static u64 arm_spe_pmu_next_off(struct perf_output_handle *handle)
>> -static void arm_spe_pmu_begin_discard(struct perf_output_handle *handle,
>> -				      struct perf_event *event)
>> +static void arm_spe_pmu_begin_discard(struct perf_output_handle *handle,
>> +				      struct perf_event *event)
>>   {
>> -	u64 limit;
>> +	struct arm_spe_pmu *spe_pmu = to_spe_pmu(event->pmu);
>> +	struct arm_spe_pmu_sw_discard_buf *discard_buf = event->pmu_private;
>> +	u64 base, limit;
>> +
>> +	if (spe_pmu->features & SPE_PMU_FEAT_HW_DISCARD) {
>> +		limit = FIELD_PREP(PMBLIMITR_EL1_FM, PMBLIMITR_EL1_FM_DISCARD);
>> +		limit |= PMBLIMITR_EL1_E;
>> +	} else {
>> +		base = (u64) discard_buf->base;
>> +		limit = base + ARM_SPE_DISCARD_BUF_SIZE;
>> +		limit |= PMBLIMITR_EL1_E;
>> +		write_sysreg_s(base, SYS_PMBPTR_EL1);
>> +
>> +		/*
>> +		 * Track the event so we can access the discard buffer through
>> +		 * event->pmu_private. perf_aux_output_begin() normally handles
>> +		 * this, but we can do it manually if that's not being used.
>> +		 */
>> +		handle->event = event;
> 
> [Severity: High]
> Does this write to SYS_PMBPTR_EL1 require an immediate isb() synchronization
> barrier?
> 
> Without it, the subsequent memory store (handle->event = event) might execute
> while the CPU pipeline is operating under an architecturally undefined state.
> 
>> +	}
>>   
>> -	limit = FIELD_PREP(PMBLIMITR_EL1_FM, PMBLIMITR_EL1_FM_DISCARD);
>> -	limit |= PMBLIMITR_EL1_E;
>> +	write_sysreg_s(limit, SYS_PMBLIMITR_EL1);
>> +}
> 
> [Severity: High]
> Is an isb() barrier also missing after this write to SYS_PMBLIMITR_EL1?
> 
> Because it is at the very end of arm_spe_pmu_begin_discard(), the function
> return and any subsequent conditional branches in the caller will execute
> before the pipeline state is synchronized.
> 

The register write orderings are identical to the old 
arm_spe_perf_aux_output_begin(). So this would be an existing issue if 
it even was an issue.