[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <19b6e749-15d7-e522-95ad-b1ae42f032df@huawei.com>
Date: Wed, 10 Apr 2024 18:08:47 +0800
From: Yicong Yang <yangyicong@...wei.com>
To: Will Deacon <will@...nel.org>
CC: <yangyicong@...ilicon.com>, <mark.rutland@....com>,
<catalin.marinas@....com>, <broonie@...nel.org>,
<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
<jonathan.cameron@...wei.com>, <prime.zeng@...ilicon.com>,
<linuxarm@...wei.com>
Subject: Re: [PATCH] arm64: arm_pmuv3: Correctly extract and check the PMUVer
On 2024/4/10 0:09, Will Deacon wrote:
> On Mon, Apr 08, 2024 at 04:11:58PM +0800, Yicong Yang wrote:
>> From: Yicong Yang <yangyicong@...ilicon.com>
>>
>> Currently we're using "sbfx" to extract the PMUVer from ID_AA64DFR0_EL1
>> and skip the init/reset if no PMU present when the extracted PMUVer is
>> negative or is zero. However for PMUv3p8 the PMUVer will be 0b1000 and
>> PMUVer extracted by "sbfx" will always be negative and we'll skip the
>> init/reset in __init_el2_debug/reset_pmuserenr_el0 unexpectedly.
>>
>> So this patch use "ubfx" instead of "sbfx" to extract the PMUVer. If
>> the PMUVer is implementation defined (0b1111) then reset it to zero
>> and skip the reset/init. Previously we'll also skip the init/reset
>> if the PMUVer is higher than the version we known (currently PMUv3p9),
>> with this patch we'll only skip if the PMU is not implemented or
>> implementation defined. This keeps consistence with how we probe
>> the PMU in the driver with pmuv3_implemented().
>>
>> Signed-off-by: Yicong Yang <yangyicong@...ilicon.com>
>> ---
>> arch/arm64/include/asm/assembler.h | 5 ++++-
>> arch/arm64/include/asm/el2_setup.h | 5 ++++-
>> 2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
>> index ab8b396428da..3b7373d6c565 100644
>> --- a/arch/arm64/include/asm/assembler.h
>> +++ b/arch/arm64/include/asm/assembler.h
>> @@ -480,7 +480,10 @@ alternative_endif
>> */
>> .macro reset_pmuserenr_el0, tmpreg
>> mrs \tmpreg, id_aa64dfr0_el1
>> - sbfx \tmpreg, \tmpreg, #ID_AA64DFR0_EL1_PMUVer_SHIFT, #4
>> + ubfx \tmpreg, \tmpreg, #ID_AA64DFR0_EL1_PMUVer_SHIFT, #4
>> + cmp \tmpreg, #ID_AA64DFR0_EL1_PMUVer_IMP_DEF
>> + csel \tmpreg, xzr, \tmpreg, eq // If PMU's IMP_DEF, regard it
>> + // as not implemented and skip
>> cmp \tmpreg, #1 // Skip if no PMU present
>> b.lt 9000f
>> msr pmuserenr_el0, xzr // Disable PMU access from EL0
>
> I think the cmp/csel/cmp/b.lt sequence might be a little tidier if you
> reworked it to use ccmp. For example, something like (totally untested):
>
> cmp \tmpreg, ID_AA64DFR0_EL1_PMUVer_NI
> ccmp \tmpreg, ID_AA64DFR0_EL1_PMUVer_IMP_DEF, #4, ne
>
> would then, I think, mean we could just b.eq 9000f. But please check
> this because encoding nzcv as an immediate always catches me out.
>
ok. will have a test on my boards. Wrote a small demo for checking all the available
versions with suggested code and seems work fine.
>> diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
>> index b7afaa026842..2438e12b60c5 100644
>> --- a/arch/arm64/include/asm/el2_setup.h
>> +++ b/arch/arm64/include/asm/el2_setup.h
>> @@ -59,7 +59,10 @@
>>
>> .macro __init_el2_debug
>> mrs x1, id_aa64dfr0_el1
>> - sbfx x0, x1, #ID_AA64DFR0_EL1_PMUVer_SHIFT, #4
>> + ubfx x0, x1, #ID_AA64DFR0_EL1_PMUVer_SHIFT, #4
>> + cmp x0, #ID_AA64DFR0_EL1_PMUVer_IMP_DEF
>> + csel x0, xzr, x0, eq // If PMU's IMP_DEF, regard it
>> + // as not implemented and skip
>> cmp x0, #1
>> b.lt .Lskip_pmu_\@ // Skip if no PMU present
>> mrs x0, pmcr_el0 // Disable debug access traps
>
> Similar sort of thing here.
will also need to change the cond after the .Lskip_pmu_\@ like below:
.macro __init_el2_debug
mrs x1, id_aa64dfr0_el1
- sbfx x0, x1, #ID_AA64DFR0_EL1_PMUVer_SHIFT, #4
- cmp x0, #1
- b.lt .Lskip_pmu_\@ // Skip if no PMU present
+ ubfx x0, x1, #ID_AA64DFR0_EL1_PMUVer_SHIFT, #4
+ cmp x0, #ID_AA64DFR0_EL1_PMUVer_NI
+ ccmp x0, #ID_AA64DFR0_EL1_PMUVer_IMP_DEF, #4, ne
+ b.eq .Lskip_pmu_\@ // Skip if no PMU present or IMP_DEF
mrs x0, pmcr_el0 // Disable debug access traps
ubfx x0, x0, #11, #5 // to EL2 and allow access to
.Lskip_pmu_\@:
- csel x2, xzr, x0, lt // all PMU counters from EL1
+ csel x2, xzr, x0, eq // all PMU counters from EL1
Will respin a v2 after tests.
Thanks.
Powered by blists - more mailing lists