[PATCH 0/4] OpenRISC floating point context support

[PATCH 0/4] OpenRISC floating point context support

[Stafford Horne]

This series adds support for storing and restoring the OpenRISC floating point
context as well as user space API's and regsets.

To support these patches an architecture change had to be made.  The OpenRISC
fpu status and control register FPCSR is now read/writeable by user space as of
architecture spec revision 1.4:

 - https://openrisc.io/revisions/r1.4

Previous to this FPCSR reads and writes from user-mode would just be ignored,
reads and writes to non permissioned special purpose registers (SPR's) are
no-ops.

The patch is split into 3 main parts, the first patch being a cleanup to
handling exceptions noticed while working on this.

 1. Add support to context saving and switching
 2. Add support for SIGFPE and sigcontext
 3. Add regset for ptrace register reading/writing

The series has been tested with the glibc test suite and all math tests are
passing.  Patched projects include:

 - GCC - patches upstream to fix issue with resetting exceptions in libgcc
 - glibc - https://github.com/stffrdhrn/or1k-glibc/commits/or1k-hard-float
 - qemu - https://github.com/stffrdhrn/qemu/commits/or1k-user-fpcsr

I have not worked on binutils-gdb patches yet.

Stafford Horne (4):
  openrisc: Properly store r31 to pt_regs on unhandled exceptions
  openrisc: Support storing and restoring fpu state
  openrisc: Support floating point user api
  openrisc: Add floating point regset

 arch/openrisc/include/asm/ptrace.h          |  4 +--
 arch/openrisc/include/uapi/asm/elf.h        |  3 +-
 arch/openrisc/include/uapi/asm/ptrace.h     |  4 +++
 arch/openrisc/include/uapi/asm/sigcontext.h |  1 +
 arch/openrisc/kernel/entry.S                | 31 +++++++++++++----
 arch/openrisc/kernel/head.S                 |  4 +--
 arch/openrisc/kernel/ptrace.c               | 37 +++++++++++++++++++++
 arch/openrisc/kernel/signal.c               |  2 ++
 arch/openrisc/kernel/traps.c                | 27 +++++++++++++--
 9 files changed, 99 insertions(+), 14 deletions(-)

-- 
2.39.1

[PATCH 1/4] openrisc: Properly store r31 to pt_regs on unhandled exceptions

[Stafford Horne]

In commit 91993c8c2ed5 ("openrisc: use shadow registers to save regs on
exception") the unhandled exception path was changed to do an early
store of r30 instead of r31.  The entry code was not updated and r31 is
not getting stored to pt_regs.

This patch updates the entry handler to store r31 instead of r30.  We
also remove some misleading commented out store r30 and r31
instructrions.

I noticed this while working on adding floating point exception
handling,  This issue probably would never impact anything since we kill
the process or Oops right away on unhandled exceptions.

Fixes: 91993c8c2ed5 ("openrisc: use shadow registers to save regs on exception")
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 arch/openrisc/kernel/entry.S | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/openrisc/kernel/entry.S b/arch/openrisc/kernel/entry.S
index 54a87bba35ca..a130c4dac48d 100644
--- a/arch/openrisc/kernel/entry.S
+++ b/arch/openrisc/kernel/entry.S
@@ -173,7 +173,6 @@ handler:							;\
 	l.sw    PT_GPR28(r1),r28					;\
 	l.sw    PT_GPR29(r1),r29					;\
 	/* r30 already save */					;\
-/*        l.sw    PT_GPR30(r1),r30*/					;\
 	l.sw    PT_GPR31(r1),r31					;\
 	TRACE_IRQS_OFF_ENTRY						;\
 	/* Store -1 in orig_gpr11 for non-syscall exceptions */	;\
@@ -211,9 +210,8 @@ handler:							;\
 	l.sw    PT_GPR27(r1),r27					;\
 	l.sw    PT_GPR28(r1),r28					;\
 	l.sw    PT_GPR29(r1),r29					;\
-	/* r31 already saved */					;\
-	l.sw    PT_GPR30(r1),r30					;\
-/*        l.sw    PT_GPR31(r1),r31	*/				;\
+	/* r30 already saved */						;\
+	l.sw    PT_GPR31(r1),r31					;\
 	/* Store -1 in orig_gpr11 for non-syscall exceptions */	;\
 	l.addi	r30,r0,-1					;\
 	l.sw	PT_ORIG_GPR11(r1),r30				;\
-- 
2.39.1

[PATCH 2/4] openrisc: Support storing and restoring fpu state

[Stafford Horne]

OpenRISC floating point state is not so expensive to save as OpenRISC uses
general purpose registers for floating point instructions.  We need to save
only the floating point status and control register, FPCSR.

Add support to maintain the FPCSR unconditionally upon exceptions and
switches.  On machines that do not support FPU this will always just
store 0x0 and restore is a no-op.  On FPU systems this adds an
additional special purpose register read/write and read/write to memory
(already cached).

Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 arch/openrisc/include/asm/ptrace.h |  4 ++--
 arch/openrisc/kernel/entry.S       | 14 ++++++++++++++
 arch/openrisc/kernel/traps.c       |  5 +++--
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/arch/openrisc/include/asm/ptrace.h b/arch/openrisc/include/asm/ptrace.h
index 01f81d4e97dc..375147ff71fc 100644
--- a/arch/openrisc/include/asm/ptrace.h
+++ b/arch/openrisc/include/asm/ptrace.h
@@ -59,7 +59,7 @@ struct pt_regs {
 	 * -1 for all other exceptions.
 	 */
 	long  orig_gpr11;	/* For restarting system calls */
-	long dummy;		/* Cheap alignment fix */
+	long fpcsr;		/* Floating point control status register. */
 	long dummy2;		/* Cheap alignment fix */
 };
 
@@ -115,6 +115,6 @@ static inline long regs_return_value(struct pt_regs *regs)
 #define PT_GPR31      124
 #define PT_PC	      128
 #define PT_ORIG_GPR11 132
-#define PT_SYSCALLNO  136
+#define PT_FPCSR      136
 
 #endif /* __ASM_OPENRISC_PTRACE_H */
diff --git a/arch/openrisc/kernel/entry.S b/arch/openrisc/kernel/entry.S
index a130c4dac48d..c7b47e571220 100644
--- a/arch/openrisc/kernel/entry.S
+++ b/arch/openrisc/kernel/entry.S
@@ -106,6 +106,8 @@
 	l.mtspr r0,r3,SPR_EPCR_BASE				;\
 	l.lwz   r3,PT_SR(r1)					;\
 	l.mtspr r0,r3,SPR_ESR_BASE				;\
+	l.lwz	r3,PT_FPCSR(r1)					;\
+	l.mtspr	r0,r3,SPR_FPCSR					;\
 	l.lwz   r2,PT_GPR2(r1)					;\
 	l.lwz   r3,PT_GPR3(r1)					;\
 	l.lwz   r4,PT_GPR4(r1)					;\
@@ -175,6 +177,8 @@ handler:							;\
 	/* r30 already save */					;\
 	l.sw    PT_GPR31(r1),r31					;\
 	TRACE_IRQS_OFF_ENTRY						;\
+	l.mfspr	r30,r0,SPR_FPCSR				;\
+	l.sw	PT_FPCSR(r1),r30				;\
 	/* Store -1 in orig_gpr11 for non-syscall exceptions */	;\
 	l.addi	r30,r0,-1					;\
 	l.sw	PT_ORIG_GPR11(r1),r30
@@ -215,6 +219,8 @@ handler:							;\
 	/* Store -1 in orig_gpr11 for non-syscall exceptions */	;\
 	l.addi	r30,r0,-1					;\
 	l.sw	PT_ORIG_GPR11(r1),r30				;\
+	l.mfspr	r30,r0,SPR_FPCSR				;\
+	l.sw	PT_FPCSR(r1),r30				;\
 	l.addi	r3,r1,0						;\
 	/* r4 is exception EA */				;\
 	l.addi	r5,r0,vector					;\
@@ -1087,6 +1093,10 @@ ENTRY(_switch)
 	l.sw    PT_GPR28(r1),r28
 	l.sw    PT_GPR30(r1),r30
 
+	/* Store the old FPU state to new pt_regs */
+	l.mfspr	r29,r0,SPR_FPCSR
+	l.sw	PT_FPCSR(r1),r29
+
 	l.addi	r11,r10,0			/* Save old 'current' to 'last' return value*/
 
 	/* We use thread_info->ksp for storing the address of the above
@@ -1109,6 +1119,10 @@ ENTRY(_switch)
 	l.lwz	r29,PT_SP(r1)
 	l.sw	TI_KSP(r10),r29
 
+	/* Restore the old value of FPCSR */
+	l.lwz	r29,PT_FPCSR(r1)
+	l.mtspr	r0,r29,SPR_FPCSR
+
 	/* ...and restore the registers, except r11 because the return value
 	 * has already been set above.
 	 */
diff --git a/arch/openrisc/kernel/traps.c b/arch/openrisc/kernel/traps.c
index fd9a0f2b66c4..f5bbe6b55849 100644
--- a/arch/openrisc/kernel/traps.c
+++ b/arch/openrisc/kernel/traps.c
@@ -75,8 +75,9 @@ void show_registers(struct pt_regs *regs)
 		in_kernel = 0;
 
 	printk("CPU #: %d\n"
-	       "   PC: %08lx    SR: %08lx    SP: %08lx\n",
-	       smp_processor_id(), regs->pc, regs->sr, regs->sp);
+	       "   PC: %08lx    SR: %08lx    SP: %08lx FPCSR: %08lx\n",
+	       smp_processor_id(), regs->pc, regs->sr, regs->sp,
+	       regs->fpcsr);
 	printk("GPR00: %08lx GPR01: %08lx GPR02: %08lx GPR03: %08lx\n",
 	       0L, regs->gpr[1], regs->gpr[2], regs->gpr[3]);
 	printk("GPR04: %08lx GPR05: %08lx GPR06: %08lx GPR07: %08lx\n",
-- 
2.39.1

[PATCH 3/4] openrisc: Support floating point user api

[Stafford Horne]

Add support for handling floating point exceptions and forwarding the
SIGFPE signal to processes.  Also, add fpu state to sigcontext.

Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 arch/openrisc/include/uapi/asm/elf.h        |  3 +--
 arch/openrisc/include/uapi/asm/ptrace.h     |  4 ++++
 arch/openrisc/include/uapi/asm/sigcontext.h |  1 +
 arch/openrisc/kernel/entry.S                | 11 +++++++++--
 arch/openrisc/kernel/head.S                 |  4 ++--
 arch/openrisc/kernel/signal.c               |  2 ++
 arch/openrisc/kernel/traps.c                | 22 +++++++++++++++++++++
 7 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/arch/openrisc/include/uapi/asm/elf.h b/arch/openrisc/include/uapi/asm/elf.h
index e892d5061685..6868f81c281e 100644
--- a/arch/openrisc/include/uapi/asm/elf.h
+++ b/arch/openrisc/include/uapi/asm/elf.h
@@ -53,8 +53,7 @@ typedef unsigned long elf_greg_t;
 #define ELF_NGREG (sizeof(struct user_regs_struct) / sizeof(elf_greg_t))
 typedef elf_greg_t elf_gregset_t[ELF_NGREG];
 
-/* A placeholder; OR32 does not have fp support yes, so no fp regs for now.  */
-typedef unsigned long elf_fpregset_t;
+typedef struct __or1k_fpu_state elf_fpregset_t;
 
 /* EM_OPENRISC is defined in linux/elf-em.h */
 #define EM_OR32         0x8472
diff --git a/arch/openrisc/include/uapi/asm/ptrace.h b/arch/openrisc/include/uapi/asm/ptrace.h
index d4fab268f6aa..a77cc9915ca8 100644
--- a/arch/openrisc/include/uapi/asm/ptrace.h
+++ b/arch/openrisc/include/uapi/asm/ptrace.h
@@ -30,6 +30,10 @@ struct user_regs_struct {
 	unsigned long pc;
 	unsigned long sr;
 };
+
+struct __or1k_fpu_state {
+	unsigned long fpcsr;
+};
 #endif
 
 
diff --git a/arch/openrisc/include/uapi/asm/sigcontext.h b/arch/openrisc/include/uapi/asm/sigcontext.h
index 8ab775fc3450..ca585e4af6b8 100644
--- a/arch/openrisc/include/uapi/asm/sigcontext.h
+++ b/arch/openrisc/include/uapi/asm/sigcontext.h
@@ -28,6 +28,7 @@
 
 struct sigcontext {
 	struct user_regs_struct regs;  /* needs to be first */
+	struct __or1k_fpu_state fpu;
 	unsigned long oldmask;
 };
 
diff --git a/arch/openrisc/kernel/entry.S b/arch/openrisc/kernel/entry.S
index c7b47e571220..c9f48e750b72 100644
--- a/arch/openrisc/kernel/entry.S
+++ b/arch/openrisc/kernel/entry.S
@@ -848,9 +848,16 @@ _syscall_badsys:
 
 /******* END SYSCALL HANDLING *******/
 
-/* ---[ 0xd00: Trap exception ]------------------------------------------ */
+/* ---[ 0xd00: Floating Point exception ]-------------------------------- */
 
-UNHANDLED_EXCEPTION(_vector_0xd00,0xd00)
+EXCEPTION_ENTRY(_fpe_trap_handler)
+	CLEAR_LWA_FLAG(r3)
+	/* r4: EA of fault (set by EXCEPTION_HANDLE) */
+	l.jal   do_fpe_trap
+	 l.addi  r3,r1,0 /* pt_regs */
+
+	l.j     _ret_from_exception
+	 l.nop
 
 /* ---[ 0xe00: Trap exception ]------------------------------------------ */
 
diff --git a/arch/openrisc/kernel/head.S b/arch/openrisc/kernel/head.S
index e11699f3d6bd..439e00f81e5d 100644
--- a/arch/openrisc/kernel/head.S
+++ b/arch/openrisc/kernel/head.S
@@ -424,9 +424,9 @@ _dispatch_do_ipage_fault:
     .org 0xc00
 	EXCEPTION_HANDLE(_sys_call_handler)
 
-/* ---[ 0xd00: Trap exception ]------------------------------------------ */
+/* ---[ 0xd00: Floating point exception ]--------------------------------- */
     .org 0xd00
-	UNHANDLED_EXCEPTION(_vector_0xd00)
+	EXCEPTION_HANDLE(_fpe_trap_handler)
 
 /* ---[ 0xe00: Trap exception ]------------------------------------------ */
     .org 0xe00
diff --git a/arch/openrisc/kernel/signal.c b/arch/openrisc/kernel/signal.c
index 80f69740c731..4664a18f0787 100644
--- a/arch/openrisc/kernel/signal.c
+++ b/arch/openrisc/kernel/signal.c
@@ -50,6 +50,7 @@ static int restore_sigcontext(struct pt_regs *regs,
 	err |= __copy_from_user(regs, sc->regs.gpr, 32 * sizeof(unsigned long));
 	err |= __copy_from_user(&regs->pc, &sc->regs.pc, sizeof(unsigned long));
 	err |= __copy_from_user(&regs->sr, &sc->regs.sr, sizeof(unsigned long));
+	err |= __copy_from_user(&regs->fpcsr, &sc->fpu.fpcsr, sizeof(unsigned long));
 
 	/* make sure the SM-bit is cleared so user-mode cannot fool us */
 	regs->sr &= ~SPR_SR_SM;
@@ -112,6 +113,7 @@ static int setup_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc)
 	err |= __copy_to_user(sc->regs.gpr, regs, 32 * sizeof(unsigned long));
 	err |= __copy_to_user(&sc->regs.pc, &regs->pc, sizeof(unsigned long));
 	err |= __copy_to_user(&sc->regs.sr, &regs->sr, sizeof(unsigned long));
+	err |= __copy_to_user(&sc->fpu.fpcsr, &regs->fpcsr, sizeof(unsigned long));
 
 	return err;
 }
diff --git a/arch/openrisc/kernel/traps.c b/arch/openrisc/kernel/traps.c
index f5bbe6b55849..0aa6b07efda1 100644
--- a/arch/openrisc/kernel/traps.c
+++ b/arch/openrisc/kernel/traps.c
@@ -243,6 +243,28 @@ asmlinkage void unhandled_exception(struct pt_regs *regs, int ea, int vector)
 	die("Oops", regs, 9);
 }
 
