On Wed, 23 Aug 2023, Andy Chiu wrote:
> > > diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h
> > > index e17c550986a6..283800130614 100644
> > > --- a/arch/riscv/include/uapi/asm/ptrace.h
> > > +++ b/arch/riscv/include/uapi/asm/ptrace.h
> > > @@ -97,6 +97,7 @@ struct __riscv_v_ext_state {
> > > unsigned long vl;
> > > unsigned long vtype;
> > > unsigned long vcsr;
> > > + unsigned long vlenb;
> > > void *datap;
> >
> > I think we really ought to make a distinct structure holding the vector
> > CSR state only, and then have it included as a leading member of a pair of
> > other structures, one for the signal context with a trailing `datap' (or
> > `vregp' or `vreg') member and another one for the regset with a flexible
> > array member of the `char' type, e.g. (actual names TBD):
> >
> > struct __riscv_v_csr_state {
> > unsigned long vstart;
> > unsigned long vl;
> > unsigned long vtype;
> > unsigned long vcsr;
> > unsigned long vlenb;
> > };
> >
> > struct __riscv_v_signal_state {
> > struct __riscv_v_csr_state csr;
> > void *vregp;
> > };
> >
> > struct __riscv_v_regset_state {
> > struct __riscv_v_csr_state csr;
> > char vreg[];
> > };
> >
> > This will make the API cleaner and avoid both UB with making accesses
> > beyond the end of a structure and clutter with an unused entry in core
> > files and data exchanged via ptrace(2).
>
> Yes, and may I understand why there is a need for having struct
> __riscv_v_csr_state? Unless there is a need for getting CSRs only, yet
> vector CSRs are not meaningful without the content of Vector
> registers.
Well, it's a data type only, it doesn't *have* to be used on it's own
just because it exists.
> Personally I'd like to have one universal structure for
> both ptrace/signal/context-swicth(internal to the kernel), or one for
> UAPI and the other for kernel internal-used. Because then we don't
> have to mess with all kinds of access helpers for similar things.
I'm not sure what kind of access helpers you mean, please elaborate.
> Maybe I lost something or just haven't read enough but doesn't it
> sound confusing that we create two structures in UAPI just for the
> Vector registers dump?
AFAICT we need two structures, one for the signal context and another for
the debug stuff, because we represent the vector context differently in
each of these two cases. I proposed the embedded `__riscv_v_csr_state'
structure as a named member, because C doesn't have syntax available for
embedding an already defined structure as an anonymous member and I didn't
want to make use of a macro (which would then become a part of the uAPI)
as means for the same data definition not to be repeated.
Maybe it's not a big deal though. If we inlined the CSR context in both
structures, then the definitions could look like:
struct __riscv_v_signal_state {
unsigned long vstart;
unsigned long vl;
unsigned long vtype;
unsigned long vcsr;
unsigned long vlenb;
void *vregp;
};
struct __riscv_v_regset_state {
unsigned long vstart;
unsigned long vl;
unsigned long vtype;
unsigned long vcsr;
unsigned long vlenb;
char vreg[];
};
OTOH I'm not fully convinced this is actually cleaner. And the CSR state
is distinct in a way here.
NB I'm only concerned about the user API and ABI here, because once we've
set them they'll have been cast in stone. Conversely we can change an
internal representation of the vector context at any time, so if we make a
mistake or change our minds for whatever reason, it is not going to be a
big deal.
Cc-ing LKML in case someone not subscribed to linux-riscv wanted to chime
in. It's always a good idea to cc LKML on patch submissions anyway.
Maciej