Re: [v1, 5/6] riscv: vector: allow kernel-mode Vector with preemption

[Conor Dooley <conor.dooley@microchip.com>]

[-- Attachment #1.1: Type: text/plain, Size: 8277 bytes --]

On Sat, Jul 15, 2023 at 03:00:31PM +0000, Andy Chiu wrote:
> Add kernel_vstate to keep track of kernel-mode Vector registers when
> trap introduced context switch happens. Also, provide trap_pt_regs to
> let context save/restore routine reference status.VS at which the trap
> takes place. The thread flag TIF_RISCV_V_KMV indicates whether a task is
> running in kernel-mode Vector with preemption 'ON'. So context switch
> routines know and would save V-regs to kernel_vstate and restore V-regs
> immediately from kernel_vstate if the bit is set.
> 
> Apart from a task's preemption status, the capability of
> running preemptive kernel-mode Vector is jointly controlled by the
> RISCV_V_VSTATE_CTRL_KMV_PREEMPTIBLE mask in the task's
> thread.vstate_ctrl. This bit is masked whenever a trap takes place in
> kernel mode while executing preemptive Vector code.
> 
> Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> ---
>  arch/riscv/include/asm/processor.h     |  2 +
>  arch/riscv/include/asm/thread_info.h   |  4 ++
>  arch/riscv/include/asm/vector.h        | 27 ++++++++++--
>  arch/riscv/kernel/asm-offsets.c        |  2 +
>  arch/riscv/kernel/entry.S              | 41 ++++++++++++++++++
>  arch/riscv/kernel/kernel_mode_vector.c | 57 ++++++++++++++++++++++++--
>  arch/riscv/kernel/process.c            |  8 +++-
>  arch/riscv/kernel/vector.c             |  3 +-
>  8 files changed, 136 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> index e82af1097e26..d337b750f2ec 100644
> --- a/arch/riscv/include/asm/processor.h
> +++ b/arch/riscv/include/asm/processor.h
> @@ -42,6 +42,8 @@ struct thread_struct {
>  	unsigned long bad_cause;
>  	unsigned long vstate_ctrl;
>  	struct __riscv_v_ext_state vstate;
> +	struct pt_regs *trap_pt_regs;
> +	struct __riscv_v_ext_state kernel_vstate;
>  };
>  
>  /* Whitelist the fstate from the task_struct for hardened usercopy */
> diff --git a/arch/riscv/include/asm/thread_info.h b/arch/riscv/include/asm/thread_info.h
> index d83975efe866..59d88adfc4de 100644
> --- a/arch/riscv/include/asm/thread_info.h
> +++ b/arch/riscv/include/asm/thread_info.h
> @@ -102,6 +102,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
>  #define TIF_UPROBE		10	/* uprobe breakpoint or singlestep */
>  #define TIF_32BIT		11	/* compat-mode 32bit process */
>  #define TIF_RISCV_V_DEFER_RESTORE	12
> +#define TIF_RISCV_V_KMV			13

Same comment about comments.

Also, the "V" here is a dupe, since you have RISCV_V in the name.
Ditto everywhere else, afaict. Perhaps you could do s/KMV/KERNEL_MODE/?

>  #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
>  #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
> @@ -109,9 +110,12 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
>  #define _TIF_NOTIFY_SIGNAL	(1 << TIF_NOTIFY_SIGNAL)
>  #define _TIF_UPROBE		(1 << TIF_UPROBE)
>  #define _TIF_RISCV_V_DEFER_RESTORE	(1 << TIF_RISCV_V_DEFER_RESTORE)
> +#define _TIF_RISCV_V_KMV		(1 << TIF_RISCV_V_KMV_TASK)

Where is KMV_TASK defined?

>  
>  #define _TIF_WORK_MASK \
>  	(_TIF_NOTIFY_RESUME | _TIF_SIGPENDING | _TIF_NEED_RESCHED | \
>  	 _TIF_NOTIFY_SIGNAL | _TIF_UPROBE)
>  
> +#define RISCV_V_VSTATE_CTRL_KMV_PREEMPTIBLE	0x20
> +
>  #endif /* _ASM_RISCV_THREAD_INFO_H */
> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> index 50c556afd95a..d004c9fa6a57 100644
> --- a/arch/riscv/include/asm/vector.h
> +++ b/arch/riscv/include/asm/vector.h
> @@ -25,6 +25,12 @@ bool riscv_v_first_use_handler(struct pt_regs *regs);
>  int kernel_rvv_begin(void);
>  void kernel_rvv_end(void);
>  
> +#ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE_KMV
> +void riscv_v_vstate_ctrl_config_kmv(bool preemptive_kmv);
> +#else
> +#define riscv_v_vstate_ctrl_config_kmv(preemptive_kmv)	do {} while (0)
> +#endif

For clang/llvm allmodconfig:
../arch/riscv/kernel/process.c:213:2: error: call to undeclared function 'riscv_v_vstate_ctrl_config_kmv'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]

Probably also happens when vector is disabled?


> +
>  static __always_inline bool has_vector(void)
>  {
>  	return riscv_has_extension_unlikely(RISCV_ISA_EXT_v);
> @@ -195,9 +201,24 @@ static inline void __switch_to_vector(struct task_struct *prev,
>  {
>  	struct pt_regs *regs;
>  
> -	regs = task_pt_regs(prev);
> -	riscv_v_vstate_save(prev->thread.vstate, regs);
> -	riscv_v_vstate_set_restore(next, task_pt_regs(next));
> +	if (IS_ENABLED(CONFIG_RISCV_ISA_V_PREEMPTIVE_KMV) &&

w.r.t. this symbol, just drop the KMV?

> +	    test_tsk_thread_flag(prev, TIF_RISCV_V_KMV)) {
> +		regs = prev->thread.trap_pt_regs;
> +		WARN_ON(!regs);
> +		riscv_v_vstate_save(&prev->thread.kernel_vstate, regs);
> +	} else {
> +		regs = task_pt_regs(prev);
> +		riscv_v_vstate_save(&prev->thread.vstate, regs);
> +	}
> +
> +	if (IS_ENABLED(CONFIG_RISCV_ISA_V_PREEMPTIVE_KMV) &&

Possibly stupid question, but not explained by the patch, why would we
ever want to have RISCV_ISA_V_PREEMPTIVE_KMV disabled?

> +	    test_tsk_thread_flag(next, TIF_RISCV_V_KMV)) {
> +		regs = next->thread.trap_pt_regs;
> +		WARN_ON(!regs);
> +		riscv_v_vstate_restore(&next->thread.kernel_vstate, regs);
> +	} else {
> +		riscv_v_vstate_set_restore(next, task_pt_regs(next));
> +	}
>  }
>  
>  void riscv_v_vstate_ctrl_init(struct task_struct *tsk);
> diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
> index d6a75aac1d27..4b062f7741b2 100644
> --- a/arch/riscv/kernel/asm-offsets.c
> +++ b/arch/riscv/kernel/asm-offsets.c
> @@ -38,6 +38,8 @@ 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_THREAD_TRAP_REGP, task_struct, thread.trap_pt_regs);
> +	OFFSET(TASK_THREAD_VSTATE_CTRL, task_struct, thread.vstate_ctrl);
>  
>  	OFFSET(TASK_THREAD_F0,  task_struct, thread.fstate.f[0]);
>  	OFFSET(TASK_THREAD_F1,  task_struct, thread.fstate.f[1]);
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index 143a2bb3e697..42b80b90626a 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -66,6 +66,27 @@ _save_context:
>  	REG_S s4, PT_CAUSE(sp)
>  	REG_S s5, PT_TP(sp)
>  
> +	/*
> +	 * Reocrd the register set at the frame where in-kernel V registers are

nit: s/Reocrd/Record/

> diff --git a/arch/riscv/kernel/kernel_mode_vector.c b/arch/riscv/kernel/kernel_mode_vector.c
> index 30f1b861cac0..bcd6a69a5266 100644
> --- a/arch/riscv/kernel/kernel_mode_vector.c
> +++ b/arch/riscv/kernel/kernel_mode_vector.c
> @@ -10,6 +10,7 @@
>  #include <linux/percpu.h>
>  #include <linux/preempt.h>
>  #include <linux/types.h>
> +#include <linux/slab.h>
>  
>  #include <asm/vector.h>
>  #include <asm/switch_to.h>
> @@ -35,7 +36,8 @@ static __must_check inline bool may_use_vector(void)
>  	 * where it is set.
>  	 */
>  	return !in_irq() && !irqs_disabled() && !in_nmi() &&
> -	       !this_cpu_read(vector_context_busy);
> +	       !this_cpu_read(vector_context_busy) &&
> +	       !test_thread_flag(TIF_RISCV_V_KMV);
>  }
>  
>  /*
> @@ -69,6 +71,47 @@ static void put_cpu_vector_context(void)
>  	preempt_enable();
>  }
>  
> +#ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE_KMV
> +void riscv_v_vstate_ctrl_config_kmv(bool preemptive_kmv)

I don't understand what this function is trying to do, based on the
function name. The lack of a verb in it is somewhat confusing.

> +{
> +	if (preemptive_kmv)
> +		current->thread.vstate_ctrl |= RISCV_V_VSTATE_CTRL_KMV_PREEMPTIBLE;
> +	else
> +		current->thread.vstate_ctrl &= ~RISCV_V_VSTATE_CTRL_KMV_PREEMPTIBLE;
> +}
> +
> +static bool riscv_v_kmv_preempitble(void)

Beyond the ible/able stuff, there's a typo in this function name.

> +{
> +	return !!(current->thread.vstate_ctrl & RISCV_V_VSTATE_CTRL_KMV_PREEMPTIBLE);
> +}

Little comment on the rest, not qualified to do so :)

Thanks,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

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