diff mbox series

[V3,03/16] arm64/cpufeature: Make doublelock a signed feature in ID_AA64DFR0

Message ID 1588426445-24344-4-git-send-email-anshuman.khandual@arm.com (mailing list archive)
State New, archived
Headers show
Series arm64/cpufeature: Introduce ID_PFR2, ID_DFR1, ID_MMFR5 and other changes | expand

Commit Message

Anshuman Khandual May 2, 2020, 1:33 p.m. UTC
Double lock feature can have the following possible values.

0b0000 - Double lock implemented
0b1111 - Double lock not implemented

But in case of a conflict the safe value should be 0b1111. Hence this must
be a signed feature instead. Also change FTR_EXACT to FTR_LOWER_SAFE.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org

Suggested-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/kernel/cpufeature.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Will Deacon May 5, 2020, 11:10 a.m. UTC | #1
On Sat, May 02, 2020 at 07:03:52PM +0530, Anshuman Khandual wrote:
> Double lock feature can have the following possible values.
> 
> 0b0000 - Double lock implemented
> 0b1111 - Double lock not implemented
> 
> But in case of a conflict the safe value should be 0b1111. Hence this must
> be a signed feature instead. Also change FTR_EXACT to FTR_LOWER_SAFE.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> 
> Suggested-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/kernel/cpufeature.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 51386dade423..cba43e4a5c79 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -338,7 +338,7 @@ static const struct arm64_ftr_bits ftr_id_mmfr0[] = {
>  };
>  
>  static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
> -	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, 36, 28, 0),
> +	S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 36, 28, 0),

Wait, isn't this buggered today? Shouldn't that 28 be a 4? I think we really
need to:

	1. Make it impossible to describe overlapping fields, incomplete
	   registers etc (ideally at build-time)

	2. Have a macro that for 4-bit fields so you don't have to type '4'
	   all the time

Suzuki, any ideas how we can make this a bit more robust?

Will
Anshuman Khandual May 8, 2020, 4:59 a.m. UTC | #2
On 05/05/2020 04:40 PM, Will Deacon wrote:
> On Sat, May 02, 2020 at 07:03:52PM +0530, Anshuman Khandual wrote:
>> Double lock feature can have the following possible values.
>>
>> 0b0000 - Double lock implemented
>> 0b1111 - Double lock not implemented
>>
>> But in case of a conflict the safe value should be 0b1111. Hence this must
>> be a signed feature instead. Also change FTR_EXACT to FTR_LOWER_SAFE.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>>
>> Suggested-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/kernel/cpufeature.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index 51386dade423..cba43e4a5c79 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -338,7 +338,7 @@ static const struct arm64_ftr_bits ftr_id_mmfr0[] = {
>>  };
>>  
>>  static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
>> -	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, 36, 28, 0),
>> +	S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 36, 28, 0),
> 
> Wait, isn't this buggered today? Shouldn't that 28 be a 4? I think we really

Ahh, right. Will fix it.

> need to:
> 
> 	1. Make it impossible to describe overlapping fields, incomplete
> 	   registers etc (ideally at build-time)

AFICS the _SHIFT defines for a given register must be placed sequentially
with dummy place holders (4 bit wide) for any missing fields. In that case
we could just call BUILD_BUG_ON() for any possible breakage or overlap. But
wondering how and where to loop over these SHIFT values for these registers.

Another way (not build time though) will be to scan through ftr_id_xxx[],
fetch individual arm64_ftr_bits (assuming there are dummy entries for non
existing fields) and assert that arm6r_ftr_bits[shift, width] validates
against the previous arm6r_ftr_bits[shift, width] in an increasing manner.

Either of these methods will require some more thoughts.

> 
> 	2. Have a macro that for 4-bit fields so you don't have to type '4'
> 	   all the time

Except for ftr_single32[], all other arm64_ftr_bits entries have the exact
same shift value (i.e 4). ARM64_FTR_WIDTH sounds good ?

> 
> Suzuki, any ideas how we can make this a bit more robust?
> 
> Will
>
diff mbox series

Patch

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 51386dade423..cba43e4a5c79 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -338,7 +338,7 @@  static const struct arm64_ftr_bits ftr_id_mmfr0[] = {
 };
 
 static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
-	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, 36, 28, 0),
+	S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 36, 28, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_PMSVER_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_CTX_CMPS_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_WRPS_SHIFT, 4, 0),