[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ff94b709-2861-445d-81c4-9ed98125fe94@linaro.org>
Date: Mon, 8 Sep 2025 15:35:51 +0100
From: James Clark <james.clark@...aro.org>
To: Will Deacon <will@...nel.org>, Alexandru Elisei <Alexandru.Elisei@....com>
Cc: Mark Rutland <mark.rutland@....com>,
Catalin Marinas <catalin.marinas@....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:57 pm, Will Deacon wrote:
> On Mon, Sep 08, 2025 at 02:54:32PM +0100, James Clark wrote:
>>
>>
>> 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.
>>>
>>
>> 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);
>
> ... but my point is that profiling is still disabled after writing to
> PMBLIMITR_EL1.
>
> Will
Oh I see what you mean, I misunderstood that.
You might be right, but I'm looking at statement SFDXJJ and it only says
"...PMBLIMITR_EL1.E is 0b1, meaning the Profiling Buffer is enabled...",
so it's just the buffer rather than "profiling is enabled" which would
require both bits PMBLIMITR_EL1.E = 1 and PMBSR_EL1.S = 0:
SFDXJJ
When PMBLIMITR_EL1.E is 0b1, meaning the Profiling Buffer is enabled,
software must behave as if the PE can do all of the following:
* Ignore writes to the Profiling Buffer controls, other than a write
to PMBLIMITR_EL1.E that disables the Profiling Buffer. The
Statistical Profiling Unit registers affected are:
- PMBPTR_EL1.
- PMBLIMITR_EL1.
- PMBSR_EL1.
- If FEAT_SPE_nVM is implemented, PMBMAR_EL1.
I'm trying to read Alex's other reply to this patch with it in mind that
profiling is still disabled, and it feels like your same point might
apply. Even if it's incorrectly programmed according to the existing Arm
ARM it doesn't matter if it's disabled.
We did discuss internally about the difference between just the buffer
being enabled or profiling altogether being enabled. Looks like I need
to check again.
James
Powered by blists - more mailing lists