Re: [PATCH] arm64/sme: Move storage of reg_smidr to __cpuinfo_store_cpu()

[Marc Zyngier <maz@kernel.org>]

On Mon, 16 Dec 2024 14:31:47 +0000,
Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Mon, Dec 16, 2024 at 01:23:55PM +0000, Mark Brown wrote:
> > On Mon, Dec 16, 2024 at 12:44:14PM +0000, Mark Rutland wrote:
> > 
> > > ... didn't matter either way, and using '&boot_cpu_data' was intended to
> > > make it clear that the features were based on the boot CPU's info, even
> > > if you just grepped for that and didn't see the surrounding context.
> > 
> > Right, that was my best guess as to what was supposed to be going on
> > but it wasn't super clear.  The code could use some more comments.
> > 
> > > I think the real fix here is to move the reading back into
> > > __cpuinfo_store_cpu(), but to have an explicit check that SME has been
> > > disabled on the commandline, with a comment explaining that this is a
> > > bodge for broken FW which traps the SME ID regs.
> > 
> > That should be doable.
> > 
> > There's a few other similar ID registers (eg, we already read GMID_EL1
> > and MPAMIDR_EL1) make me a bit nervous that we might need to generalise
> > it a bit, but we can deal with that if it comes up.  Even for SME the
> > disable was added speculatively, the factors that made this come up for
> > SVE are less likely to be an issue with SME.
> 
> FWIW, I had a quick go (with only the SME case), and I think the shape
> that we want is roughly as below, which I think is easy to generalise to
> those other cases.
> 
> MarcZ, thoughts?
> 
> Mark.
> 
> ---->8----
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 8b4e5a3cd24c8..f16eb99c10723 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -91,6 +91,16 @@ struct arm64_ftr_override {
>  	u64		mask;
>  };
>  
> +static inline u64
> +arm64_ftr_override_apply(const struct arm64_ftr_override *override,
> +			 u64 val)
> +{
> +	val &= ~override->mask;
> +	val |= override->val & override->mask;
> +
> +	return val;
> +}
> +
>  /*
>   * @arm64_ftr_reg - Feature register
>   * @strict_mask		Bits which should match across all CPUs for sanity.
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 6ce71f444ed84..faad7d3e4cf5f 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1167,12 +1167,6 @@ void __init init_cpu_features(struct cpuinfo_arm64 *info)
>  	    id_aa64pfr1_sme(read_sanitised_ftr_reg(SYS_ID_AA64PFR1_EL1))) {
>  		unsigned long cpacr = cpacr_save_enable_kernel_sme();
>  
> -		/*
> -		 * We mask out SMPS since even if the hardware
> -		 * supports priorities the kernel does not at present
> -		 * and we block access to them.
> -		 */
> -		info->reg_smidr = read_cpuid(SMIDR_EL1) & ~SMIDR_EL1_SMPS;
>  		vec_init_vq_map(ARM64_VEC_SME);
>  
>  		cpacr_restore(cpacr);
> @@ -1550,10 +1544,8 @@ u64 __read_sysreg_by_encoding(u32 sys_id)
>  	}
>  
>  	regp  = get_arm64_ftr_reg(sys_id);
> -	if (regp) {
> -		val &= ~regp->override->mask;
> -		val |= (regp->override->val & regp->override->mask);
> -	}
> +	if (regp)
> +		val = arm64_ftr_override_apply(regp->override, val);
>  
>  	return val;
>  }
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> index d79e88fccdfce..1460e3541132f 100644
> --- a/arch/arm64/kernel/cpuinfo.c
> +++ b/arch/arm64/kernel/cpuinfo.c
> @@ -439,6 +439,24 @@ static void __cpuinfo_store_cpu_32bit(struct cpuinfo_32bit *info)
>  	info->reg_mvfr2 = read_cpuid(MVFR2_EL1);
>  }
>  
> +static void __cpuinfo_store_cpu_sme(struct cpuinfo_arm64 *info)
> +{
> +	/*
> +	 * TODO: explain that this bodge is to avoid trapping.
> +	 */
> +	u64 pfr1 = info->reg_id_aa64pfr1;
> +	pfr1 = arm64_ftr_override_apply(&id_aa64pfr1_override, pfr1);
> +	if (!id_aa64pfr1_sme(pfr1))
> +		return;

I don't think blindly applying the override at this stage is a good
thing. Specially not the whole register, and definitely not
non-disabling values.

It needs to be done on a per feature basis, and only to disable
things.

See the hack I posted for the things I think need checking.

	M.

-- 
Without deviation from the norm, progress is not possible.