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

[Eric Biggers]

On Fri, Dec 22, 2023 at 04:26:03PM +0800, Andy Chiu wrote:
> 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();

Maybe it should just be a single bit for now, and it can be changed later if the
more complex version actually turns out to be needed?

FWIW, the existing architectures don't allow reentrant enabling of SIMD.

> > > +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()

Can you leave comments in the code explaining this?

Also, if these barriers aren't needed until preemptible vector support, they
probably should be part of that patch, not this one.

- Eric