+asmlinkage void do_fpe_trap(struct pt_regs *regs, unsigned long address)
+{
+	int code = FPE_FLTUNK;
+	unsigned long fpcsr = regs->fpcsr;
+
+	if (fpcsr & SPR_FPCSR_IVF)
+		code = FPE_FLTINV;
+	else if (fpcsr & SPR_FPCSR_OVF)
+		code = FPE_FLTOVF;
+	else if (fpcsr & SPR_FPCSR_UNF)
+		code = FPE_FLTUND;
+	else if (fpcsr & SPR_FPCSR_DZF)
+		code = FPE_FLTDIV;
+	else if (fpcsr & SPR_FPCSR_IXF)
+		code = FPE_FLTRES;
+
+	/* Clear all flags */
+	regs->fpcsr &= ~SPR_FPCSR_ALLF;
+
+	force_sig_fault(SIGFPE, code, (void __user *)regs->pc);
+}
+
 asmlinkage void do_trap(struct pt_regs *regs, unsigned long address)
 {
 	force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)regs->pc);
-- 
2.39.1

Re: [PATCH 3/4] openrisc: Support floating point user api

[Szabolcs Nagy]

* Stafford Horne <shorne@gmail.com> [2023-04-18 17:58:12 +0100]:
> Add support for handling floating point exceptions and forwarding the
> SIGFPE signal to processes.  Also, add fpu state to sigcontext.
> 
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
...
> --- a/arch/openrisc/include/uapi/asm/sigcontext.h
> +++ b/arch/openrisc/include/uapi/asm/sigcontext.h
> @@ -28,6 +28,7 @@
>  
>  struct sigcontext {
>  	struct user_regs_struct regs;  /* needs to be first */
> +	struct __or1k_fpu_state fpu;
>  	unsigned long oldmask;
>  };

this seems to break userspace abi.
glibc and musl have or1k abi without this field.

either this is a new abi where binaries opt-in with some marking
and then the base sigcontext should be unmodified,

or the fp state needs to be added to the signal frame in a way that
does not break existing abi (e.g. end of the struct ?) and also
advertise the new thing via a hwcap, otherwise userspace cannot
make use of it.

unless i'm missing something.

Re: [PATCH 3/4] openrisc: Support floating point user api

[Stafford Horne]

On Mon, Jun 26, 2023 at 11:38:40PM +0200, Szabolcs Nagy wrote:
> * Stafford Horne <shorne@gmail.com> [2023-04-18 17:58:12 +0100]:
> > Add support for handling floating point exceptions and forwarding the
> > SIGFPE signal to processes.  Also, add fpu state to sigcontext.
> > 
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> ...
> > --- a/arch/openrisc/include/uapi/asm/sigcontext.h
> > +++ b/arch/openrisc/include/uapi/asm/sigcontext.h
> > @@ -28,6 +28,7 @@
> >  
> >  struct sigcontext {
> >  	struct user_regs_struct regs;  /* needs to be first */
> > +	struct __or1k_fpu_state fpu;
> >  	unsigned long oldmask;
> >  };
> 
> this seems to break userspace abi.
> glibc and musl have or1k abi without this field.
> 
> either this is a new abi where binaries opt-in with some marking
> and then the base sigcontext should be unmodified,
> 
> or the fp state needs to be added to the signal frame in a way that
> does not break existing abi (e.g. end of the struct ?) and also
> advertise the new thing via a hwcap, otherwise userspace cannot
> make use of it.
> 
> unless i'm missing something.

I think you are right, I meant to look into this but it must have slipped
though.  Is this something causing you issues or did you just notice it?

I didn't run into issues when running the glibc test suite, but I may have
missed it.

Just moving this to the end of the sigcontext may be all that is needed.

-Stafford

Re: [PATCH 3/4] openrisc: Support floating point user api

[Szabolcs Nagy]

* Stafford Horne <shorne@gmail.com> [2023-06-27 17:41:03 +0100]:
> On Mon, Jun 26, 2023 at 11:38:40PM +0200, Szabolcs Nagy wrote:
> > * Stafford Horne <shorne@gmail.com> [2023-04-18 17:58:12 +0100]:
> > > Add support for handling floating point exceptions and forwarding the
> > > SIGFPE signal to processes.  Also, add fpu state to sigcontext.
> > > 
> > > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > > ---
> > ...
> > > --- a/arch/openrisc/include/uapi/asm/sigcontext.h
> > > +++ b/arch/openrisc/include/uapi/asm/sigcontext.h
> > > @@ -28,6 +28,7 @@
> > >  
> > >  struct sigcontext {
> > >  	struct user_regs_struct regs;  /* needs to be first */
> > > +	struct __or1k_fpu_state fpu;
> > >  	unsigned long oldmask;
> > >  };
> > 
> > this seems to break userspace abi.
> > glibc and musl have or1k abi without this field.
> > 
> > either this is a new abi where binaries opt-in with some marking
> > and then the base sigcontext should be unmodified,
> > 
> > or the fp state needs to be added to the signal frame in a way that
> > does not break existing abi (e.g. end of the struct ?) and also
> > advertise the new thing via a hwcap, otherwise userspace cannot
> > make use of it.
> > 
> > unless i'm missing something.
> 
> I think you are right, I meant to look into this but it must have slipped
> though.  Is this something causing you issues or did you just notice it?

