| Message ID | 20230816155450.26200-3-andy.chiu@sifive.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c35f3aa34509085bfc9800c86bc9998f8954933d |
| Headers | show |
| Series | riscv: fix ptrace and export VLENB | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Series has a cover letter |
| conchuod/tree_selection | success | Guessed tree name to be fixes at HEAD ca09f772ccca |
| conchuod/fixes_present | success | Fixes tag present in non-next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 4 and now 4 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 2810 this patch: 2810 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 15873 this patch: 15872 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 12 this patch: 12 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 17 lines checked |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | Fixes tag looks correct |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
On Wed, 16 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). Since this is a part of the UAPI I suggest consulting with libc people, possibly by posting an RFC to <libc-alpha@sourceware.org>. Maciej
Hi, On Thu, Aug 17, 2023 at 8:35 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote: > > On Wed, 16 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. 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. 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? > > Since this is a part of the UAPI I suggest consulting with libc people, > possibly by posting an RFC to <libc-alpha@sourceware.org>. > > Maciej Thanks, Andy
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
diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h index 3d78930cab51..c5ee07b3df07 100644 --- a/arch/riscv/include/asm/vector.h +++ b/arch/riscv/include/asm/vector.h @@ -70,8 +70,9 @@ static __always_inline void __vstate_csr_save(struct __riscv_v_ext_state *dest) "csrr %1, " __stringify(CSR_VTYPE) "\n\t" "csrr %2, " __stringify(CSR_VL) "\n\t" "csrr %3, " __stringify(CSR_VCSR) "\n\t" + "csrr %4, " __stringify(CSR_VLENB) "\n\t" : "=r" (dest->vstart), "=r" (dest->vtype), "=r" (dest->vl), - "=r" (dest->vcsr) : :); + "=r" (dest->vcsr), "=r" (dest->vlenb) : :); } static __always_inline void __vstate_csr_restore(struct __riscv_v_ext_state *src) 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; /* * In signal handler, datap will be set a correct user stack offset
VLENB is critical for callers of ptrace to reconstruct Vector register files from the register dump of NT_RISCV_VECTOR. Also, future systems may will have a writable VLENB, so add it now to potentially save future compatibility issue. Fixes: 0c59922c769a ("riscv: Add ptrace vector support") Signed-off-by: Andy Chiu <andy.chiu@sifive.com> --- arch/riscv/include/asm/vector.h | 3 ++- arch/riscv/include/uapi/asm/ptrace.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)