Received-SPF: pass (google.com: domain of linux-kernel+bounces-206790-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 7 Jun 2024 14:59:00 -0700
From: Deepak Gupta <debug@rivosinc.com>
To: Samuel Holland <samuel.holland@sifive.com>
Cc: linux-riscv@lists.infradead.org, Palmer Dabbelt <palmer@dabbelt.com>,
	Andrew Jones <ajones@ventanamicro.com>,
	Conor Dooley <conor@kernel.org>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] riscv: Add support for per-thread envcfg CSR values
Message-ID: <ZmOCpF5ACdQiSfcm@debug.ba.rivosinc.com>
References: <20240605205658.184399-1-samuel.holland@sifive.com>
 <20240605205658.184399-3-samuel.holland@sifive.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20240605205658.184399-3-samuel.holland@sifive.com>

On Wed, Jun 05, 2024 at 01:56:46PM -0700, Samuel Holland wrote:
>Some bits in the [ms]envcfg CSR, such as the CFI state and pointer
>masking mode, need to be controlled on a per-thread basis. Support this
>by keeping a copy of the CSR value in struct thread_struct and writing
>it during context switches. It is safe to discard the old CSR value
>during the context switch because the CSR is modified only by software,
>so the CSR will remain in sync with the copy in thread_struct.
>
>Use ALTERNATIVE directly instead of riscv_has_extension_unlikely() to
>minimize branchiness in the context switching code.
>
>Since thread_struct is copied during fork(), setting the value for the
>init task sets the default value for all other threads.
>
>Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>---
>
> arch/riscv/include/asm/processor.h | 1 +
> arch/riscv/include/asm/switch_to.h | 8 ++++++++
> arch/riscv/kernel/cpufeature.c     | 2 +-
> 3 files changed, 10 insertions(+), 1 deletion(-)
>
>diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
>index 68c3432dc6ea..0838922bd1c8 100644
>--- a/arch/riscv/include/asm/processor.h
>+++ b/arch/riscv/include/asm/processor.h
>@@ -118,6 +118,7 @@ struct thread_struct {
> 	unsigned long s[12];	/* s[0]: frame pointer */
> 	struct __riscv_d_ext_state fstate;
> 	unsigned long bad_cause;
>+	unsigned long envcfg;
> 	u32 riscv_v_flags;
> 	u32 vstate_ctrl;
> 	struct __riscv_v_ext_state vstate;
>diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h
>index 7594df37cc9f..9685cd85e57c 100644
>--- a/arch/riscv/include/asm/switch_to.h
>+++ b/arch/riscv/include/asm/switch_to.h
>@@ -70,6 +70,13 @@ static __always_inline bool has_fpu(void) { return false; }
> #define __switch_to_fpu(__prev, __next) do { } while (0)
> #endif
>
>+static inline void __switch_to_envcfg(struct task_struct *next)
>+{
>+	asm volatile (ALTERNATIVE("nop", "csrw " __stringify(CSR_ENVCFG) ", %0",
>+				  0, RISCV_ISA_EXT_XLINUXENVCFG, 1)
>+			:: "r" (next->thread.envcfg) : "memory");
>+}
>+
> extern struct task_struct *__switch_to(struct task_struct *,
> 				       struct task_struct *);
>
>@@ -103,6 +110,7 @@ do {							\
> 		__switch_to_vector(__prev, __next);	\
> 	if (switch_to_should_flush_icache(__next))	\
> 		local_flush_icache_all();		\
>+	__switch_to_envcfg(__next);			\
> 	((last) = __switch_to(__prev, __next));		\
> } while (0)

Suggestion:
Probably make this patch 1

>
>diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>index 2879e26dbcd8..1153b96346ae 100644
>--- a/arch/riscv/kernel/cpufeature.c
>+++ b/arch/riscv/kernel/cpufeature.c
>@@ -728,7 +728,7 @@ unsigned long riscv_get_elf_hwcap(void)
> void riscv_user_isa_enable(void)
> {
> 	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ))
>-		csr_set(CSR_ENVCFG, ENVCFG_CBZE);
>+		current->thread.envcfg |= ENVCFG_CBZE;

Suggestion:
Squash this with current patch 1 and call it patch 2.

> 	else if (any_cpu_has_zicboz)
> 		pr_warn_once("Zicboz disabled as it is unavailable on some harts\n");
> }
>-- 
>2.44.1
>