i noticed it while trying to update musl headers to linux 6.4 uapi.

> I didn't run into issues when running the glibc test suite, but I may have
> missed it.

i would only expect issues when accessing ucontext entries
after uc_mcontext.regs in a signal handler registered with
SA_SIGINFO.

in particular uc_sigmask is after uc_mcontext on or1k and e.g.
musl thread cancellation uses this entry to affect the mask on
signal return which will not work on a 6.4 kernel (not tested).

i don't think glibc has tests for the ucontext signal abi.

> Just moving this to the end of the sigcontext may be all that is needed.

that won't help since uc_sigmask comes after sigcontext in ucontext.
it has to go to the end of ucontext or outside of ucontext then.

one way to have fpu in sigcontext is

struct sigcontext {
	struct user_regs_struct regs;
	unsigned long oldmask;
	char padding[sizeof(__userspace_sigset_t)];
	struct __or1k_fpu_state fpu;
};

but the kernel still has to interpret the padding in a bwcompat
way. (and if libc wants to expose fpu in its ucontext then it
needs a flag day abi break as the ucontext size is abi.)

(part of the userspace uc_sigmask is unused because sigset_t is
larger than necessary so may be that can be reused but this is
a hack as that's libc owned.)

not sure how important this fpu field is, arm does not seem to
have fpu state in ucontext and armhf works.

there may be other ways, i'm adding Rich (musl maintainer) on cc
in case he has an opinion.

Re: [PATCH 3/4] openrisc: Support floating point user api

[Rich Felker]

On Tue, Jun 27, 2023 at 07:56:38PM +0200, Szabolcs Nagy wrote:
> * Stafford Horne <shorne@gmail.com> [2023-06-27 17:41:03 +0100]:
> > On Mon, Jun 26, 2023 at 11:38:40PM +0200, Szabolcs Nagy wrote:
> > > * Stafford Horne <shorne@gmail.com> [2023-04-18 17:58:12 +0100]:
> > > > Add support for handling floating point exceptions and forwarding the
> > > > SIGFPE signal to processes.  Also, add fpu state to sigcontext.
> > > > 
> > > > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > > > ---
> > > ...
> > > > --- a/arch/openrisc/include/uapi/asm/sigcontext.h
> > > > +++ b/arch/openrisc/include/uapi/asm/sigcontext.h
> > > > @@ -28,6 +28,7 @@
> > > >  
> > > >  struct sigcontext {
> > > >  	struct user_regs_struct regs;  /* needs to be first */
> > > > +	struct __or1k_fpu_state fpu;
> > > >  	unsigned long oldmask;
> > > >  };
> > > 
> > > this seems to break userspace abi.
> > > glibc and musl have or1k abi without this field.
> > > 
> > > either this is a new abi where binaries opt-in with some marking
> > > and then the base sigcontext should be unmodified,
> > > 
> > > or the fp state needs to be added to the signal frame in a way that
> > > does not break existing abi (e.g. end of the struct ?) and also
> > > advertise the new thing via a hwcap, otherwise userspace cannot
> > > make use of it.
> > > 
> > > unless i'm missing something.
> > 
> > I think you are right, I meant to look into this but it must have slipped
> > though.  Is this something causing you issues or did you just notice it?
> 
> i noticed it while trying to update musl headers to linux 6.4 uapi.
> 
> > I didn't run into issues when running the glibc test suite, but I may have
> > missed it.
> 
> i would only expect issues when accessing ucontext entries
> after uc_mcontext.regs in a signal handler registered with
> SA_SIGINFO.
> 
> in particular uc_sigmask is after uc_mcontext on or1k and e.g.
> musl thread cancellation uses this entry to affect the mask on
> signal return which will not work on a 6.4 kernel (not tested).
> 
> i don't think glibc has tests for the ucontext signal abi.
> 
> > Just moving this to the end of the sigcontext may be all that is needed.
> 
> that won't help since uc_sigmask comes after sigcontext in ucontext.
> it has to go to the end of ucontext or outside of ucontext then.
> 
> one way to have fpu in sigcontext is
> 
> struct sigcontext {
> 	struct user_regs_struct regs;
> 	unsigned long oldmask;
> 	char padding[sizeof(__userspace_sigset_t)];
> 	struct __or1k_fpu_state fpu;
> };
> 
> but the kernel still has to interpret the padding in a bwcompat
> way. (and if libc wants to expose fpu in its ucontext then it
> needs a flag day abi break as the ucontext size is abi.)
> 
> (part of the userspace uc_sigmask is unused because sigset_t is
> larger than necessary so may be that can be reused but this is
> a hack as that's libc owned.)
> 
> not sure how important this fpu field is, arm does not seem to
> have fpu state in ucontext and armhf works.
> 
> there may be other ways, i'm adding Rich (musl maintainer) on cc
> in case he has an opinion.

Indeed, mcontext_t cannot be modified because uc_sigmask follows it in
ucontext_t. The only clean solution here is probably to store the
additional data at offsets past

	sizeof(struct sigcontext) + sizeof(sigset_t)

and not expose this at all in the uapi types. Some hwcap flag can
inform userspace that this additional space is present and accessible
if that's needed.

Optionally you could consider exposing this in the uapi headers'
ucontext_t structure; whether it's an API breakage depends on whether
userspace is relying on being able to allocate its own ucontext_t etc.
This would leave the actual userspace headers (provided by libc) free
to decide whether to modify their type or not according to an
assessment of whether it's a breaking change to application linkage.

What's not workable though is the ABI break that shipped in 6.4. It's
a serious violation of "don't break userspace" and makes existing
application binaries just *not work* (cancellation breaks and possibly
corrupts program state). This needs to be reverted.

Rich

Re: [PATCH 3/4] openrisc: Support floating point user api

[Stafford Horne]

On Tue, Jun 27, 2023 at 03:27:19PM -0400, Rich Felker wrote:
> On Tue, Jun 27, 2023 at 07:56:38PM +0200, Szabolcs Nagy wrote:
> > * Stafford Horne <shorne@gmail.com> [2023-06-27 17:41:03 +0100]:
> > > On Mon, Jun 26, 2023 at 11:38:40PM +0200, Szabolcs Nagy wrote:
> > > > * Stafford Horne <shorne@gmail.com> [2023-04-18 17:58:12 +0100]:
> > > > > Add support for handling floating point exceptions and forwarding the
> > > > > SIGFPE signal to processes.  Also, add fpu state to sigcontext.
> > > > > 
> > > > > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > > > > ---
> > > > ...
> > > > > --- a/arch/openrisc/include/uapi/asm/sigcontext.h
> > > > > +++ b/arch/openrisc/include/uapi/asm/sigcontext.h
> > > > > @@ -28,6 +28,7 @@
> > > > >  
> > > > >  struct sigcontext {
> > > > >  	struct user_regs_struct regs;  /* needs to be first */
> > > > > +	struct __or1k_fpu_state fpu;
> > > > >  	unsigned long oldmask;
> > > > >  };
> > > > 
> > > > this seems to break userspace abi.
> > > > glibc and musl have or1k abi without this field.
> > > > 
> > > > either this is a new abi where binaries opt-in with some marking
> > > > and then the base sigcontext should be unmodified,
> > > > 
> > > > or the fp state needs to be added to the signal frame in a way that
> > > > does not break existing abi (e.g. end of the struct ?) and also
> > > > advertise the new thing via a hwcap, otherwise userspace cannot
> > > > make use of it.
> > > > 
> > > > unless i'm missing something.
> > > 
> > > I think you are right, I meant to look into this but it must have slipped
> > > though.  Is this something causing you issues or did you just notice it?
> > 
> > i noticed it while trying to update musl headers to linux 6.4 uapi.
> > 
> > > I didn't run into issues when running the glibc test suite, but I may have
> > > missed it.
> > 
> > i would only expect issues when accessing ucontext entries
> > after uc_mcontext.regs in a signal handler registered with
> > SA_SIGINFO.
> > 
> > in particular uc_sigmask is after uc_mcontext on or1k and e.g.
> > musl thread cancellation uses this entry to affect the mask on
> > signal return which will not work on a 6.4 kernel (not tested).
> > 
> > i don't think glibc has tests for the ucontext signal abi.
> > 
> > > Just moving this to the end of the sigcontext may be all that is needed.
> > 
> > that won't help since uc_sigmask comes after sigcontext in ucontext.
> > it has to go to the end of ucontext or outside of ucontext then.
> > 
> > one way to have fpu in sigcontext is
> > 
> > struct sigcontext {
> > 	struct user_regs_struct regs;
> > 	unsigned long oldmask;
> > 	char padding[sizeof(__userspace_sigset_t)];
> > 	struct __or1k_fpu_state fpu;
> > };
> > 
> > but the kernel still has to interpret the padding in a bwcompat
> > way. (and if libc wants to expose fpu in its ucontext then it
> > needs a flag day abi break as the ucontext size is abi.)
> > 
> > (part of the userspace uc_sigmask is unused because sigset_t is
> > larger than necessary so may be that can be reused but this is
> > a hack as that's libc owned.)
> > 
> > not sure how important this fpu field is, arm does not seem to
> > have fpu state in ucontext and armhf works.
> > 
> > there may be other ways, i'm adding Rich (musl maintainer) on cc
> > in case he has an opinion.
> 
> Indeed, mcontext_t cannot be modified because uc_sigmask follows it in
> ucontext_t. The only clean solution here is probably to store the
> additional data at offsets past
> 
> 	sizeof(struct sigcontext) + sizeof(sigset_t)
> 
> and not expose this at all in the uapi types. Some hwcap flag can
> inform userspace that this additional space is present and accessible
> if that's needed.
> 
> Optionally you could consider exposing this in the uapi headers'
> ucontext_t structure; whether it's an API breakage depends on whether
> userspace is relying on being able to allocate its own ucontext_t etc.
> This would leave the actual userspace headers (provided by libc) free
> to decide whether to modify their type or not according to an
> assessment of whether it's a breaking change to application linkage.
> 
> What's not workable though is the ABI break that shipped in 6.4. It's
> a serious violation of "don't break userspace" and makes existing
> application binaries just *not work* (cancellation breaks and possibly
> corrupts program state). This needs to be reverted.

Hi Szabolcs, Rich,

Let me work on reverting the bits that try to expose fpcsr in sigcontext.  I am
very aware of rules about not breaking userspace, but for some reason this was
completely missed.

I don't think we do have any need to expose this to userspace at the moment so I
prefer to just leave the fpu state out of sigcontext if that is usable.

The fix will take me about a day or two to get tested and sent.

-Stafford

Re: [PATCH 3/4] openrisc: Support floating point user api

[Stafford Horne]

On Tue, Jun 27, 2023 at 03:27:19PM -0400, Rich Felker wrote:
> On Tue, Jun 27, 2023 at 07:56:38PM +0200, Szabolcs Nagy wrote:
> > * Stafford Horne <shorne@gmail.com> [2023-06-27 17:41:03 +0100]:
> > > On Mon, Jun 26, 2023 at 11:38:40PM +0200, Szabolcs Nagy wrote:
> > > > * Stafford Horne <shorne@gmail.com> [2023-04-18 17:58:12 +0100]:
> > > > > Add support for handling floating point exceptions and forwarding the
> > > > > SIGFPE signal to processes.  Also, add fpu state to sigcontext.
> > > > > 
> > > > > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > > > > ---
> > > > ...
> > > > > --- a/arch/openrisc/include/uapi/asm/sigcontext.h
> > > > > +++ b/arch/openrisc/include/uapi/asm/sigcontext.h
> > > > > @@ -28,6 +28,7 @@
> > > > >  
> > > > >  struct sigcontext {
> > > > >  	struct user_regs_struct regs;  /* needs to be first */
> > > > > +	struct __or1k_fpu_state fpu;
> > > > >  	unsigned long oldmask;
> > > > >  };
> > > > 
> > > > this seems to break userspace abi.
> > > > glibc and musl have or1k abi without this field.
> > > > 
> > > > either this is a new abi where binaries opt-in with some marking
> > > > and then the base sigcontext should be unmodified,
> > > > 
> > > > or the fp state needs to be added to the signal frame in a way that
> > > > does not break existing abi (e.g. end of the struct ?) and also
> > > > advertise the new thing via a hwcap, otherwise userspace cannot
> > > > make use of it.
> > > > 
> > > > unless i'm missing something.
> > > 
> > > I think you are right, I meant to look into this but it must have slipped
> > > though.  Is this something causing you issues or did you just notice it?
> > 
> > i noticed it while trying to update musl headers to linux 6.4 uapi.
> > 
> > > I didn't run into issues when running the glibc test suite, but I may have
> > > missed it.
> > 
> > i would only expect issues when accessing ucontext entries
> > after uc_mcontext.regs in a signal handler registered with
> > SA_SIGINFO.
> > 
> > in particular uc_sigmask is after uc_mcontext on or1k and e.g.
> > musl thread cancellation uses this entry to affect the mask on
> > signal return which will not work on a 6.4 kernel (not tested).
> > 
> > i don't think glibc has tests for the ucontext signal abi.
> > 
> > > Just moving this to the end of the sigcontext may be all that is needed.
> > 
> > that won't help since uc_sigmask comes after sigcontext in ucontext.
> > it has to go to the end of ucontext or outside of ucontext then.
> > 
> > one way to have fpu in sigcontext is
> > 
> > struct sigcontext {
> > 	struct user_regs_struct regs;
> > 	unsigned long oldmask;
> > 	char padding[sizeof(__userspace_sigset_t)];
> > 	struct __or1k_fpu_state fpu;
> > };
> > 
> > but the kernel still has to interpret the padding in a bwcompat
> > way. (and if libc wants to expose fpu in its ucontext then it
> > needs a flag day abi break as the ucontext size is abi.)
> > 
> > (part of the userspace uc_sigmask is unused because sigset_t is
> > larger than necessary so may be that can be reused but this is
> > a hack as that's libc owned.)
> > 
> > not sure how important this fpu field is, arm does not seem to
> > have fpu state in ucontext and armhf works.
> > 
> > there may be other ways, i'm adding Rich (musl maintainer) on cc
> > in case he has an opinion.
> 
> Indeed, mcontext_t cannot be modified because uc_sigmask follows it in
> ucontext_t. The only clean solution here is probably to store the
> additional data at offsets past
> 
> 	sizeof(struct sigcontext) + sizeof(sigset_t)
> 
> and not expose this at all in the uapi types. Some hwcap flag can
> inform userspace that this additional space is present and accessible
> if that's needed.
> 
> Optionally you could consider exposing this in the uapi headers'
> ucontext_t structure; whether it's an API breakage depends on whether
> userspace is relying on being able to allocate its own ucontext_t etc.
> This would leave the actual userspace headers (provided by libc) free
> to decide whether to modify their type or not according to an
> assessment of whether it's a breaking change to application linkage.
> 
> What's not workable though is the ABI break that shipped in 6.4. It's
> a serious violation of "don't break userspace" and makes existing
> application binaries just *not work* (cancellation breaks and possibly
> corrupts program state). This needs to be reverted.

Hi Szabolcs, Rich,

My fix for this has now made it into 6.4.5 stable release.  You should be able
to use this release to update musl.

The fix was to use some unused space in sigcontext, for the fpu state.

-Stafford

[PATCH 4/4] openrisc: Add floating point regset

[Stafford Horne]

Define REGSET_FPU to allow reading and writing the FPCSR fpu state
register.  This will be used primarily by debuggers like GDB.

Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 arch/openrisc/kernel/ptrace.c | 37 +++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/arch/openrisc/kernel/ptrace.c b/arch/openrisc/kernel/ptrace.c
index 85ace93fc251..f1fc276d46bb 100644
--- a/arch/openrisc/kernel/ptrace.c
+++ b/arch/openrisc/kernel/ptrace.c
@@ -84,11 +84,40 @@ static int genregs_set(struct task_struct *target,
 	return ret;
 }
 
+/*
+ * As OpenRISC shares GPRs and floating point registers we don't need to export
+ * the floating point registers again.  So here we only export the fpcsr special
+ * purpose register.
+ */
+static int fpregs_get(struct task_struct *target,
+		       const struct user_regset *regset,
+		       struct membuf to)
+{
+	const struct pt_regs *regs = task_pt_regs(target);
+
+	return membuf_store(&to, regs->fpcsr);
+}
+
+static int fpregs_set(struct task_struct *target,
+		       const struct user_regset *regset,
+		       unsigned int pos, unsigned int count,
+		       const void *kbuf, const void __user *ubuf)
+{
+	struct pt_regs *regs = task_pt_regs(target);
+	int ret;
+
+	/* FPCSR */
+	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
+				 &regs->fpcsr, 0, 4);
+	return ret;
+}
+
 /*
  * Define the register sets available on OpenRISC under Linux
  */
 enum or1k_regset {
 	REGSET_GENERAL,
+	REGSET_FPU,
 };
 
 static const struct user_regset or1k_regsets[] = {
@@ -100,6 +129,14 @@ static const struct user_regset or1k_regsets[] = {
 			    .regset_get = genregs_get,
 			    .set = genregs_set,
 			    },
+	[REGSET_FPU] = {
+			    .core_note_type = NT_PRFPREG,
+			    .n = sizeof(struct __or1k_fpu_state) / sizeof(long),
+			    .size = sizeof(long),
+			    .align = sizeof(long),
+			    .regset_get = fpregs_get,
+			    .set = fpregs_set,
+			    },
 };
 
 static const struct user_regset_view user_or1k_native_view = {
-- 
2.39.1