Re: [PATCH v3 01/29] riscv: envcfg save and restore on task switching

Charlie Jenkins
2024-05-09T00:11:06Z

On Wed, Apr 03, 2024 at 04:34:49PM -0700, Deepak Gupta wrote:
> envcfg CSR defines enabling bits for cache management instructions and
> soon will control enabling for control flow integrity and pointer
> masking features.
>
> Control flow integrity enabling for forward cfi and backward cfi are
> controlled via envcfg and thus need to be enabled on per thread basis.
>
> This patch creates a place holder for envcfg CSR in `thread_info` and
> adds logic to save and restore on task switching.
>
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> ---
> arch/riscv/include/asm/switch_to.h | 10 ++++++++++
> arch/riscv/include/asm/thread_info.h | 1 +
> 2 files changed, 11 insertions(+)
>
> diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h
> index 7efdb0584d47..2d9a00a30394 100644
> --- a/arch/riscv/include/asm/switch_to.h
> +++ b/arch/riscv/include/asm/switch_to.h
> @@ -69,6 +69,15 @@ 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)
> +{
> + register unsigned long envcfg = next->thread_info.envcfg;

This doesn't need the register storage class.

> +
> + asm volatile (ALTERNATIVE("nop", "csrw " __stringify(CSR_ENVCFG) ", %0", 0,
> + RISCV_ISA_EXT_XLINUXENVCFG, 1)
> + :: "r" (envcfg) : "memory");
> +}
> +

Something like:

static inline void __switch_to_envcfg(struct task_struct *next)
{
if (riscv_has_extension_unlikely(RISCV_ISA_EXT_XLINUXENVCFG))
csr_write(CSR_ENVCFG, next->thread_info.envcfg);
}

would be easier to read, but the alternative you have written doesn't
have the jump that riscv_has_extension_unlikely has so what you have
will be more performant.

Does envcfg need to be save/restored always or just with
CONFIG_RISCV_USER_CFI?

- Charlie

> extern struct task_struct *__switch_to(struct task_struct *,
> struct task_struct *);
>
> @@ -80,6 +89,7 @@ do { \
> __switch_to_fpu(__prev, __next); \
> if (has_vector()) \
> __switch_to_vector(__prev, __next); \
> + __switch_to_envcfg(__next); \
> ((last) = __switch_to(__prev, __next)); \
> } while (0)
>
> diff --git a/arch/riscv/include/asm/thread_info.h b/arch/riscv/include/asm/thread_info.h
> index 5d473343634b..a503bdc2f6dd 100644
> --- a/arch/riscv/include/asm/thread_info.h
> +++ b/arch/riscv/include/asm/thread_info.h
> @@ -56,6 +56,7 @@ struct thread_info {
> long user_sp; /* User stack pointer */
> int cpu;
> unsigned long syscall_work; /* SYSCALL_WORK_ flags */
> + unsigned long envcfg;
> #ifdef CONFIG_SHADOW_CALL_STACK
> void *scs_base;
> void *scs_sp;
> --
> 2.43.2