linux-kernel - Re: [PATCH 1/3] perf: arm_spe: Add barrier before enabling profiling buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b4bcb7cf-da03-4263-9101-feec6d0e0d8d@linaro.org>
Date: Mon, 8 Sep 2025 14:54:32 +0100
From: James Clark <james.clark@...aro.org>
To: Will Deacon <will@...nel.org>
Cc: Mark Rutland <mark.rutland@....com>,
 Catalin Marinas <catalin.marinas@....com>,
 Alexandru Elisei <Alexandru.Elisei@....com>,
 Anshuman Khandual <Anshuman.Khandual@....com>,
 Rob Herring <Rob.Herring@....com>, Suzuki Poulose <Suzuki.Poulose@....com>,
 Robin Murphy <Robin.Murphy@....com>, linux-arm-kernel@...ts.infradead.org,
 linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] perf: arm_spe: Add barrier before enabling profiling
 buffer



On 08/09/2025 2:41 pm, Will Deacon wrote:
> On Tue, Jul 01, 2025 at 04:31:57PM +0100, James Clark wrote:
>> DEN0154 states that PMBPTR_EL1 must not be modified while the profiling
>> buffer is enabled. Ensure that enabling the buffer comes after setting
>> PMBPTR_EL1 by inserting an isb().
>>
>> This only applies to guests for now, but in future versions of the
>> architecture the PE will be allowed to behave in the same way.
>>
>> Fixes: d5d9696b0380 ("drivers/perf: Add support for ARMv8.2 Statistical Profiling Extension")
>> Signed-off-by: James Clark <james.clark@...aro.org>
>> ---
>>   drivers/perf/arm_spe_pmu.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
>> index 3efed8839a4e..6235ca7ecd48 100644
>> --- a/drivers/perf/arm_spe_pmu.c
>> +++ b/drivers/perf/arm_spe_pmu.c
>> @@ -537,6 +537,7 @@ static void arm_spe_perf_aux_output_begin(struct perf_output_handle *handle,
>>   	limit += (u64)buf->base;
>>   	base = (u64)buf->base + PERF_IDX2OFF(handle->head, buf);
>>   	write_sysreg_s(base, SYS_PMBPTR_EL1);
>> +	isb();
> 
> 
> Hmm.
> 
> arm_spe_perf_aux_output_begin() is only called in two places:
> 
> 1. From arm_spe_pmu_start()
> 2. From arm_spe_pmu_irq_handler()
> 
> For (1), we know that profiling is disabled by PMSCR_EL1.ExSPE.
> For (2), we know that profiling is disabled by PMBSR_EL1.S.
> 
> In both cases, we already have an isb() before enabling profiling again
> so I don't understand what this additional isb() is achieving.
> 
> Will

It's to prevent PMBPTR_EL1 from being written to after the PMBLIMITR_EL1 
write than enables the buffer again. So you're right it's already 
disabled up to this point, which is why we didn't need to add another 
isb(). This change is only for the re-enabling bit.

If the instructions were reordered you could get this ordering at the 
end of arm_spe_perf_aux_output_begin():

   write_sysreg_s(limit, SYS_PMBLIMITR_EL1); // Enables buffer

   write_sysreg_s(base, SYS_PMBPTR_EL1);  // Invalid write to PMBPTR

Instead of the new version with the barrier where PMBPTR must come before:

   write_sysreg_s(base, SYS_PMBPTR_EL1);
   isb()
   write_sysreg_s(limit, SYS_PMBLIMITR_EL1);

Thanks
James