[v7, 01/10] riscv: Add support for kernel mode vector

[Andy Chiu]

On Fri, Dec 22, 2023 at 1:30 PM Eric Biggers <ebiggers at 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