LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Greentime Hu <greentime.hu@sifive.com>
To: Darius Rad <darius@bluespec.com>
Cc: linux-riscv <linux-riscv@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Vincent Chen <vincent.chen@sifive.com>
Subject: Re: [RFC PATCH v8 09/21] riscv: Add task switch support for vector
Date: Tue, 28 Sep 2021 22:56:52 +0800	[thread overview]
Message-ID: <CAHCEehJ1uuSFqFWreS1kh+fR5GFfnEycdLMe+4FHXpFsHrRUvA@mail.gmail.com> (raw)
In-Reply-To: <c81267bc-4c91-3978-8c1f-78925c46c78b@bluespec.com>

Darius Rad <darius@bluespec.com> 於 2021年9月13日 週一 下午8:21寫道:
>
> On 9/8/21 1:45 PM, Greentime Hu wrote:
> > This patch adds task switch support for vector. It supports partial lazy
> > save and restore mechanism. It also supports all lengths of vlen.
> >
> > [guoren@linux.alibaba.com: First available porting to support vector
> > context switching]
> > [nick.knight@sifive.com: Rewrite vector.S to support dynamic vlen, xlen and
> > code refine]
> > [vincent.chen@sifive.co: Fix the might_sleep issue in vstate_save,
> > vstate_restore]
> > Co-developed-by: Nick Knight <nick.knight@sifive.com>
> > Signed-off-by: Nick Knight <nick.knight@sifive.com>
> > Co-developed-by: Guo Ren <guoren@linux.alibaba.com>
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > Co-developed-by: Vincent Chen <vincent.chen@sifive.com>
> > Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> > Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> > ---
> >   arch/riscv/include/asm/switch_to.h | 66 +++++++++++++++++++++++
> >   arch/riscv/kernel/Makefile         |  1 +
> >   arch/riscv/kernel/process.c        | 38 ++++++++++++++
> >   arch/riscv/kernel/vector.S         | 84 ++++++++++++++++++++++++++++++
> >   4 files changed, 189 insertions(+)
> >   create mode 100644 arch/riscv/kernel/vector.S
> >
> > diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h
> > index ec83770b3d98..de0573dad78f 100644
> > --- a/arch/riscv/include/asm/switch_to.h
> > +++ b/arch/riscv/include/asm/switch_to.h
> > @@ -7,10 +7,12 @@
> >   #define _ASM_RISCV_SWITCH_TO_H
> >
> >   #include <linux/jump_label.h>
> > +#include <linux/slab.h>
> >   #include <linux/sched/task_stack.h>
> >   #include <asm/processor.h>
> >   #include <asm/ptrace.h>
> >   #include <asm/csr.h>
> > +#include <asm/asm-offsets.h>
> >
> >   #ifdef CONFIG_FPU
> >   extern void __fstate_save(struct task_struct *save_to);
> > @@ -68,6 +70,68 @@ static __always_inline bool has_fpu(void) { return false; }
> >   #define __switch_to_fpu(__prev, __next) do { } while (0)
> >   #endif
> >
> > +#ifdef CONFIG_VECTOR
> > +extern bool has_vector;
> > +extern unsigned long riscv_vsize;
> > +extern void __vstate_save(struct __riscv_v_state *save_to, void *datap);
> > +extern void __vstate_restore(struct __riscv_v_state *restore_from, void *datap);
> > +
> > +static inline void __vstate_clean(struct pt_regs *regs)
> > +{
> > +     regs->status = (regs->status & ~(SR_VS)) | SR_VS_CLEAN;
> > +}
> > +
> > +static inline void vstate_off(struct task_struct *task,
> > +                           struct pt_regs *regs)
> > +{
> > +     regs->status = (regs->status & ~SR_VS) | SR_VS_OFF;
> > +}
> > +
> > +static inline void vstate_save(struct task_struct *task,
> > +                            struct pt_regs *regs)
> > +{
> > +     if ((regs->status & SR_VS) == SR_VS_DIRTY) {
> > +             struct __riscv_v_state *vstate = &(task->thread.vstate);
> > +
> > +             __vstate_save(vstate, vstate->datap);
> > +             __vstate_clean(regs);
> > +     }
> > +}
> > +
> > +static inline void vstate_restore(struct task_struct *task,
> > +                               struct pt_regs *regs)
> > +{
> > +     if ((regs->status & SR_VS) != SR_VS_OFF) {
> > +             struct __riscv_v_state *vstate = &(task->thread.vstate);
> > +
> > +             /* Allocate space for vector registers. */
> > +             if (!vstate->datap) {
> > +                     vstate->datap = kzalloc(riscv_vsize, GFP_ATOMIC);
> > +                     vstate->size = riscv_vsize;
> > +             }
> > +             __vstate_restore(vstate, vstate->datap);
> > +             __vstate_clean(regs);
> > +     }
> > +}
> > +
> > +static inline void __switch_to_vector(struct task_struct *prev,
> > +                                struct task_struct *next)
> > +{
> > +     struct pt_regs *regs;
> > +
> > +     regs = task_pt_regs(prev);
> > +     if (unlikely(regs->status & SR_SD))
> > +             vstate_save(prev, regs);
> > +     vstate_restore(next, task_pt_regs(next));
> > +}
> > +
> > +#else
> > +#define has_vector false
> > +#define vstate_save(task, regs) do { } while (0)
> > +#define vstate_restore(task, regs) do { } while (0)
> > +#define __switch_to_vector(__prev, __next) do { } while (0)
> > +#endif
> > +
> >   extern struct task_struct *__switch_to(struct task_struct *,
> >                                      struct task_struct *);
> >
> > @@ -77,6 +141,8 @@ do {                                                       \
> >       struct task_struct *__next = (next);            \
> >       if (has_fpu())                                  \
> >               __switch_to_fpu(__prev, __next);        \
> > +     if (has_vector)                                 \
> > +             __switch_to_vector(__prev, __next);     \
> >       ((last) = __switch_to(__prev, __next));         \
> >   } while (0)
> >
> > diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> > index 3397ddac1a30..344078080839 100644
> > --- a/arch/riscv/kernel/Makefile
> > +++ b/arch/riscv/kernel/Makefile
> > @@ -40,6 +40,7 @@ obj-$(CONFIG_MMU) += vdso.o vdso/
> >
> >   obj-$(CONFIG_RISCV_M_MODE)  += traps_misaligned.o
> >   obj-$(CONFIG_FPU)           += fpu.o
> > +obj-$(CONFIG_VECTOR)         += vector.o
> >   obj-$(CONFIG_SMP)           += smpboot.o
> >   obj-$(CONFIG_SMP)           += smp.o
> >   obj-$(CONFIG_SMP)           += cpu_ops.o
> > diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
> > index 03ac3aa611f5..0b86e9e531c9 100644
> > --- a/arch/riscv/kernel/process.c
> > +++ b/arch/riscv/kernel/process.c
> > @@ -95,6 +95,16 @@ void start_thread(struct pt_regs *regs, unsigned long pc,
> >                */
> >               fstate_restore(current, regs);
> >       }
> > +
> > +     if (has_vector) {
> > +             regs->status |= SR_VS_INITIAL;
> > +             /*
> > +              * Restore the initial value to the vector register
> > +              * before starting the user program.
> > +              */
> > +             vstate_restore(current, regs);
> > +     }
> > +
>
> So this will unconditionally enable vector instructions, and allocate
> memory for vector state, for all processes, regardless of whether vector
> instructions are used?
>

Hi Darius,

Yes, it will enable vector if has_vector() is true. The reason that we
choose to enable and allocate memory for user space program is because
we also implement some common functions in the glibc such as memcpy
vector version and it is called very often by every process. So that
we assume if the user program is running in a CPU with vector ISA
would like to use vector by default. If we disable it by default and
make it trigger the illegal instruction, that might be a burden since
almost every process will use vector glibc memcpy or something like
that.

> Given the size of the vector state and potential power and performance
> implications of enabling the vector engine, it seems like this should
> treated similarly to Intel AMX on x86.  The full discussion of that is
> here:
>
> https://lore.kernel.org/lkml/CALCETrW2QHa2TLvnUuVxAAheqcbSZ-5_WRXtDSAGcbG8N+gtdQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org/
>
> The cover letter for recent Intel AMX patches has a summary of the x86
> implementation:
>
> https://lore.kernel.org/lkml/20210825155413.19673-1-chang.seok.bae@intel.com/
>
> If RISC-V were to adopt a similar approach, I think the significant
> points are:
>
>   1. A process (or thread) must specifically request the desire to use
> vector extensions (perhaps with some new arch_prctl() API),
>
>   2. The kernel is free to deny permission, perhaps based on
> administrative rules or for other reasons, and
>
>   3. If a process attempts to use vector extensions before doing the
> above, the process will die due to an illegal instruction.

Thank you for sharing this, but I am not sure if we should treat
vector like AMX on x86. IMHO, compiler might generate code with vector
instructions automatically someday, maybe we should treat vector
extensions like other extensions.
If user knows the vector extension is supported in this CPU and he
would like to use it, it seems we should let user use it directly just
like other extensions.
If user don't know it exists or not, user should use the library API
transparently and let glibc or other library deal with it. The glibc
ifunc feature or multi-lib should be able to choose the correct
implementation.

  reply	other threads:[~2021-09-28 14:57 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-08 17:45 [RFC PATCH v8 00/21] riscv: Add vector ISA support Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 01/21] riscv: Separate patch for cflags and aflags Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 02/21] riscv: Rename __switch_to_aux -> fpu Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 03/21] riscv: Extending cpufeature.c to detect V-extension Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 04/21] riscv: Add new csr defines related to vector extension Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 05/21] riscv: Add vector feature to compile Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 06/21] riscv: Add has_vector/riscv_vsize to save vector features Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 07/21] riscv: Reset vector register Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 08/21] riscv: Add vector struct and assembler definitions Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 09/21] riscv: Add task switch support for vector Greentime Hu
2021-09-13 12:21   ` Darius Rad
2021-09-28 14:56     ` Greentime Hu [this message]
2021-09-29 13:28       ` Darius Rad
2021-10-01  2:46         ` Ley Foon Tan
2021-10-04 12:41           ` Greentime Hu
2021-10-05  2:12             ` Ley Foon Tan
2021-10-05 15:46               ` Greentime Hu
2021-10-07 10:10                 ` Ley Foon Tan
2021-10-04 12:36         ` Greentime Hu
2021-10-05 13:57           ` Darius Rad
2021-10-21  1:01             ` Paul Walmsley
2021-10-21 10:50               ` Darius Rad
2021-10-22  3:52                 ` Vincent Chen
2021-10-22 10:40                   ` Darius Rad
2021-10-25  4:47                     ` Greentime Hu
2021-10-25 16:22                       ` Darius Rad
2021-10-26  4:44                         ` Greentime Hu
2021-10-27 12:58                           ` Darius Rad
2021-11-09  9:49                             ` Greentime Hu
2021-11-09 19:21                               ` Darius Rad
2021-10-26 14:58                     ` Heiko Stübner
2021-09-08 17:45 ` [RFC PATCH v8 10/21] riscv: Add ptrace vector support Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 11/21] riscv: Add sigcontext save/restore for vector Greentime Hu
2021-09-30  2:37   ` Ley Foon Tan
2021-09-08 17:45 ` [RFC PATCH v8 12/21] riscv: signal: Report signal frame size to userspace via auxv Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 13/21] riscv: Add support for kernel mode vector Greentime Hu
2021-09-09  6:17   ` Christoph Hellwig
2021-09-08 17:45 ` [RFC PATCH v8 14/21] riscv: Use CSR_STATUS to replace sstatus in vector.S Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 15/21] riscv: Add vector extension XOR implementation Greentime Hu
2021-09-09  6:12   ` Christoph Hellwig
2021-09-28  7:00     ` Greentime Hu
2021-09-14  8:29   ` Ley Foon Tan
2021-09-28  7:01     ` Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 16/21] riscv: Initialize vector registers with proper vsetvli then it can work normally Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 17/21] riscv: Optimize vector registers initialization Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 18/21] riscv: Fix an illegal instruction exception when accessing vlenb without enable vector first Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 19/21] riscv: Allocate space for vector registers in start_thread() Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 20/21] riscv: Optimize task switch codes of vector Greentime Hu
2021-09-15 14:29   ` Jisheng Zhang
2021-10-04 14:13     ` Greentime Hu
2021-09-08 17:45 ` [RFC PATCH v8 21/21] riscv: Turn has_vector into a static key if VECTOR=y Greentime Hu
2021-09-15 14:24   ` Jisheng Zhang
2021-10-04 15:04     ` Greentime Hu
2021-09-13  1:47 ` [RFC PATCH v8 00/21] riscv: Add vector ISA support Vincent Chen
2021-09-13 17:18 ` Vineet Gupta

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAHCEehJ1uuSFqFWreS1kh+fR5GFfnEycdLMe+4FHXpFsHrRUvA@mail.gmail.com \
    --to=greentime.hu@sifive.com \
    --cc=aou@eecs.berkeley.edu \
    --cc=darius@bluespec.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=palmer@dabbelt.com \
    --cc=paul.walmsley@sifive.com \
    --cc=vincent.chen@sifive.com \
    --subject='Re: [RFC PATCH v8 09/21] riscv: Add task switch support for vector' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).