Re: [PATCH v4 10/38] arm64: Explicitly save/restore CPACR when probing SVE and SME

From: Catalin Marinas <catalin.marinas@arm.com>
To: Mark Brown <broonie@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
	linux-arm-kernel@lists.infradead.org, ardb@kernel.org,
	bertrand.marquis@arm.com, boris.ostrovsky@oracle.com,
	daniel.lezcano@linaro.org, james.morse@arm.com, jgross@suse.com,
	kristina.martsenko@arm.com, maz@kernel.org,
	oliver.upton@linux.dev, pcc@google.com, sstabellini@kernel.org,
	suzuki.poulose@arm.com, tglx@linutronix.de,
	vladimir.murzin@arm.com, will@kernel.org
Subject: Re: [PATCH v4 10/38] arm64: Explicitly save/restore CPACR when probing SVE and SME
Date: Mon, 16 Oct 2023 17:11:15 +0100	[thread overview]
Message-ID: <ZS1goxBNnv7VVSQV@arm.com> (raw)
In-Reply-To: <ab20ab8c-57bf-4cc7-a277-ce8a3482f2df@sirena.org.uk>

On Mon, Oct 16, 2023 at 01:02:13PM +0100, Mark Brown wrote:
> On Mon, Oct 16, 2023 at 11:24:33AM +0100, Mark Rutland wrote:
> > When a CPUs onlined we first probe for supported features and
> > propetites, and then we subsequently enable features that have been
> > detected. This is a little problematic for SVE and SME, as some
> > properties (e.g. vector lengths) cannot be probed while they are
> > disabled. Due to this, the code probing for SVE properties has to enable
> 
> Reviewed-by: Mark Brown <broonie@kernel.org>

Thanks Mark for reviewing. Could you please also check my conflict
resolution? Mark R's patches conflict with your patches on
for-next/sve-remove-pseudo-regs (maybe I should have applied them on
top; hopefully git rerere remembers it correctly).

diff --cc arch/arm64/include/asm/fpsimd.h
index 9e5d3a0812b6,c43ae9c013ec..50e5f25d3024

