[v1,2/6] riscv: Add support for kernel mode vector

Commit Message

Andy Chiu July 15, 2023, 3 p.m. UTC

From: Greentime Hu <[e-mail]>

Add kernel_rvv_begin() and kernel_rvv_end() function declarations
and corresponding definitions in kernel_mode_vector.c

These are needed to wrap uses of vector in kernel mode.

Co-developed-by: Vincent Chen <[e-mail]>
Signed-off-by: Vincent Chen <[e-mail]>
Signed-off-by: Greentime Hu <[e-mail]>
Signed-off-by: Andy Chiu <[e-mail]>
---
 arch/riscv/include/asm/vector.h        |   2 +
 arch/riscv/kernel/Makefile             |   1 +
 arch/riscv/kernel/kernel_mode_vector.c | 129 +++++++++++++++++++++++++
 3 files changed, 132 insertions(+)
 create mode 100644 arch/riscv/kernel/kernel_mode_vector.c

Comments

Conor Dooley July 17, 2023, 10:22 a.m. UTC | #1

On Sat, Jul 15, 2023 at 03:00:28PM +0000, Andy Chiu wrote:
> From: Greentime Hu <[e-mail]>
> 
> Add kernel_rvv_begin() and kernel_rvv_end() function declarations
> and corresponding definitions in kernel_mode_vector.c
> 
> These are needed to wrap uses of vector in kernel mode.
> 
> Co-developed-by: Vincent Chen <[e-mail]>
> Signed-off-by: Vincent Chen <[e-mail]>
> Signed-off-by: Greentime Hu <[e-mail]>
> Signed-off-by: Andy Chiu <[e-mail]>
> ---
>  arch/riscv/include/asm/vector.h        |   2 +
>  arch/riscv/kernel/Makefile             |   1 +
>  arch/riscv/kernel/kernel_mode_vector.c | 129 +++++++++++++++++++++++++
>  3 files changed, 132 insertions(+)
>  create mode 100644 arch/riscv/kernel/kernel_mode_vector.c
> 
> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> index a4f3705fd144..9831b19153ae 100644
> --- a/arch/riscv/include/asm/vector.h
> +++ b/arch/riscv/include/asm/vector.h
> @@ -22,6 +22,8 @@
>  extern unsigned long riscv_v_vsize;
>  int riscv_v_setup_vsize(void);
>  bool riscv_v_first_use_handler(struct pt_regs *regs);
> +int kernel_rvv_begin(void);
> +void kernel_rvv_end(void);

So, we ditched all of the "rvv" stuff in the last series, using either
"vector" - has_vector() - or "riscv_v". I'd rather not introduce a third
naming scheme for vector related things...

Given what you add below is full of other things that use "vector", how
does s/rvv/vector/ sound here?


> diff --git a/arch/riscv/kernel/kernel_mode_vector.c b/arch/riscv/kernel/kernel_mode_vector.c
> new file mode 100644
> index 000000000000..c0c152c501a5
> --- /dev/null
> +++ b/arch/riscv/kernel/kernel_mode_vector.c

> +/*
> + * kernel_rvv_begin(): obtain the CPU vector registers for use by the calling
> + * context
> + *
> + * Must not be called unless may_use_vector() returns true.
> + * Task context in the vector registers is saved back to memory as necessary.
> + *
> + * A matching call to kernel_rvv_end() must be made before returning from the
> + * calling context.
> + *
> + * The caller may freely use the vector registers until kernel_rvv_end() is
> + * called.
> + */
> +int kernel_rvv_begin(void)

How come this returns an int, but you never actually check the result? The
other kernel_*_begin()s don't seem to return anything other than void.

> +{
> +	if (!has_vector())
> +		return -EOPNOTSUPP;
> +
> +	if (!may_use_vector())
> +		return -EPERM;

I notice arm64 takes a stronger approach. There, if the has() call
fails, it has a WARN_ON(). For the !may_use() case, it BUG()s.
Since users are forced to check may_use_vector() before calling
kernel_rvv_begin().

Is there a reason that we should not take the same approach?

> +
> +	/* Save vector state, if any */
> +	riscv_v_vstate_save(current, task_pt_regs(current));
> +
> +	/* Acquire kernel mode vector */
> +	get_cpu_vector_context();
> +
> +	/* Enable vector */

These three comments are mostly a statement of the obvious, no?

> +	riscv_v_enable();
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(kernel_rvv_begin);
> +
> +/*
> + * kernel_rvv_end(): give the CPU vector registers back to the current task
> + *
> + * Must be called from a context in which kernel_rvv_begin() was previously
> + * called, with no call to kernel_rvv_end() in the meantime.
> + *
> + * The caller must not use the vector registers after this function is called,
> + * unless kernel_rvv_begin() is called again in the meantime.
> + */
> +void kernel_rvv_end(void)
> +{
> +	if (WARN_ON(!has_vector()))

But there is a WARN_ON() here...

> +		return;
> +
> +	/* Restore vector state, if any */
> +	riscv_v_vstate_set_restore(current, task_pt_regs(current));
> +
> +	/* disable vector */
> +	riscv_v_disable();
> +
> +	/* release kernel mode vector */

Again, comments kinda state the obvious, no?

Otherwise, this stuff looks generally fine to me & similar to what is
being done elsewhere.

Thanks,
Conor.

Andy Chiu July 20, 2023, 2:54 p.m. UTC | #2

On Mon, Jul 17, 2023 at 6:23 PM Conor Dooley <[e-mail]> wrote:
>
> On Sat, Jul 15, 2023 at 03:00:28PM +0000, Andy Chiu wrote:
> > From: Greentime Hu <[e-mail]>
> >
> > Add kernel_rvv_begin() and kernel_rvv_end() function declarations
> > and corresponding definitions in kernel_mode_vector.c
> >
> > These are needed to wrap uses of vector in kernel mode.
> >
> > Co-developed-by: Vincent Chen <[e-mail]>
> > Signed-off-by: Vincent Chen <[e-mail]>
> > Signed-off-by: Greentime Hu <[e-mail]>
> > Signed-off-by: Andy Chiu <[e-mail]>
> > ---
> >  arch/riscv/include/asm/vector.h        |   2 +
> >  arch/riscv/kernel/Makefile             |   1 +
> >  arch/riscv/kernel/kernel_mode_vector.c | 129 +++++++++++++++++++++++++
> >  3 files changed, 132 insertions(+)
> >  create mode 100644 arch/riscv/kernel/kernel_mode_vector.c
> >
> > diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> > index a4f3705fd144..9831b19153ae 100644
> > --- a/arch/riscv/include/asm/vector.h
> > +++ b/arch/riscv/include/asm/vector.h
> > @@ -22,6 +22,8 @@
> >  extern unsigned long riscv_v_vsize;
> >  int riscv_v_setup_vsize(void);
> >  bool riscv_v_first_use_handler(struct pt_regs *regs);
> > +int kernel_rvv_begin(void);
> > +void kernel_rvv_end(void);
>
> So, we ditched all of the "rvv" stuff in the last series, using either
> "vector" - has_vector() - or "riscv_v". I'd rather not introduce a third
> naming scheme for vector related things...
>
> Given what you add below is full of other things that use "vector", how
> does s/rvv/vector/ sound here?

Yes, I agree. Let's use 'vector' instead of rvv.

>
>
> > diff --git a/arch/riscv/kernel/kernel_mode_vector.c b/arch/riscv/kernel/kernel_mode_vector.c
> > new file mode 100644
> > index 000000000000..c0c152c501a5
> > --- /dev/null
> > +++ b/arch/riscv/kernel/kernel_mode_vector.c
>
> > +/*
> > + * kernel_rvv_begin(): obtain the CPU vector registers for use by the calling
> > + * context
> > + *
> > + * Must not be called unless may_use_vector() returns true.
> > + * Task context in the vector registers is saved back to memory as necessary.
> > + *
> > + * A matching call to kernel_rvv_end() must be made before returning from the
> > + * calling context.
> > + *
> > + * The caller may freely use the vector registers until kernel_rvv_end() is
> > + * called.
> > + */
> > +int kernel_rvv_begin(void)
>
> How come this returns an int, but you never actually check the result? The
> other kernel_*_begin()s don't seem to return anything other than void.
>
> > +{
> > +     if (!has_vector())
> > +             return -EOPNOTSUPP;
> > +
> > +     if (!may_use_vector())
> > +             return -EPERM;
>
> I notice arm64 takes a stronger approach. There, if the has() call
> fails, it has a WARN_ON(). For the !may_use() case, it BUG()s.
> Since users are forced to check may_use_vector() before calling
> kernel_rvv_begin().
>
> Is there a reason that we should not take the same approach?

Yes, I agree that it is better to return nothing after some thinking.
Originally I was hoping to return a failure code if the preemptible
kernel-mode Vector failed to allocate memory in
riscv_v_start_kernel_context(). However, returning things here just
complicates the programming model for kernel-mode Vector. So, let's
just make it transparent to users of kernel_vector_begin() and return
nothing here.

>
> > +
> > +     /* Save vector state, if any */
> > +     riscv_v_vstate_save(current, task_pt_regs(current));
> > +
> > +     /* Acquire kernel mode vector */
> > +     get_cpu_vector_context();
> > +
> > +     /* Enable vector */
>
> These three comments are mostly a statement of the obvious, no?

Agree, I will drop those comments.

>
> > +     riscv_v_enable();
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(kernel_rvv_begin);
> > +
> > +/*
> > + * kernel_rvv_end(): give the CPU vector registers back to the current task
> > + *
> > + * Must be called from a context in which kernel_rvv_begin() was previously
> > + * called, with no call to kernel_rvv_end() in the meantime.
> > + *
> > + * The caller must not use the vector registers after this function is called,
> > + * unless kernel_rvv_begin() is called again in the meantime.
> > + */
> > +void kernel_rvv_end(void)
> > +{
> > +     if (WARN_ON(!has_vector()))
>
> But there is a WARN_ON() here...
>
> > +             return;
> > +
> > +     /* Restore vector state, if any */
> > +     riscv_v_vstate_set_restore(current, task_pt_regs(current));
> > +
> > +     /* disable vector */
> > +     riscv_v_disable();
> > +
> > +     /* release kernel mode vector */
>
> Again, comments kinda state the obvious, no?
>
> Otherwise, this stuff looks generally fine to me & similar to what is
> being done elsewhere.
>
> Thanks,
> Conor.

Thanks,
Andy

Patch

diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
index a4f3705fd144..9831b19153ae 100644
--- a/arch/riscv/include/asm/vector.h
+++ b/arch/riscv/include/asm/vector.h
@@ -22,6 +22,8 @@ 
 extern unsigned long riscv_v_vsize;
 int riscv_v_setup_vsize(void);
 bool riscv_v_first_use_handler(struct pt_regs *regs);
+int kernel_rvv_begin(void);
+void kernel_rvv_end(void);
 
 static __always_inline bool has_vector(void)
 {
diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
index a42951911067..b954bbf17c84 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -57,6 +57,7 @@  obj-$(CONFIG_MMU) += vdso.o vdso/
 obj-$(CONFIG_RISCV_M_MODE)	+= 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..c0c152c501a5
--- /dev/null
+++ b/arch/riscv/kernel/kernel_mode_vector.c
@@ -0,0 +1,129 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2012 ARM Ltd.
+ * Author: Catalin Marinas <[e-mail]>
+ * Copyright (C) 2017 Linaro Ltd. <[e-mail]>
+ * 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>
+
+DECLARE_PER_CPU(bool, vector_context_busy);
+DEFINE_PER_CPU(bool, vector_context_busy);
+
+/*
+ * may_use_vector - 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_vector(void)
+{
+	/*
+	 * vector_context_busy is only set while preemption is disabled,
+	 * and is clear whenever preemption is enabled. Since
+	 * this_cpu_read() is atomic w.r.t. preemption, vector_context_busy
+	 * cannot change under our feet -- if it's set we cannot be
+	 * migrated, and if it's clear we cannot be migrated to a CPU
+	 * where it is set.
+	 */
+	return !in_irq() && !irqs_disabled() && !in_nmi() &&
+	       !this_cpu_read(vector_context_busy);
+}
+
+/*
+ * 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.
+ */
+static void get_cpu_vector_context(void)
+{
+	bool busy;
+
+	preempt_disable();
+	busy = __this_cpu_xchg(vector_context_busy, true);
+
+	WARN_ON(busy);
+}
+
+/*
+ * 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.
+ */
+static void put_cpu_vector_context(void)
+{
+	bool busy = __this_cpu_xchg(vector_context_busy, false);
+
+	WARN_ON(!busy);
+	preempt_enable();
+}
+
+/*
+ * kernel_rvv_begin(): obtain the CPU vector registers for use by the calling
+ * context
+ *
+ * Must not be called unless may_use_vector() returns true.
+ * Task context in the vector registers is saved back to memory as necessary.
+ *
+ * A matching call to kernel_rvv_end() must be made before returning from the
+ * calling context.
+ *
+ * The caller may freely use the vector registers until kernel_rvv_end() is
+ * called.
+ */
+int kernel_rvv_begin(void)
+{
+	if (!has_vector())
+		return -EOPNOTSUPP;
+
+	if (!may_use_vector())
+		return -EPERM;
+
+	/* Save vector state, if any */
+	riscv_v_vstate_save(current, task_pt_regs(current));
+
+	/* Acquire kernel mode vector */
+	get_cpu_vector_context();
+
+	/* Enable vector */
+	riscv_v_enable();
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(kernel_rvv_begin);
+
+/*
+ * kernel_rvv_end(): give the CPU vector registers back to the current task
+ *
+ * Must be called from a context in which kernel_rvv_begin() was previously
+ * called, with no call to kernel_rvv_end() in the meantime.
+ *
+ * The caller must not use the vector registers after this function is called,
+ * unless kernel_rvv_begin() is called again in the meantime.
+ */
+void kernel_rvv_end(void)
+{
+	if (WARN_ON(!has_vector()))
+		return;
+
+	/* Restore vector state, if any */
+	riscv_v_vstate_set_restore(current, task_pt_regs(current));
+
+	/* disable vector */
+	riscv_v_disable();
+
+	/* release kernel mode vector */
+	put_cpu_vector_context();
+}
+EXPORT_SYMBOL_GPL(kernel_rvv_end);