Re: [PATCH v8 01/38] arm64: cpufeature: Always specify and use a field width for capabilities

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 25/01/2022 00:10, Mark Brown wrote:
> Since all the fields in the main ID registers are 4 bits wide we have up
> until now not bothered specifying the width in the code. Since we now
> wish to use this mechanism to enumerate features from the floating point
> feature registers which do not follow this pattern add a width to the
> table.  This means updating all the existing table entries but makes it
> less likely that we run into issues in future due to implicitly assuming
> a 4 bit width.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>   arch/arm64/include/asm/cpufeature.h |   1 +
>   arch/arm64/kernel/cpufeature.c      | 167 +++++++++++++++++-----------
>   2 files changed, 102 insertions(+), 66 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index ef6be92b1921..2728abd9cae4 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -356,6 +356,7 @@ struct arm64_cpu_capabilities {
>   		struct {	/* Feature register checking */
>   			u32 sys_reg;
>   			u8 field_pos;
> +			u8 field_width;
>   			u8 min_field_value;
>   			u8 hwcap_type;
>   			bool sign;


> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index a46ab3b1c4d5..d9f09e40aaf6 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1307,7 +1307,9 @@ u64 __read_sysreg_by_encoding(u32 sys_id)
>   static bool
>   feature_matches(u64 reg, const struct arm64_cpu_capabilities *entry)
>   {
> -	int val = cpuid_feature_extract_field(reg, entry->field_pos, entry->sign);
> +	int val = cpuid_feature_extract_field_width(reg, entry->field_pos,
> +						    entry->field_width,
> +						    entry->sign);
>   

Could we do something like :

+ int val = cpuid_feature_extract_field_width(reg, 		entry->field_pos,
		entry->field_width ? : 4,
		..
		);

and leave the existing structures as they are ?

>   	return val >= entry->min_field_value;
>   }
> @@ -1952,6 +1954,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = {

There is arm64_errata[] array in cpu_errata.c. So, if we use the above
proposal we could leave things unchanged for all existing uses.

>   		.matches = has_cpuid_feature,
>   		.sys_reg = SYS_ID_AA64MMFR0_EL1,
>   		.field_pos = ID_AA64MMFR0_ECV_SHIFT,
> +		.field_width = 4,
>   		.sign = FTR_UNSIGNED,
>   		.min_field_value = 1,
>   	},
...

> -#define HWCAP_CPUID_MATCH(reg, field, s, min_value)				\
> +#define HWCAP_CPUID_MATCH(reg, field, width, s, min_value)			\
>   		.matches = has_cpuid_feature,					\
>   		.sys_reg = reg,							\
>   		.field_pos = field,						\
> +		.field_width = width,						\
>   		.sign = s,							\
>   		.min_field_value = min_value,

And that could avoid these changes too. We could add :

#define HWCAP_CPUID_MATCH_WIDTH(...)

when/if we need one, which sets the width.

Cheers
Suzuki
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm