Message-ID: <CABgGipXgDKHPEBLT_e0DgHTo0WLwNxf5SD4sTDwmTHVj6Wz5VA@mail.gmail.com>
Date:   Fri, 14 Jul 2023 01:19:47 +0800
From:   Andy Chiu <andy.chiu@...ive.com>
To:     Rémi Denis-Courmont <remi@...lab.net>
Cc:     linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 01/12] riscv: Add support for kernel mode vector

Hi, Heiko

On Wed, Jul 12, 2023 at 1:15 AM Rémi Denis-Courmont <remi@...lab.net> wrote:
>
>         Hi,
>
> Le tiistaina 11. heinäkuuta 2023, 18.37.32 EEST Heiko Stuebner a écrit :
> > From: Greentime Hu <greentime.hu@...ive.com>
> >
> > 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 <vincent.chen@...ive.com>
> > Signed-off-by: Vincent Chen <vincent.chen@...ive.com>
> > Signed-off-by: Greentime Hu <greentime.hu@...ive.com>
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@...ll.eu>
> > ---
> >  arch/riscv/include/asm/vector.h        |  17 ++++
> >  arch/riscv/kernel/Makefile             |   1 +
> >  arch/riscv/kernel/kernel_mode_vector.c | 132 +++++++++++++++++++++++++
> >  3 files changed, 150 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 3d78930cab51..ac2c23045eec 100644
> > --- a/arch/riscv/include/asm/vector.h
> > +++ b/arch/riscv/include/asm/vector.h
> > @@ -196,6 +196,23 @@ static inline void __switch_to_vector(struct
> > task_struct *prev, void riscv_v_vstate_ctrl_init(struct task_struct *tsk);
> >  bool riscv_v_vstate_ctrl_user_allowed(void);
> >
> > +static inline void riscv_v_flush_cpu_state(void)
> > +{
> > +     asm volatile (
> > +             ".option push\n\t"
> > +             ".option arch, +v\n\t"
> > +             "vsetvli        t0, x0, e8, m8, ta, ma\n\t"
> > +             "vmv.v.i        v0, 0\n\t"
> > +             "vmv.v.i        v8, 0\n\t"
> > +             "vmv.v.i        v16, 0\n\t"
> > +             "vmv.v.i        v24, 0\n\t"
> > +             ".option pop\n\t"
> > +             : : : "t0");
>
> Why bother with zeroing out the vectors before kernel use? That sounds like it
> will only hide bugs in kernel code - implicitly assuming that everything is
> initially zero. Ditto initialising the vector configuration; if you really want
> to have a fixed initial value rather than "leak" whatever user set, better use
> an invalid configuration (vill=1), IMO.

Yes, I agree that we don't have to zero out (or invalid) v registers
before any kernel uses. And we only have to restore user's v registers
once, before really returning back to the user space. Actually I am
going to send out the series (for kernel-mode vector) having these
improvements in a few days. Does it seem ok to you to drop the first
two patches and rebase on top of mine at the next respin for the
crypto vector? Or is there any good way that you can think of to let
us cooperate on this?

>
> > +}
> > +
> > +void kernel_rvv_begin(void);
> > +void kernel_rvv_end(void);
> > +
> >  #else /* ! CONFIG_RISCV_ISA_V  */
> >
> >  struct pt_regs;
> > diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> > index 506cc4a9a45a..3f4435746af7 100644
> > --- a/arch/riscv/kernel/Makefile
> > +++ b/arch/riscv/kernel/Makefile
> > @@ -61,6 +61,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..2d704190c054
> > --- /dev/null
> > +++ b/arch/riscv/kernel/kernel_mode_vector.c
> > @@ -0,0 +1,132 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2012 ARM Ltd.
> > + * Author: Catalin Marinas <catalin.marinas@....com>
> > + * Copyright (C) 2017 Linaro Ltd. <ard.biesheuvel@...aro.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>
> > +
> > +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.
> > + */
> > +void kernel_rvv_begin(void)
> > +{
> > +     if (WARN_ON(!has_vector()))
> > +             return;
> > +
> > +     WARN_ON(!may_use_vector());
> > +
> > +     /* Acquire kernel mode vector */
> > +     get_cpu_vector_context();
> > +
> > +     /* Save vector state, if any */
> > +     riscv_v_vstate_save(current, task_pt_regs(current));
> > +
> > +     /* Enable vector */
> > +     riscv_v_enable();
> > +
> > +     /* Invalidate vector regs */
> > +     riscv_v_flush_cpu_state();
> > +}
> > +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;
> > +
> > +     /* Invalidate vector regs */
> > +     riscv_v_flush_cpu_state();
> > +
> > +     /* Restore vector state, if any */
> > +     riscv_v_vstate_restore(current, task_pt_regs(current));
>
> I thought that the kernel was already nuking user vectors on every system
> call, since the RVV spec says so.
>
> Are you trying to use vectors from interrupts? Otherwise, isn't this flush &
> restore superfluous?
>
> > +
> > +     /* disable vector */
> > +     riscv_v_disable();
> > +
> > +     /* release kernel mode vector */
> > +     put_cpu_vector_context();
> > +}
> > +EXPORT_SYMBOL_GPL(kernel_rvv_end);
>
>
> --
> 雷米‧德尼-库尔蒙
> http://www.remlab.net/
>
>
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Thanks.

Andy

Powered by blists - more mailing lists