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

[Andy Chiu]

On Sat, Dec 23, 2023 at 11:27 PM Eric Biggers <ebiggers at kernel.org> wrote:
>
> 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?

Having more bits allows us to check against overflow [1]. For example,
we can detect if people accidentally call riscv_v_ctx_cnt_add() even
number of times with a few more bits. Another way to do this is to use
test_and_* operation on riscv_v_flags, but then we will have to
maintain two kinds of accessors to the flag. E.g. the preempt_v one
will still have to do counter add/sub operations on the flag.

>
> 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.

Yes, I think these barriers are not needed until then. I will move
them to patch 10

>
> - Eric

- [1]: https://lore.kernel.org/all/ZXvxIuZwCQ8zeXhr@ghost/