Re: [RFC V2 1/3] arm64/cpufeature: Add field details for ID_AA64DFR1_EL1 register

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 8/5/24 19:29, Will Deacon wrote:
> On Thu, Jun 20, 2024 at 02:56:05PM +0530, Anshuman Khandual wrote:
>> This adds required field details for ID_AA64DFR1_EL1, and also drops dummy
>> ftr_raz[] array which is now redundant. These register fields will be used
>> to enable increased breakpoint and watchpoint registers via FEAT_Debugv8p9
>> later.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> cc: Mark Brown <broonie@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/kernel/cpufeature.c | 21 ++++++++++++++++-----
>>  1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index 48e7029f1054..12f0a5181bf2 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -527,6 +527,21 @@ static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
>>  	ARM64_FTR_END,
>>  };
>>  
>> +static const struct arm64_ftr_bits ftr_id_aa64dfr1[] = {
>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ABL_CMPs_SHIFT, 8, 0),
>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_DPFZS_SHIFT, 4, 0),
> 
> I only got this far in the patch, but why is this FTR_STRICT +
> FTR_LOWER_SAFE? The behaviour of the cycle counter on an SPE management
> event sounds like it would be fine to differ between cores, no?

You are right I guess, will this change as FTR_NONSTRICT instead.

> 
> Please go through all the new fields, bearing in mind that most of the
> PMU stuff is per-CPU type rather than global.
Looking at all the register fields in ID_AA64DFR1_EL1 as in the latest
ARM ARM DDI 0487K.a.

Overall there are two different register field categories, the first is
whether an arch feature is enabled or not. This should be all the same
across different cpus on the system, hence FTR_STRICT + FTR_LOWER_SAFE
probably makes sense. The second type is number of HW counter or element
entries per CPU/PMU which can be different across cpus, hence
FTR_NONSTRICT + FTR_LOWER_SAFE might be applicable in such cases.

1. ABL_CMPs
	- Number of breakpoints that support address linking, minus 1

	[FTR_NONSTRICT + FTR_LOWER_SAFE]

2. DPFZS
	- Behavior of the cycle counter when event counting is frozen
	  by a Statistical Profiling management event

	[FTR_NONSTRICT + FTR_LOWER_SAFE]

3. EBEP
	- Exception-based event profiling

	[FTR_STRICT + FTR_LOWER_SAFE]

4. ITE
	- Instrumentation Trace Extension

	[FTR_STRICT + FTR_LOWER_SAFE]

5. ABLE
	- Address Breakpoint Linking Extension

	[FTR_STRICT + FTR_LOWER_SAFE]

6. PMICNTR
	- PMU fixed-function instruction counter
	- Could this vary between cpus ?

	[FTR_NONSTRICT + FTR_LOWER_SAFE]

7. SPMU
	- System PMU extension

	[FTR_STRICT + FTR_LOWER_SAFE]

8. CTX_CMPs
	- Context-aware breakpoints

	[FTR_NONSTRICT + FTR_LOWER_SAFE]

9. WRPs
	- Watchpoints

	[FTR_NONSTRICT + FTR_LOWER_SAFE]

10. BRPs
	- Breakpoints

	[FTR_NONSTRICT + FTR_LOWER_SAFE]

11. SYSPMUID
	- System PMU ID

	[FTR_STRICT + FTR_LOWER_SAFE]

Although please note that existing breakpoint/watchpoint numbers are represented
as FTR_STRICT + FTR_LOWER_SAFE. Hence just wondering if these extended watchpoint
or breakpoint numbers should represented any different.

static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
..............
ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0),
ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_WRPs_SHIFT, 4, 0),
ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRPs_SHIFT, 4, 0),
..............
};

Please suggest if any of the above needs change. Thank you.

- Anshuman