Re: [RFC PATCH v1 02/28] riscv: envcfg save and restore on trap entry/exit

[Deepak Gupta]

On Thu, Jan 25, 2024 at 09:09:14AM -0800, Deepak Gupta wrote:
> On Thu, Jan 25, 2024 at 02:19:29AM -0500, Stefan O'Rear wrote:
> > On Thu, Jan 25, 2024, at 1:21 AM, debug@xxxxxxxxxxxx wrote:
> > > From: Deepak Gupta <debug@xxxxxxxxxxxx>
> > >
> > > 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 is 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 trap entry and exits.
> >
> > Should only be "restore"?  I don't see saving.
>
> It's always saved in `thread_info` and user mode can't change it.
> So no point saving it.

Also I'll fix the commit message. I think that's what you were pointing out.

> > > Signed-off-by: Deepak Gupta <debug@xxxxxxxxxxxx>
> > > ---
> > > arch/riscv/include/asm/thread_info.h | 1 +
> > > arch/riscv/kernel/asm-offsets.c      | 1 +
> > > arch/riscv/kernel/entry.S            | 4 ++++
> > > 3 files changed, 6 insertions(+)
> > >
> > > diff --git a/arch/riscv/include/asm/thread_info.h
> > > b/arch/riscv/include/asm/thread_info.h
> > > index 574779900bfb..320bc899a63b 100644
> > > --- a/arch/riscv/include/asm/thread_info.h
> > > +++ b/arch/riscv/include/asm/thread_info.h
> > > @@ -57,6 +57,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;
> > > diff --git a/arch/riscv/kernel/asm-offsets.c
> > > b/arch/riscv/kernel/asm-offsets.c
> > > index a03129f40c46..cdd8f095c30c 100644
> > > --- a/arch/riscv/kernel/asm-offsets.c
> > > +++ b/arch/riscv/kernel/asm-offsets.c
> > > @@ -39,6 +39,7 @@ void asm_offsets(void)
> > > 	OFFSET(TASK_TI_PREEMPT_COUNT, task_struct, thread_info.preempt_count);
> > > 	OFFSET(TASK_TI_KERNEL_SP, task_struct, thread_info.kernel_sp);
> > > 	OFFSET(TASK_TI_USER_SP, task_struct, thread_info.user_sp);
> > > +	OFFSET(TASK_TI_ENVCFG, task_struct, thread_info.envcfg);
> > > #ifdef CONFIG_SHADOW_CALL_STACK
> > > 	OFFSET(TASK_TI_SCS_SP, task_struct, thread_info.scs_sp);
> > > #endif
> > > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> > > index 54ca4564a926..63c3855ba80d 100644
> > > --- a/arch/riscv/kernel/entry.S
> > > +++ b/arch/riscv/kernel/entry.S
> > > @@ -129,6 +129,10 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
> > > 	addi s0, sp, PT_SIZE_ON_STACK
> > > 	REG_S s0, TASK_TI_KERNEL_SP(tp)
> > >
> > > +	/* restore envcfg bits for current thread */
> > > +	REG_L s0, TASK_TI_ENVCFG(tp)
> > > +	csrw CSR_ENVCFG, s0
> > > +
> >
> > This is redundant if we're repeatedly processing interrupts or exceptions
> > within a single task.  We should only be writing envcfg when switching
> > between tasks or as part of the prctl.
> >
> > We need to use an ALTERNATIVE for this since the oldest supported hardware
> > does not have envcfg csrs.
>
> Yeah fixing that in next series. Thanks
>
> > -s
> >
> > > 	/* Save the kernel shadow call stack pointer */
> > > 	scs_save_current
> > >
> > > --
> > > 2.43.0
> > >
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@xxxxxxxxxxxxxxxxxxx
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv