| Message ID | 20231221134318.28105-2-andy.chiu@sifive.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: support kernel-mode Vector | expand |
On Thu, Dec 21, 2023 at 01:43:08PM +0000, Andy Chiu wrote: > +/* > + * We use a flag to track in-kernel Vector context. Currently the flag has the > + * following meaning: > + * > + * - bit 0-7 indicates whether the in-kernel Vector context is active. The > + * activation of this state disables the preemption. On a non-RT kernel, it > + * also disable bh. Currently only 0 and 1 are valid value for this field. > + * Other values are reserved for future uses. > + */ > + > +#define RISCV_KERNEL_MODE_V_MASK 0xff > + > +#define RISCV_KERNEL_MODE_V 0x1 Is there a reason this isn't just a single bit flag? > +static inline void riscv_v_ctx_cnt_add(u32 offset) > +{ > + current->thread.riscv_v_flags += offset; > + barrier(); > +} > + > +static inline void riscv_v_ctx_cnt_sub(u32 offset) > +{ > + barrier(); > + current->thread.riscv_v_flags -= offset; > +} What is the purpose of the barriers above? > +static inline u32 riscv_v_ctx_cnt(void) > +{ > + return READ_ONCE(current->thread.riscv_v_flags); > +} What is the purpose of using READ_ONCE() here? > diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c > index 4f21d970a129..5c4dcf518684 100644 > --- a/arch/riscv/kernel/process.c > +++ b/arch/riscv/kernel/process.c > @@ -187,7 +187,6 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) > *dst = *src; > /* clear entire V context, including datap for a new task */ > memset(&dst->thread.vstate, 0, sizeof(struct __riscv_v_ext_state)); > - > return 0; > } Unnecessary whitespace change. Otherwise this patch looks good, thanks! - Eric
On Fri, Dec 22, 2023 at 1:30 PM Eric Biggers <ebiggers@kernel.org> wrote: > > On Thu, Dec 21, 2023 at 01:43:08PM +0000, Andy Chiu wrote: > > +/* > > + * We use a flag to track in-kernel Vector context. Currently the flag has the > > + * following meaning: > > + * > > + * - bit 0-7 indicates whether the in-kernel Vector context is active. The > > + * activation of this state disables the preemption. On a non-RT kernel, it > > + * also disable bh. Currently only 0 and 1 are valid value for this field. > > + * Other values are reserved for future uses. > > + */ > > + > > +#define RISCV_KERNEL_MODE_V_MASK 0xff > > + > > +#define RISCV_KERNEL_MODE_V 0x1 > > Is there a reason this isn't just a single bit flag? I have not yet drawn a conclusion on this. But I am thinking if it would be useful to allow calling kernel_vector_begin multiple times on a call chain. Then these extra bits would be useful if we were to allow and implement it. For example, the use case would be like kernel_vector_begin(); memset(); //chained to another kernel_vector_begin and vectorized memset do_things_with_vector(); kernel_vector_end(); > > > +static inline void riscv_v_ctx_cnt_add(u32 offset) > > +{ > > + current->thread.riscv_v_flags += offset; > > + barrier(); > > +} > > + > > +static inline void riscv_v_ctx_cnt_sub(u32 offset) > > +{ > > + barrier(); > > + current->thread.riscv_v_flags -= offset; > > +} > > What is the purpose of the barriers above? > > > +static inline u32 riscv_v_ctx_cnt(void) > > +{ > > + return READ_ONCE(current->thread.riscv_v_flags); > > +} > > What is the purpose of using READ_ONCE() here? These codes provide compiler barriers, e.g. to prevent riscv_v_flag counting slips into Vector operations. Currently. it should be fine for non-preemptible Vector as riscv_v_ctx_cnt_add()/sub() are guarded with preempt_disable()/local_bh_disable(). However, if preempt_v or the above use-case is concerned, then these compiler barriers are needed and should not be mixed with Vector operations afterward. Or, it would confuse the context tracking when traps take place. riscv_v_ctx_cnt_add() do things with Vector, maybe inlined (vstate_save) riscv_v_ctx_cnt_sub() > > > diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c > > index 4f21d970a129..5c4dcf518684 100644 > > --- a/arch/riscv/kernel/process.c > > +++ b/arch/riscv/kernel/process.c > > @@ -187,7 +187,6 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) > > *dst = *src; > > /* clear entire V context, including datap for a new task */ > > memset(&dst->thread.vstate, 0, sizeof(struct __riscv_v_ext_state)); > > - > > return 0; > > } > > Unnecessary whitespace change. This will be fixed in v8, thanks! > > Otherwise this patch looks good, thanks! > > - Eric Thanks, Andy
On Fri, Dec 22, 2023 at 04:26:03PM +0800, Andy Chiu wrote: > On Fri, Dec 22, 2023 at 1:30 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > > On Thu, Dec 21, 2023 at 01:43:08PM +0000, Andy Chiu wrote: > > > +/* > > > + * We use a flag to track in-kernel Vector context. Currently the flag has the > > > + * following meaning: > > > + * > > > + * - bit 0-7 indicates whether the in-kernel Vector context is active. The > > > + * activation of this state disables the preemption. On a non-RT kernel, it > > > + * also disable bh. Currently only 0 and 1 are valid value for this field. > > > + * Other values are reserved for future uses. > > > + */ > > > + > > > +#define RISCV_KERNEL_MODE_V_MASK 0xff > > > + > > > +#define RISCV_KERNEL_MODE_V 0x1 > > > > Is there a reason this isn't just a single bit flag? > > I have not yet drawn a conclusion on this. But I am thinking if it > would be useful to allow calling kernel_vector_begin multiple times on > a call chain. Then these extra bits would be useful if we were to > allow and implement it. For example, the use case would be like > > kernel_vector_begin(); > memset(); //chained to another kernel_vector_begin and vectorized memset > do_things_with_vector(); > kernel_vector_end(); Maybe it should just be a single bit for now, and it can be changed later if the more complex version actually turns out to be needed? FWIW, the existing architectures don't allow reentrant enabling of SIMD. > > > +static inline void riscv_v_ctx_cnt_add(u32 offset) > > > +{ > > > + current->thread.riscv_v_flags += offset; > > > + barrier(); > > > +} > > > + > > > +static inline void riscv_v_ctx_cnt_sub(u32 offset) > > > +{ > > > + barrier(); > > > + current->thread.riscv_v_flags -= offset; > > > +} > > > > What is the purpose of the barriers above? > > > > > +static inline u32 riscv_v_ctx_cnt(void) > > > +{ > > > + return READ_ONCE(current->thread.riscv_v_flags); > > > +} > > > > What is the purpose of using READ_ONCE() here? > > These codes provide compiler barriers, e.g. to prevent riscv_v_flag > counting slips into Vector operations. Currently. it should be fine > for non-preemptible Vector as riscv_v_ctx_cnt_add()/sub() are guarded > with preempt_disable()/local_bh_disable(). However, if preempt_v or > the above use-case is concerned, then these compiler barriers are > needed and should not be mixed with Vector operations afterward. Or, > it would confuse the context tracking when traps take place. > > riscv_v_ctx_cnt_add() > do things with Vector, maybe inlined (vstate_save) > riscv_v_ctx_cnt_sub() Can you leave comments in the code explaining this? Also, if these barriers aren't needed until preemptible vector support, they probably should be part of that patch, not this one. - Eric
On Sat, Dec 23, 2023 at 11:27 PM Eric Biggers <ebiggers@kernel.org> wrote: > > On Fri, Dec 22, 2023 at 04:26:03PM +0800, Andy Chiu wrote: > > On Fri, Dec 22, 2023 at 1:30 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > On Thu, Dec 21, 2023 at 01:43:08PM +0000, Andy Chiu wrote: > > > > +/* > > > > + * We use a flag to track in-kernel Vector context. Currently the flag has the > > > > + * following meaning: > > > > + * > > > > + * - bit 0-7 indicates whether the in-kernel Vector context is active. The > > > > + * activation of this state disables the preemption. On a non-RT kernel, it > > > > + * also disable bh. Currently only 0 and 1 are valid value for this field. > > > > + * Other values are reserved for future uses. > > > > + */ > > > > + > > > > +#define RISCV_KERNEL_MODE_V_MASK 0xff > > > > + > > > > +#define RISCV_KERNEL_MODE_V 0x1 > > > > > > Is there a reason this isn't just a single bit flag? > > > > I have not yet drawn a conclusion on this. But I am thinking if it > > would be useful to allow calling kernel_vector_begin multiple times on > > a call chain. Then these extra bits would be useful if we were to > > allow and implement it. For example, the use case would be like > > > > kernel_vector_begin(); > > memset(); //chained to another kernel_vector_begin and vectorized memset > > do_things_with_vector(); > > kernel_vector_end(); > > Maybe it should just be a single bit for now, and it can be changed later if the > more complex version actually turns out to be needed? Having more bits allows us to check against overflow [1]. For example, we can detect if people accidentally call riscv_v_ctx_cnt_add() even number of times with a few more bits. Another way to do this is to use test_and_* operation on riscv_v_flags, but then we will have to maintain two kinds of accessors to the flag. E.g. the preempt_v one will still have to do counter add/sub operations on the flag. > > FWIW, the existing architectures don't allow reentrant enabling of SIMD. > > > > > +static inline void riscv_v_ctx_cnt_add(u32 offset) > > > > +{ > > > > + current->thread.riscv_v_flags += offset; > > > > + barrier(); > > > > +} > > > > + > > > > +static inline void riscv_v_ctx_cnt_sub(u32 offset) > > > > +{ > > > > + barrier(); > > > > + current->thread.riscv_v_flags -= offset; > > > > +} > > > > > > What is the purpose of the barriers above? > > > > > > > +static inline u32 riscv_v_ctx_cnt(void) > > > > +{ > > > > + return READ_ONCE(current->thread.riscv_v_flags); > > > > +} > > > > > > What is the purpose of using READ_ONCE() here? > > > > These codes provide compiler barriers, e.g. to prevent riscv_v_flag > > counting slips into Vector operations. Currently. it should be fine > > for non-preemptible Vector as riscv_v_ctx_cnt_add()/sub() are guarded > > with preempt_disable()/local_bh_disable(). However, if preempt_v or > > the above use-case is concerned, then these compiler barriers are > > needed and should not be mixed with Vector operations afterward. Or, > > it would confuse the context tracking when traps take place. > > > > riscv_v_ctx_cnt_add() > > do things with Vector, maybe inlined (vstate_save) > > riscv_v_ctx_cnt_sub() > > Can you leave comments in the code explaining this? > > Also, if these barriers aren't needed until preemptible vector support, they > probably should be part of that patch, not this one. Yes, I think these barriers are not needed until then. I will move them to patch 10 > > - Eric - [1]: https://lore.kernel.org/all/ZXvxIuZwCQ8zeXhr@ghost/
diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h index f19f861cda54..15781e2232e0 100644 --- a/arch/riscv/include/asm/processor.h +++ b/arch/riscv/include/asm/processor.h @@ -73,6 +73,20 @@ struct task_struct; struct pt_regs; +/* + * We use a flag to track in-kernel Vector context. Currently the flag has the + * following meaning: + * + * - bit 0-7 indicates whether the in-kernel Vector context is active. The + * activation of this state disables the preemption. On a non-RT kernel, it + * also disable bh. Currently only 0 and 1 are valid value for this field. + * Other values are reserved for future uses. + */ + +#define RISCV_KERNEL_MODE_V_MASK 0xff + +#define RISCV_KERNEL_MODE_V 0x1 + /* CPU-specific state of a task */ struct thread_struct { /* Callee-saved registers */ @@ -81,7 +95,8 @@ struct thread_struct { unsigned long s[12]; /* s[0]: frame pointer */ struct __riscv_d_ext_state fstate; unsigned long bad_cause; - unsigned long vstate_ctrl; + u32 riscv_v_flags; + u32 vstate_ctrl; struct __riscv_v_ext_state vstate; unsigned long align_ctl; }; diff --git a/arch/riscv/include/asm/simd.h b/arch/riscv/include/asm/simd.h new file mode 100644 index 000000000000..3b603e47c5d8 --- /dev/null +++ b/arch/riscv/include/asm/simd.h @@ -0,0 +1,44 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (C) 2017 Linaro Ltd. <ard.biesheuvel@linaro.org> + * Copyright (C) 2023 SiFive + */ + +#ifndef __ASM_SIMD_H +#define __ASM_SIMD_H + +#include <linux/compiler.h> +#include <linux/irqflags.h> +#include <linux/percpu.h> +#include <linux/preempt.h> +#include <linux/types.h> + +#include <asm/vector.h> + +#ifdef CONFIG_RISCV_ISA_V +/* + * may_use_simd - whether it is allowable at this time to issue vector + * instructions or access the vector register file + * + * Callers must not assume that the result remains true beyond the next + * preempt_enable() or return from softirq context. + */ +static __must_check inline bool may_use_simd(void) +{ + /* + * RISCV_KERNEL_MODE_V is only set while preemption is disabled, + * and is clear whenever preemption is enabled. + */ + return !in_hardirq() && !in_nmi() && !(riscv_v_ctx_cnt() & RISCV_KERNEL_MODE_V_MASK); +} + +#else /* ! CONFIG_RISCV_ISA_V */ + +static __must_check inline bool may_use_simd(void) +{ + return false; +} + +#endif /* ! CONFIG_RISCV_ISA_V */ + +#endif diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h index 87aaef656257..6254830c0668 100644 --- a/arch/riscv/include/asm/vector.h +++ b/arch/riscv/include/asm/vector.h @@ -22,6 +22,27 @@ extern unsigned long riscv_v_vsize; int riscv_v_setup_vsize(void); bool riscv_v_first_use_handler(struct pt_regs *regs); +void kernel_vector_begin(void); +void kernel_vector_end(void); +void get_cpu_vector_context(void); +void put_cpu_vector_context(void); + +static inline void riscv_v_ctx_cnt_add(u32 offset) +{ + current->thread.riscv_v_flags += offset; + barrier(); +} + +static inline void riscv_v_ctx_cnt_sub(u32 offset) +{ + barrier(); + current->thread.riscv_v_flags -= offset; +} + +static inline u32 riscv_v_ctx_cnt(void) +{ + return READ_ONCE(current->thread.riscv_v_flags); +} static __always_inline bool has_vector(void) { diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile index fee22a3d1b53..8c58595696b3 100644 --- a/arch/riscv/kernel/Makefile +++ b/arch/riscv/kernel/Makefile @@ -63,6 +63,7 @@ obj-$(CONFIG_MMU) += vdso.o vdso/ obj-$(CONFIG_RISCV_MISALIGNED) += traps_misaligned.o obj-$(CONFIG_FPU) += fpu.o obj-$(CONFIG_RISCV_ISA_V) += vector.o +obj-$(CONFIG_RISCV_ISA_V) += kernel_mode_vector.o obj-$(CONFIG_SMP) += smpboot.o obj-$(CONFIG_SMP) += smp.o obj-$(CONFIG_SMP) += cpu_ops.o diff --git a/arch/riscv/kernel/kernel_mode_vector.c b/arch/riscv/kernel/kernel_mode_vector.c new file mode 100644 index 000000000000..105147c7d2da --- /dev/null +++ b/arch/riscv/kernel/kernel_mode_vector.c @@ -0,0 +1,95 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2012 ARM Ltd. + * Author: Catalin Marinas <catalin.marinas@arm.com> + * Copyright (C) 2017 Linaro Ltd. <ard.biesheuvel@linaro.org> + * Copyright (C) 2021 SiFive + */ +#include <linux/compiler.h> +#include <linux/irqflags.h> +#include <linux/percpu.h> +#include <linux/preempt.h> +#include <linux/types.h> + +#include <asm/vector.h> +#include <asm/switch_to.h> +#include <asm/simd.h> + +/* + * Claim ownership of the CPU vector context for use by the calling context. + * + * The caller may freely manipulate the vector context metadata until + * put_cpu_vector_context() is called. + */ +void get_cpu_vector_context(void) +{ + preempt_disable(); + + WARN_ON((riscv_v_ctx_cnt() & RISCV_KERNEL_MODE_V_MASK) != 0); + riscv_v_ctx_cnt_add(RISCV_KERNEL_MODE_V); +} + +/* + * Release the CPU vector context. + * + * Must be called from a context in which get_cpu_vector_context() was + * previously called, with no call to put_cpu_vector_context() in the + * meantime. + */ +void put_cpu_vector_context(void) +{ + WARN_ON((riscv_v_ctx_cnt() & RISCV_KERNEL_MODE_V_MASK) != RISCV_KERNEL_MODE_V); + riscv_v_ctx_cnt_sub(RISCV_KERNEL_MODE_V); + + preempt_enable(); +} + +/* + * kernel_vector_begin(): obtain the CPU vector registers for use by the calling + * context + * + * Must not be called unless may_use_simd() returns true. + * Task context in the vector registers is saved back to memory as necessary. + * + * A matching call to kernel_vector_end() must be made before returning from the + * calling context. + * + * The caller may freely use the vector registers until kernel_vector_end() is + * called. + */ +void kernel_vector_begin(void) +{ + if (WARN_ON(!has_vector())) + return; + + BUG_ON(!may_use_simd()); + + get_cpu_vector_context(); + + riscv_v_vstate_save(current, task_pt_regs(current)); + + riscv_v_enable(); +} +EXPORT_SYMBOL_GPL(kernel_vector_begin); + +/* + * kernel_vector_end(): give the CPU vector registers back to the current task + * + * Must be called from a context in which kernel_vector_begin() was previously + * called, with no call to kernel_vector_end() in the meantime. + * + * The caller must not use the vector registers after this function is called, + * unless kernel_vector_begin() is called again in the meantime. + */ +void kernel_vector_end(void) +{ + if (WARN_ON(!has_vector())) + return; + + riscv_v_vstate_restore(current, task_pt_regs(current)); + + riscv_v_disable(); + + put_cpu_vector_context(); +} +EXPORT_SYMBOL_GPL(kernel_vector_end); diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c index 4f21d970a129..5c4dcf518684 100644 --- a/arch/riscv/kernel/process.c +++ b/arch/riscv/kernel/process.c @@ -187,7 +187,6 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) *dst = *src; /* clear entire V context, including datap for a new task */ memset(&dst->thread.vstate, 0, sizeof(struct __riscv_v_ext_state)); - return 0; } @@ -221,6 +220,7 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) childregs->a0 = 0; /* Return value of fork() */ p->thread.s[0] = 0; } + p->thread.riscv_v_flags = 0; p->thread.ra = (unsigned long)ret_from_fork; p->thread.sp = (unsigned long)childregs; /* kernel sp */ return 0;