public inbox for linux-riscv@lists.infradead.org
 help / color / mirror / Atom feed
* [v1, 0/3] riscv: fix ptrace and export VLENB
@ 2023-08-16 15:54 Andy Chiu
  2023-08-16 15:54 ` [v1, 1/3] RISC-V: Remove ptrace support for vectors Andy Chiu
                   ` (5 more replies)
  0 siblings, 6 replies; 10+ messages in thread
From: Andy Chiu @ 2023-08-16 15:54 UTC (permalink / raw)
  To: linux-riscv, palmer
  Cc: greentime.hu, guoren, bjorn, conor.dooley, Andy Chiu,
	Paul Walmsley, Albert Ou

We add a vlenb field in Vector context and save it with the
riscv_vstate_save() macro. It should not cause performance regression as
VLENB is a design-time constant and is frequently used by hardware.
Also, adding this field into the __sc_riscv_v_state may benifit us on a
future compatibility issue becuse a hardware may have writable VLENB.

Adding and saving VLENB have an immediate benifit as it gives ptrace a
better view of the Vector extension and makes it possible to reconstruct
Vector register files from the dump without doing an additional csr read.

This patchset also sync the number of note types between us and gdb for
riscv to solve a conflicting note.

This is not an ABI break given that 6.5 has not been released yet.

The series is tested on a virt QEMU by verifying VLENB is saved in
ptrace, coredump, and signal stack.

[1] https://sourceware.org/pipermail/gdb-patches/2023-August/201492.html

Andy Chiu (2):
  RISC-V: vector: export VLENB csr in __sc_riscv_v_state
  RISC-V: Add ptrace support for vectors

Palmer Dabbelt (1):
  RISC-V: Remove ptrace support for vectors

 arch/riscv/include/asm/vector.h      | 3 ++-
 arch/riscv/include/uapi/asm/ptrace.h | 1 +
 include/uapi/linux/elf.h             | 3 ++-
 3 files changed, 5 insertions(+), 2 deletions(-)

-- 
2.17.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [v1, 1/3] RISC-V: Remove ptrace support for vectors
  2023-08-16 15:54 [v1, 0/3] riscv: fix ptrace and export VLENB Andy Chiu
@ 2023-08-16 15:54 ` Andy Chiu
  2023-08-16 15:54 ` [v1, 2/3] RISC-V: vector: export VLENB csr in __sc_riscv_v_state Andy Chiu
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Andy Chiu @ 2023-08-16 15:54 UTC (permalink / raw)
  To: linux-riscv, palmer
  Cc: greentime.hu, guoren, bjorn, conor.dooley, Palmer Dabbelt,
	Andy Chiu, Oleg Nesterov, Paul Walmsley, Albert Ou,
	Eric Biederman, Kees Cook, Vincent Chen, Michael Ellerman,
	Benjamin Gray, Qing Zhang, Rolf Eike Beer, Baruch Siach

From: Palmer Dabbelt <palmer@rivosinc.com>

We've found two bugs here: NT_RISCV_VECTOR steps on NT_RISCV_CSR (which
is only for embedded), and we don't have vlenb in the core dumps.  Given
that we've have a pair of bugs croup up as part of the GDB review we've
probably got other issues, so let's just cut this for 6.5 and get it
right.

Fixes: 0c59922c769a ("riscv: Add ptrace vector support")
Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
Reviewed-by: Maciej W. Rozycki <macro@orcam.me.uk>
Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
---
 arch/riscv/kernel/ptrace.c | 69 --------------------------------------
 include/uapi/linux/elf.h   |  1 -
 2 files changed, 70 deletions(-)

diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
index 1d572cf3140f..487303e3ef22 100644
--- a/arch/riscv/kernel/ptrace.c
+++ b/arch/riscv/kernel/ptrace.c
@@ -25,9 +25,6 @@ enum riscv_regset {
 #ifdef CONFIG_FPU
 	REGSET_F,
 #endif
-#ifdef CONFIG_RISCV_ISA_V
-	REGSET_V,
-#endif
 };
 
 static int riscv_gpr_get(struct task_struct *target,
@@ -84,61 +81,6 @@ static int riscv_fpr_set(struct task_struct *target,
 }
 #endif
 
-#ifdef CONFIG_RISCV_ISA_V
-static int riscv_vr_get(struct task_struct *target,
-			const struct user_regset *regset,
-			struct membuf to)
-{
-	struct __riscv_v_ext_state *vstate = &target->thread.vstate;
-
-	if (!riscv_v_vstate_query(task_pt_regs(target)))
-		return -EINVAL;
-
-	/*
-	 * Ensure the vector registers have been saved to the memory before
-	 * copying them to membuf.
-	 */
-	if (target == current)
-		riscv_v_vstate_save(current, task_pt_regs(current));
-
-	/* Copy vector header from vstate. */
-	membuf_write(&to, vstate, offsetof(struct __riscv_v_ext_state, datap));
-	membuf_zero(&to, sizeof(vstate->datap));
-
-	/* Copy all the vector registers from vstate. */
-	return membuf_write(&to, vstate->datap, riscv_v_vsize);
-}
-
-static int riscv_vr_set(struct task_struct *target,
-			const struct user_regset *regset,
-			unsigned int pos, unsigned int count,
-			const void *kbuf, const void __user *ubuf)
-{
-	int ret, size;
-	struct __riscv_v_ext_state *vstate = &target->thread.vstate;
-
-	if (!riscv_v_vstate_query(task_pt_regs(target)))
-		return -EINVAL;
-
-	/* Copy rest of the vstate except datap */
-	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, vstate, 0,
-				 offsetof(struct __riscv_v_ext_state, datap));
-	if (unlikely(ret))
-		return ret;
-
-	/* Skip copy datap. */
-	size = sizeof(vstate->datap);
-	count -= size;
-	ubuf += size;
-
-	/* Copy all the vector registers. */
-	pos = 0;
-	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, vstate->datap,
-				 0, riscv_v_vsize);
-	return ret;
-}
-#endif
-
 static const struct user_regset riscv_user_regset[] = {
 	[REGSET_X] = {
 		.core_note_type = NT_PRSTATUS,
@@ -158,17 +100,6 @@ static const struct user_regset riscv_user_regset[] = {
 		.set = riscv_fpr_set,
 	},
 #endif
-#ifdef CONFIG_RISCV_ISA_V
-	[REGSET_V] = {
-		.core_note_type = NT_RISCV_VECTOR,
-		.align = 16,
-		.n = ((32 * RISCV_MAX_VLENB) +
-		      sizeof(struct __riscv_v_ext_state)) / sizeof(__u32),
-		.size = sizeof(__u32),
-		.regset_get = riscv_vr_get,
-		.set = riscv_vr_set,
-	},
-#endif
 };
 
 static const struct user_regset_view riscv_user_native_view = {
diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
index 0c8cf359ea5b..e0e159138331 100644
--- a/include/uapi/linux/elf.h
+++ b/include/uapi/linux/elf.h
@@ -443,7 +443,6 @@ typedef struct elf64_shdr {
 #define NT_MIPS_DSP	0x800		/* MIPS DSP ASE registers */
 #define NT_MIPS_FP_MODE	0x801		/* MIPS floating-point mode */
 #define NT_MIPS_MSA	0x802		/* MIPS SIMD registers */
-#define NT_RISCV_VECTOR	0x900		/* RISC-V vector registers */
 #define NT_LOONGARCH_CPUCFG	0xa00	/* LoongArch CPU config registers */
 #define NT_LOONGARCH_CSR	0xa01	/* LoongArch control and status registers */
 #define NT_LOONGARCH_LSX	0xa02	/* LoongArch Loongson SIMD Extension registers */
-- 
2.17.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [v1, 2/3] RISC-V: vector: export VLENB csr in __sc_riscv_v_state
  2023-08-16 15:54 [v1, 0/3] riscv: fix ptrace and export VLENB Andy Chiu
  2023-08-16 15:54 ` [v1, 1/3] RISC-V: Remove ptrace support for vectors Andy Chiu
@ 2023-08-16 15:54 ` Andy Chiu
  2023-08-17 12:35   ` Maciej W. Rozycki
  2023-08-16 15:54 ` [v1, 3/3] RISC-V: Add ptrace support for vectors Andy Chiu
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 10+ messages in thread
From: Andy Chiu @ 2023-08-16 15:54 UTC (permalink / raw)
  To: linux-riscv, palmer
  Cc: greentime.hu, guoren, bjorn, conor.dooley, Andy Chiu,
	Paul Walmsley, Albert Ou, Heiko Stuebner, Guo Ren, Vincent Chen,
	Björn Töpel

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

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [v1, 3/3] RISC-V: Add ptrace support for vectors
  2023-08-16 15:54 [v1, 0/3] riscv: fix ptrace and export VLENB Andy Chiu
  2023-08-16 15:54 ` [v1, 1/3] RISC-V: Remove ptrace support for vectors Andy Chiu
  2023-08-16 15:54 ` [v1, 2/3] RISC-V: vector: export VLENB csr in __sc_riscv_v_state Andy Chiu
@ 2023-08-16 15:54 ` Andy Chiu
  2023-08-17 12:35 ` [v1, 0/3] riscv: fix ptrace and export VLENB Maciej W. Rozycki
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Andy Chiu @ 2023-08-16 15:54 UTC (permalink / raw)
  To: linux-riscv, palmer
  Cc: greentime.hu, guoren, bjorn, conor.dooley, Andy Chiu,
	Oleg Nesterov, Paul Walmsley, Albert Ou, Eric Biederman,
	Kees Cook, Michael Ellerman, Russell Currey, Benjamin Gray,
	Baruch Siach, Rolf Eike Beer, Qing Zhang, Vincent Chen

This patch add back the ptrace support with the following fix:
 - Define NT_RISCV_CSR and re-number NT_RISCV_VECTOR to prevent
   conflicting with gdb's NT_RISCV_CSR.

Since gdb does not directly include the note description header in
Linux and has already defined NT_RISCV_CSR as 0x900, we decide to
sync with gdb and renumber NT_RISCV_VECTOR to solve and prevent future
conflicts.

Fixes: 0c59922c769a ("riscv: Add ptrace vector support")
Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
---
Hey Palmer, it is possible to merge this into the [1/3] patch so it
looks prettier. Or, please tell me which one would you prefer if a
respin is needed, thanks!

 arch/riscv/kernel/ptrace.c | 69 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/elf.h   |  2 ++
 2 files changed, 71 insertions(+)

diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
index 487303e3ef22..1d572cf3140f 100644
--- a/arch/riscv/kernel/ptrace.c
+++ b/arch/riscv/kernel/ptrace.c
@@ -25,6 +25,9 @@ enum riscv_regset {
 #ifdef CONFIG_FPU
 	REGSET_F,
 #endif
+#ifdef CONFIG_RISCV_ISA_V
+	REGSET_V,
+#endif
 };
 
 static int riscv_gpr_get(struct task_struct *target,
@@ -81,6 +84,61 @@ static int riscv_fpr_set(struct task_struct *target,
 }
 #endif
 
+#ifdef CONFIG_RISCV_ISA_V
+static int riscv_vr_get(struct task_struct *target,
+			const struct user_regset *regset,
+			struct membuf to)
+{
+	struct __riscv_v_ext_state *vstate = &target->thread.vstate;
+
+	if (!riscv_v_vstate_query(task_pt_regs(target)))
+		return -EINVAL;
+
+	/*
+	 * Ensure the vector registers have been saved to the memory before
+	 * copying them to membuf.
+	 */
+	if (target == current)
+		riscv_v_vstate_save(current, task_pt_regs(current));
+
+	/* Copy vector header from vstate. */
+	membuf_write(&to, vstate, offsetof(struct __riscv_v_ext_state, datap));
+	membuf_zero(&to, sizeof(vstate->datap));
+
+	/* Copy all the vector registers from vstate. */
+	return membuf_write(&to, vstate->datap, riscv_v_vsize);
+}
+
+static int riscv_vr_set(struct task_struct *target,
+			const struct user_regset *regset,
+			unsigned int pos, unsigned int count,
+			const void *kbuf, const void __user *ubuf)
+{
+	int ret, size;
+	struct __riscv_v_ext_state *vstate = &target->thread.vstate;
+
+	if (!riscv_v_vstate_query(task_pt_regs(target)))
+		return -EINVAL;
+
+	/* Copy rest of the vstate except datap */
+	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, vstate, 0,
+				 offsetof(struct __riscv_v_ext_state, datap));
+	if (unlikely(ret))
+		return ret;
+
+	/* Skip copy datap. */
+	size = sizeof(vstate->datap);
+	count -= size;
+	ubuf += size;
+
+	/* Copy all the vector registers. */
+	pos = 0;
+	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, vstate->datap,
+				 0, riscv_v_vsize);
+	return ret;
+}
+#endif
+
 static const struct user_regset riscv_user_regset[] = {
 	[REGSET_X] = {
 		.core_note_type = NT_PRSTATUS,
@@ -100,6 +158,17 @@ static const struct user_regset riscv_user_regset[] = {
 		.set = riscv_fpr_set,
 	},
 #endif
+#ifdef CONFIG_RISCV_ISA_V
+	[REGSET_V] = {
+		.core_note_type = NT_RISCV_VECTOR,
+		.align = 16,
+		.n = ((32 * RISCV_MAX_VLENB) +
+		      sizeof(struct __riscv_v_ext_state)) / sizeof(__u32),
+		.size = sizeof(__u32),
+		.regset_get = riscv_vr_get,
+		.set = riscv_vr_set,
+	},
+#endif
 };
 
 static const struct user_regset_view riscv_user_native_view = {
diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
index e0e159138331..20e285fdbc46 100644
--- a/include/uapi/linux/elf.h
+++ b/include/uapi/linux/elf.h
@@ -443,6 +443,8 @@ typedef struct elf64_shdr {
 #define NT_MIPS_DSP	0x800		/* MIPS DSP ASE registers */
 #define NT_MIPS_FP_MODE	0x801		/* MIPS floating-point mode */
 #define NT_MIPS_MSA	0x802		/* MIPS SIMD registers */
+#define NT_RISCV_CSR	0x900		/* RISC-V Control and Status Registers */
+#define NT_RISCV_VECTOR	0x901		/* RISC-V vector registers */
 #define NT_LOONGARCH_CPUCFG	0xa00	/* LoongArch CPU config registers */
 #define NT_LOONGARCH_CSR	0xa01	/* LoongArch control and status registers */
 #define NT_LOONGARCH_LSX	0xa02	/* LoongArch Loongson SIMD Extension registers */
-- 
2.17.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [v1, 0/3] riscv: fix ptrace and export VLENB
  2023-08-16 15:54 [v1, 0/3] riscv: fix ptrace and export VLENB Andy Chiu
                   ` (2 preceding siblings ...)
  2023-08-16 15:54 ` [v1, 3/3] RISC-V: Add ptrace support for vectors Andy Chiu
@ 2023-08-17 12:35 ` Maciej W. Rozycki
  2023-08-23 19:40 ` patchwork-bot+linux-riscv
  2023-08-24 20:08 ` (subset) " Palmer Dabbelt
  5 siblings, 0 replies; 10+ messages in thread
From: Maciej W. Rozycki @ 2023-08-17 12:35 UTC (permalink / raw)
  To: Andy Chiu
  Cc: linux-riscv, Palmer Dabbelt, greentime.hu, guoren, bjorn,
	conor.dooley, Paul Walmsley, Albert Ou

On Wed, 16 Aug 2023, Andy Chiu wrote:

> We add a vlenb field in Vector context and save it with the
> riscv_vstate_save() macro. It should not cause performance regression as
> VLENB is a design-time constant and is frequently used by hardware.
> Also, adding this field into the __sc_riscv_v_state may benifit us on a
> future compatibility issue becuse a hardware may have writable VLENB.
> 
> Adding and saving VLENB have an immediate benifit as it gives ptrace a
> better view of the Vector extension and makes it possible to reconstruct
> Vector register files from the dump without doing an additional csr read.

 I think it is an incorrect view of the situation.

 We need to include VLENB not to let a debugger avoid reading VLENB itself 
in its own context, but to have a complete view of the debuggee's register 
file.  While the ptrace(2) API has not been formally standardised by a 
body such as IEEE or The Open Group, our practice across ports has been to 
provide a complete view of the debuggee via ptrace(2) without a need to 
resort to examine the machine and OS on the debugger's side via other 
means.

 So we're not improving ptrace(2), but rather we're removing a defect.

  Maciej

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [v1, 2/3] RISC-V: vector: export VLENB csr in __sc_riscv_v_state
  2023-08-16 15:54 ` [v1, 2/3] RISC-V: vector: export VLENB csr in __sc_riscv_v_state Andy Chiu
@ 2023-08-17 12:35   ` Maciej W. Rozycki
  2023-08-22 18:01     ` Andy Chiu
  0 siblings, 1 reply; 10+ messages in thread
From: Maciej W. Rozycki @ 2023-08-17 12:35 UTC (permalink / raw)
  To: Andy Chiu
  Cc: linux-riscv, Palmer Dabbelt, greentime.hu, guoren, bjorn,
	conor.dooley, Paul Walmsley, Albert Ou, Heiko Stuebner, Guo Ren,
	Vincent Chen, Björn Töpel

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [v1, 2/3] RISC-V: vector: export VLENB csr in __sc_riscv_v_state
  2023-08-17 12:35   ` Maciej W. Rozycki
@ 2023-08-22 18:01     ` Andy Chiu
  2023-08-22 22:39       ` Maciej W. Rozycki
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Chiu @ 2023-08-22 18:01 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: linux-riscv, Palmer Dabbelt, greentime.hu, guoren, bjorn,
	conor.dooley, Paul Walmsley, Albert Ou, Heiko Stuebner, Guo Ren,
	Vincent Chen, Björn Töpel

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [v1, 2/3] RISC-V: vector: export VLENB csr in __sc_riscv_v_state
  2023-08-22 18:01     ` Andy Chiu
@ 2023-08-22 22:39       ` Maciej W. Rozycki
  0 siblings, 0 replies; 10+ messages in thread
From: Maciej W. Rozycki @ 2023-08-22 22:39 UTC (permalink / raw)
  To: Andy Chiu
  Cc: linux-riscv, Palmer Dabbelt, greentime.hu, guoren, bjorn,
	conor.dooley, Paul Walmsley, Albert Ou, Heiko Stuebner, Guo Ren,
	Vincent Chen, Björn Töpel, linux-kernel

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [v1, 0/3] riscv: fix ptrace and export VLENB
  2023-08-16 15:54 [v1, 0/3] riscv: fix ptrace and export VLENB Andy Chiu
                   ` (3 preceding siblings ...)
  2023-08-17 12:35 ` [v1, 0/3] riscv: fix ptrace and export VLENB Maciej W. Rozycki
@ 2023-08-23 19:40 ` patchwork-bot+linux-riscv
  2023-08-24 20:08 ` (subset) " Palmer Dabbelt
  5 siblings, 0 replies; 10+ messages in thread
From: patchwork-bot+linux-riscv @ 2023-08-23 19:40 UTC (permalink / raw)
  To: Andy Chiu
  Cc: linux-riscv, palmer, greentime.hu, guoren, bjorn, conor.dooley,
	paul.walmsley, aou

Hello:

This series was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Wed, 16 Aug 2023 15:54:47 +0000 you wrote:
> We add a vlenb field in Vector context and save it with the
> riscv_vstate_save() macro. It should not cause performance regression as
> VLENB is a design-time constant and is frequently used by hardware.
> Also, adding this field into the __sc_riscv_v_state may benifit us on a
> future compatibility issue becuse a hardware may have writable VLENB.
> 
> Adding and saving VLENB have an immediate benifit as it gives ptrace a
> better view of the Vector extension and makes it possible to reconstruct
> Vector register files from the dump without doing an additional csr read.
> 
> [...]

Here is the summary with links:
  - [v1,1/3] RISC-V: Remove ptrace support for vectors
    https://git.kernel.org/riscv/c/e3f9324b231a
  - [v1,2/3] RISC-V: vector: export VLENB csr in __sc_riscv_v_state
    https://git.kernel.org/riscv/c/c35f3aa34509
  - [v1,3/3] RISC-V: Add ptrace support for vectors
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: (subset) [v1, 0/3] riscv: fix ptrace and export VLENB
  2023-08-16 15:54 [v1, 0/3] riscv: fix ptrace and export VLENB Andy Chiu
                   ` (4 preceding siblings ...)
  2023-08-23 19:40 ` patchwork-bot+linux-riscv
@ 2023-08-24 20:08 ` Palmer Dabbelt
  5 siblings, 0 replies; 10+ messages in thread
From: Palmer Dabbelt @ 2023-08-24 20:08 UTC (permalink / raw)
  To: linux-riscv, Palmer Dabbelt, Andy Chiu
  Cc: greentime.hu, bjorn, Conor Dooley, Paul Walmsley, Albert Ou,
	Guo Ren


On Wed, 16 Aug 2023 15:54:47 +0000, Andy Chiu wrote:
> We add a vlenb field in Vector context and save it with the
> riscv_vstate_save() macro. It should not cause performance regression as
> VLENB is a design-time constant and is frequently used by hardware.
> Also, adding this field into the __sc_riscv_v_state may benifit us on a
> future compatibility issue becuse a hardware may have writable VLENB.
> 
> Adding and saving VLENB have an immediate benifit as it gives ptrace a
> better view of the Vector extension and makes it possible to reconstruct
> Vector register files from the dump without doing an additional csr read.
> 
> [...]

Applied, thanks!

[1/3] RISC-V: Remove ptrace support for vectors
      https://git.kernel.org/palmer/c/e3f9324b231a
[2/3] RISC-V: vector: export VLENB csr in __sc_riscv_v_state
      https://git.kernel.org/palmer/c/c35f3aa34509

Best regards,
-- 
Palmer Dabbelt <palmer@rivosinc.com>


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-08-24 20:09 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-16 15:54 [v1, 0/3] riscv: fix ptrace and export VLENB Andy Chiu
2023-08-16 15:54 ` [v1, 1/3] RISC-V: Remove ptrace support for vectors Andy Chiu
2023-08-16 15:54 ` [v1, 2/3] RISC-V: vector: export VLENB csr in __sc_riscv_v_state Andy Chiu
2023-08-17 12:35   ` Maciej W. Rozycki
2023-08-22 18:01     ` Andy Chiu
2023-08-22 22:39       ` Maciej W. Rozycki
2023-08-16 15:54 ` [v1, 3/3] RISC-V: Add ptrace support for vectors Andy Chiu
2023-08-17 12:35 ` [v1, 0/3] riscv: fix ptrace and export VLENB Maciej W. Rozycki
2023-08-23 19:40 ` patchwork-bot+linux-riscv
2023-08-24 20:08 ` (subset) " Palmer Dabbelt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox