Received-SPF: pass (google.com: domain of linux-kernel+bounces-138308-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
CC: <yangyicong@hisilicon.com>, <mark.rutland@arm.com>,
	<catalin.marinas@arm.com>, <broonie@kernel.org>,
	<linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
	<jonathan.cameron@huawei.com>, <prime.zeng@hisilicon.com>,
	<linuxarm@huawei.com>
Subject: Re: [PATCH] arm64: arm_pmuv3: Correctly extract and check the PMUVer
To: Will Deacon <will@kernel.org>
References: <20240408081158.15291-1-yangyicong@huawei.com>
 <20240409160915.GA24004@willie-the-truck>
From: Yicong Yang <yangyicong@huawei.com>
Message-ID: <19b6e749-15d7-e522-95ad-b1ae42f032df@huawei.com>
Date: Wed, 10 Apr 2024 18:08:47 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.5.1
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20240409160915.GA24004@willie-the-truck>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit

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@hisilicon.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@hisilicon.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.