Re: [PATCH v2 1/2] riscv: cpufeature: Fix thead vector hwcap removal

From: Andrew Jones <ajones@ventanamicro.com>
To: Charlie Jenkins <charlie@rivosinc.com>
Cc: "Paul Walmsley" <paul.walmsley@sifive.com>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Albert Ou" <aou@eecs.berkeley.edu>,
	"Guo Ren" <guoren@kernel.org>, "Conor Dooley" <conor@kernel.org>,
	"Conor Dooley" <conor.dooley@microchip.com>,
	"Clément Léger" <cleger@rivosinc.com>,
	"Evan Green" <evan@rivosinc.com>,
	"Palmer Dabbelt" <palmer@rivosinc.com>,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] riscv: cpufeature: Fix thead vector hwcap removal
Date: Mon, 29 Apr 2024 11:33:50 +0200	[thread overview]
Message-ID: <20240429-f6438977f19e44966d0dd879@orel> (raw)
In-Reply-To: <20240426-cpufeature_fixes-v2-1-7377442b1327@rivosinc.com>

On Fri, Apr 26, 2024 at 02:58:54PM GMT, Charlie Jenkins wrote:
> The riscv_cpuinfo struct that contains mvendorid and marchid is not
> populated until all harts are booted which happens after the DT parsing.
> Use the vendorid/archid values from the DT if available or assume all
> harts have the same values as the boot hart as a fallback.
> 
> Fixes: d82f32202e0d ("RISC-V: Ignore V from the riscv,isa DT property on older T-Head CPUs")
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Reviewed-by: Guo Ren <guoren@kernel.org>
> ---
>  arch/riscv/include/asm/sbi.h   |  2 ++
>  arch/riscv/kernel/cpu.c        | 40 ++++++++++++++++++++++++++++++++++++----
>  arch/riscv/kernel/cpufeature.c | 11 +++++++++--
>  3 files changed, 47 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 6e68f8dff76b..0fab508a65b3 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -370,6 +370,8 @@ static inline int sbi_remote_fence_i(const struct cpumask *cpu_mask) { return -1
>  static inline void sbi_init(void) {}
>  #endif /* CONFIG_RISCV_SBI */
>  
> +unsigned long riscv_get_mvendorid(void);
> +unsigned long riscv_get_marchid(void);
>  unsigned long riscv_cached_mvendorid(unsigned int cpu_id);
>  unsigned long riscv_cached_marchid(unsigned int cpu_id);
>  unsigned long riscv_cached_mimpid(unsigned int cpu_id);
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index d11d6320fb0d..c1f3655238fd 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -139,6 +139,34 @@ int riscv_of_parent_hartid(struct device_node *node, unsigned long *hartid)
>  	return -1;
>  }
>  
> +unsigned long __init riscv_get_marchid(void)
> +{
> +	struct riscv_cpuinfo *ci = this_cpu_ptr(&riscv_cpuinfo);
> +
> +#if IS_ENABLED(CONFIG_RISCV_SBI)
> +	ci->marchid = sbi_spec_is_0_1() ? 0 : sbi_get_marchid();
> +#elif IS_ENABLED(CONFIG_RISCV_M_MODE)
> +	ci->marchid = csr_read(CSR_MARCHID);
> +#else
> +	ci->marchid = 0;
> +#endif
> +	return ci->marchid;
> +}
> +
> +unsigned long __init riscv_get_mvendorid(void)
> +{
> +	struct riscv_cpuinfo *ci = this_cpu_ptr(&riscv_cpuinfo);
> +
> +#if IS_ENABLED(CONFIG_RISCV_SBI)
> +	ci->mvendorid = sbi_spec_is_0_1() ? 0 : sbi_get_mvendorid();
> +#elif IS_ENABLED(CONFIG_RISCV_M_MODE)
> +	ci->mvendorid = csr_read(CSR_MVENDORID);
> +#else
> +	ci->mvendorid = 0;
> +#endif
> +	return ci->mvendorid;
> +}
> +
>  DEFINE_PER_CPU(struct riscv_cpuinfo, riscv_cpuinfo);
>  
>  unsigned long riscv_cached_mvendorid(unsigned int cpu_id)
> @@ -170,12 +198,16 @@ static int riscv_cpuinfo_starting(unsigned int cpu)
>  	struct riscv_cpuinfo *ci = this_cpu_ptr(&riscv_cpuinfo);
>  
>  #if IS_ENABLED(CONFIG_RISCV_SBI)
> -	ci->mvendorid = sbi_spec_is_0_1() ? 0 : sbi_get_mvendorid();
> -	ci->marchid = sbi_spec_is_0_1() ? 0 : sbi_get_marchid();
> +	if (!ci->mvendorid)
> +		ci->mvendorid = sbi_spec_is_0_1() ? 0 : sbi_get_mvendorid();
> +	if (!ci->marchid)
> +		ci->marchid = sbi_spec_is_0_1() ? 0 : sbi_get_marchid();
>  	ci->mimpid = sbi_spec_is_0_1() ? 0 : sbi_get_mimpid();
>  #elif IS_ENABLED(CONFIG_RISCV_M_MODE)
> -	ci->mvendorid = csr_read(CSR_MVENDORID);
> -	ci->marchid = csr_read(CSR_MARCHID);
> +	if (!ci->mvendorid)
> +		ci->mvendorid = csr_read(CSR_MVENDORID);
> +	if (!ci->marchid)
> +		ci->marchid = csr_read(CSR_MARCHID);
>  	ci->mimpid = csr_read(CSR_MIMPID);
>  #else
>  	ci->mvendorid = 0;
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 3ed2359eae35..500a9bd70f51 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -490,6 +490,8 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>  	struct acpi_table_header *rhct;
>  	acpi_status status;
>  	unsigned int cpu;
> +	u64 boot_vendorid;
> +	u64 boot_archid;
>  
>  	if (!acpi_disabled) {
>  		status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
> @@ -497,6 +499,9 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>  			return;
>  	}
>  
> +	boot_vendorid = riscv_get_mvendorid();
> +	boot_archid = riscv_get_marchid();
> +
>  	for_each_possible_cpu(cpu) {
>  		struct riscv_isainfo *isainfo = &hart_isa[cpu];
>  		unsigned long this_hwcap = 0;
> @@ -543,9 +548,11 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>  		 * version of the vector specification put "v" into their DTs.
>  		 * CPU cores with the ratified spec will contain non-zero
>  		 * marchid.
> +		 *
> +		 * Assume that if the boot hart is T-Head, then all harts in the
> +		 * SoC are also T-Head and have the same archid.

The movement of the comment is only half of my suggestion. The other
suggestion is to remove the 'Assume' because we don't have to assume
anything. We can simply state that if the boot hart is T-HEAD, then we
don't want to enable V on any hart. (We don't need to assume the other
hart IDs are the same, because we don't care what they are. They're
not going to get V, no matter what.)

Thanks,
drew

>  		 */
> -		if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
> -		    riscv_cached_marchid(cpu) == 0x0) {
> +		if (acpi_disabled && boot_vendorid == THEAD_VENDOR_ID && boot_archid == 0x0) {
>  			this_hwcap &= ~isa2hwcap[RISCV_ISA_EXT_v];
>  			clear_bit(RISCV_ISA_EXT_v, isainfo->isa);
>  		}
> 
> -- 
> 2.44.0
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv