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

[Andy Chiu]

On Mon, Jul 17, 2023 at 6:23 PM Conor Dooley <conor.dooley at microchip.com> wrote:
>
> On Sat, Jul 15, 2023 at 03:00:28PM +0000, Andy Chiu wrote:
> > From: Greentime Hu <greentime.hu at sifive.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 at sifive.com>
> > Signed-off-by: Vincent Chen <vincent.chen at sifive.com>
> > Signed-off-by: Greentime Hu <greentime.hu at sifive.com>
> > Signed-off-by: Andy Chiu <andy.chiu at sifive.com>
> > ---
> >  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