--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@@ -123,11 -149,13 +149,12 @@@ extern void sme_save_state(void *state
  extern void sme_load_state(void const *state, int zt);
  
  struct arm64_cpu_capabilities;
- extern void sve_kernel_enable(const struct arm64_cpu_capabilities *__unused);
- extern void sme_kernel_enable(const struct arm64_cpu_capabilities *__unused);
- extern void sme2_kernel_enable(const struct arm64_cpu_capabilities *__unused);
- extern void fa64_kernel_enable(const struct arm64_cpu_capabilities *__unused);
+ extern void cpu_enable_fpsimd(const struct arm64_cpu_capabilities *__unused);
+ extern void cpu_enable_sve(const struct arm64_cpu_capabilities *__unused);
+ extern void cpu_enable_sme(const struct arm64_cpu_capabilities *__unused);
+ extern void cpu_enable_sme2(const struct arm64_cpu_capabilities *__unused);
+ extern void cpu_enable_fa64(const struct arm64_cpu_capabilities *__unused);
  
 -extern u64 read_zcr_features(void);
  extern u64 read_smcr_features(void);
  
  /*
diff --cc arch/arm64/kernel/cpufeature.c
index 55a3bc719d46,ad7ec30d3bd3..397a1bbf4fba
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@@ -1026,21 -1040,30 +1026,26 @@@ void __init init_cpu_features(struct cp
  
  	if (IS_ENABLED(CONFIG_ARM64_SVE) &&
  	    id_aa64pfr0_sve(read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1))) {
- 		sve_kernel_enable(NULL);
+ 		unsigned long cpacr = cpacr_save_enable_kernel_sve();
+ 
 -		info->reg_zcr = read_zcr_features();
 -		init_cpu_ftr_reg(SYS_ZCR_EL1, info->reg_zcr);
  		vec_init_vq_map(ARM64_VEC_SVE);
+ 
+ 		cpacr_restore(cpacr);
  	}
  
  	if (IS_ENABLED(CONFIG_ARM64_SME) &&
  	    id_aa64pfr1_sme(read_sanitised_ftr_reg(SYS_ID_AA64PFR1_EL1))) {
- 		sme_kernel_enable(NULL);
+ 		unsigned long cpacr = cpacr_save_enable_kernel_sme();
  
 -		info->reg_smcr = read_smcr_features();
  		/*
  		 * 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;
 -		init_cpu_ftr_reg(SYS_SMCR_EL1, info->reg_smcr);
  		vec_init_vq_map(ARM64_VEC_SME);
+ 
+ 		cpacr_restore(cpacr);
  	}
  
  	if (id_aa64pfr1_mte(info->reg_id_aa64pfr1))
@@@ -1274,29 -1297,40 +1279,35 @@@ void update_cpu_features(int cpu
  	taint |= check_update_ftr_reg(SYS_ID_AA64SMFR0_EL1, cpu,
  				      info->reg_id_aa64smfr0, boot->reg_id_aa64smfr0);
  
 +	/* Probe vector lengths */
  	if (IS_ENABLED(CONFIG_ARM64_SVE) &&
  	    id_aa64pfr0_sve(read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1))) {
 -		unsigned long cpacr = cpacr_save_enable_kernel_sve();
 +		if (!system_capabilities_finalized()) {
- 			sve_kernel_enable(NULL);
++			unsigned long cpacr = cpacr_save_enable_kernel_sve();
+ 
 -		info->reg_zcr = read_zcr_features();
 -		taint |= check_update_ftr_reg(SYS_ZCR_EL1, cpu,
 -					info->reg_zcr, boot->reg_zcr);
 -
 -		/* Probe vector lengths */
 -		if (!system_capabilities_finalized())
  			vec_update_vq_map(ARM64_VEC_SVE);
+ 
 -		cpacr_restore(cpacr);
++			cpacr_restore(cpacr);
 +		}
  	}
  
  	if (IS_ENABLED(CONFIG_ARM64_SME) &&
  	    id_aa64pfr1_sme(read_sanitised_ftr_reg(SYS_ID_AA64PFR1_EL1))) {
- 		sme_kernel_enable(NULL);
 -		unsigned long cpacr = cpacr_save_enable_kernel_sme();
--
 -		info->reg_smcr = read_smcr_features();
  		/*
  		 * 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;
 -		taint |= check_update_ftr_reg(SYS_SMCR_EL1, cpu,
 -					info->reg_smcr, boot->reg_smcr);
  
  		/* Probe vector lengths */
--		if (!system_capabilities_finalized())
++		if (!system_capabilities_finalized()) {
++			unsigned long cpacr = cpacr_save_enable_kernel_sme();
++
  			vec_update_vq_map(ARM64_VEC_SME);
+ 
 -		cpacr_restore(cpacr);
++			cpacr_restore(cpacr);
++		}
  	}
  
  	/*
@@@ -3138,7 -3182,15 +3162,9 @@@ static void verify_local_elf_hwcaps(voi
  
  static void verify_sve_features(void)
  {
+ 	unsigned long cpacr = cpacr_save_enable_kernel_sve();
+ 
 -	u64 safe_zcr = read_sanitised_ftr_reg(SYS_ZCR_EL1);
 -	u64 zcr = read_zcr_features();
 -
 -	unsigned int safe_len = safe_zcr & ZCR_ELx_LEN_MASK;
 -	unsigned int len = zcr & ZCR_ELx_LEN_MASK;
 -
 -	if (len < safe_len || vec_verify_vq_map(ARM64_VEC_SVE)) {
 +	if (vec_verify_vq_map(ARM64_VEC_SVE)) {
  		pr_crit("CPU%d: SVE: vector length support mismatch\n",
  			smp_processor_id());
  		cpu_die_early();
@@@ -3147,7 -3201,15 +3175,9 @@@
  
  static void verify_sme_features(void)
  {
+ 	unsigned long cpacr = cpacr_save_enable_kernel_sme();
+ 
 -	u64 safe_smcr = read_sanitised_ftr_reg(SYS_SMCR_EL1);
 -	u64 smcr = read_smcr_features();
 -
 -	unsigned int safe_len = safe_smcr & SMCR_ELx_LEN_MASK;
 -	unsigned int len = smcr & SMCR_ELx_LEN_MASK;
 -
 -	if (len < safe_len || vec_verify_vq_map(ARM64_VEC_SME)) {
 +	if (vec_verify_vq_map(ARM64_VEC_SME)) {
  		pr_crit("CPU%d: SME: vector length support mismatch\n",
  			smp_processor_id());
  		cpu_die_early();
diff --cc arch/arm64/kernel/fpsimd.c
index 04c801001767,d0d28bc069d2..5ddc246f1482
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@@ -1173,11 -1169,30 +1169,11 @@@ void cpu_enable_sve(const struct arm64_
  void __init sve_setup(void)
  {
  	struct vl_info *info = &vl_info[ARM64_VEC_SVE];
 -	u64 zcr;
  	DECLARE_BITMAP(tmp_map, SVE_VQ_MAX);
  	unsigned long b;
 +	int max_bit;
  
- 	if (!system_supports_sve())
+ 	if (!cpus_have_cap(ARM64_SVE))
  		return;
  
  	/*
@@@ -1307,9 -1329,29 +1301,9 @@@ void cpu_enable_fa64(const struct arm64
  void __init sme_setup(void)
  {
  	struct vl_info *info = &vl_info[ARM64_VEC_SME];
 -	u64 smcr;
 -	int min_bit;
 +	int min_bit, max_bit;
  
- 	if (!system_supports_sme())
+ 	if (!cpus_have_cap(ARM64_SME))
  		return;
  
  	/*

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
