[RFC][PATCH] x86: dovetail: Permit to declare a trap handled

[RFC][PATCH] x86: dovetail: Permit to declare a trap handled

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

This allows to skip reporting a trap to Linux when the OOB handler
resolved it directly. The handler can signal this condition by returning
"true". This feature was present in I-pipe as well.

By intention, not all spots of oob_trap_notify are enabled for this.
Debug traps are left out as well as asyncpf in kvm - no use cases from
OOB-perspective.

NOTE: breaks unprepared users, breaks non-x86.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

I'm happy to add non-x86 support as well if the direction is fine.

And if we introduce that to dovetail, we will have to add some kernel 
version checks to Xenomai so that it does the right thing with the right 
dovetail versions. Feature flags are out of fashion there as far as I 
understood.

 arch/x86/kernel/traps.c  | 38 ++++++++++++++++++++++----------------
 arch/x86/mm/fault.c      |  9 ++++++---
 include/linux/dovetail.h |  9 +++++----
 kernel/dovetail.c        |  9 ++++++---
 4 files changed, 39 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 3d04a80a1464..daf222f444f4 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -163,10 +163,12 @@ static void show_signal(struct task_struct *tsk, int signr,
 }
 
 static __always_inline
-void mark_trap_entry(int trapnr, struct pt_regs *regs)
+bool mark_trap_entry(int trapnr, struct pt_regs *regs)
 {
-	oob_trap_notify(trapnr, regs);
+	if (oob_trap_notify(trapnr, regs))
+		return false;
 	hard_cond_local_irq_enable();
+	return true;
 }
 
 static __always_inline
@@ -211,9 +213,8 @@ static void do_error_trap(struct pt_regs *regs, long error_code, char *str,
 {
 	RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
 
-	mark_trap_entry(trapnr, regs);
-
-	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
+	if (mark_trap_entry(trapnr, regs) &&
+	    notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
 			NOTIFY_STOP) {
 		cond_local_irq_enable(regs);
 		do_trap(trapnr, signr, str, regs, error_code, sicode, addr);
@@ -375,10 +376,12 @@ DEFINE_IDTENTRY_RAW(exc_invalid_op)
 		return;
 
 	state = irqentry_enter(regs);
-	mark_trap_entry(X86_TRAP_UD, regs);
+	if (!mark_trap_entry(X86_TRAP_UD, regs))
+		goto mark_exit;
 	instrumentation_begin();
 	handle_invalid_op(regs);
 	instrumentation_end();
+mark_exit:
 	mark_trap_exit(X86_TRAP_UD, regs);
 	irqentry_exit(regs, state);
 }
@@ -411,7 +414,8 @@ DEFINE_IDTENTRY_ERRORCODE(exc_alignment_check)
 {
 	char *str = "alignment check";
 
-	mark_trap_entry(X86_TRAP_AC, regs);
+	if (!mark_trap_entry(X86_TRAP_AC, regs))
+		goto mark_exit;
 
 	if (notify_die(DIE_TRAP, str, regs, error_code, X86_TRAP_AC, SIGBUS) == NOTIFY_STOP)
 		goto mark_exit;
@@ -595,7 +599,8 @@ DEFINE_IDTENTRY_DF(exc_double_fault)
 
 DEFINE_IDTENTRY(exc_bounds)
 {
-	mark_trap_entry(X86_TRAP_BR, regs);
+	if (!mark_trap_entry(X86_TRAP_BR, regs))
+		goto out;
 
 	if (notify_die(DIE_TRAP, "bounds", regs, 0,
 			X86_TRAP_BR, SIGSEGV) == NOTIFY_STOP)
@@ -806,7 +811,8 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
 		if (fixup_vdso_exception(regs, X86_TRAP_GP, error_code, 0))
 			goto exit;
 
-		mark_trap_entry(X86_TRAP_GP, regs);
+		if (!mark_trap_entry(X86_TRAP_GP, regs))
+			goto mark_exit;
 		gp_user_force_sig_segv(regs, X86_TRAP_GP, error_code, desc);
 		goto mark_exit;
 	}
@@ -1273,7 +1279,8 @@ static void math_error(struct pt_regs *regs, int trapnr)
 		if (fixup_exception(regs, trapnr, 0, 0))
 			goto exit;
 
-		mark_trap_entry(trapnr, regs);
+		if (!mark_trap_entry(trapnr, regs))
+			goto mark_exit;
 		task->thread.error_code = 0;
 		task->thread.trap_nr = trapnr;
 
@@ -1300,10 +1307,9 @@ static void math_error(struct pt_regs *regs, int trapnr)
 	if (fixup_vdso_exception(regs, trapnr, 0, 0))
 		goto exit;
 
-	mark_trap_entry(trapnr, regs);
-
-	force_sig_fault(SIGFPE, si_code,
-			(void __user *)uprobe_get_trap_addr(regs));
+	if (mark_trap_entry(trapnr, regs))
+		force_sig_fault(SIGFPE, si_code,
+				(void __user *)uprobe_get_trap_addr(regs));
 mark_exit:
 	mark_trap_exit(trapnr, regs);
 exit:
@@ -1416,8 +1422,8 @@ DEFINE_IDTENTRY(exc_device_not_available)
 		 * to kill the task than getting stuck in a never-ending
 		 * loop of #NM faults.
 		 */
-		mark_trap_entry(X86_TRAP_NM, regs);
-		die("unexpected #NM exception", regs, 0);
+		if (mark_trap_entry(X86_TRAP_NM, regs))
+			die("unexpected #NM exception", regs, 0);
 		mark_trap_exit(X86_TRAP_NM, regs);
 	}
 }
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 834a6edd0be5..1cffd321878f 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -650,7 +650,8 @@ page_fault_oops(struct pt_regs *regs, unsigned long error_code,
 	 * Do not bother unwinding the notification context on
 	 * CPU/firmware/kernel bug.
 	 */
-	oob_trap_notify(X86_TRAP_PF, regs);
+	if (oob_trap_notify(X86_TRAP_PF, regs))
+		return;
 
 #ifdef CONFIG_VMAP_STACK
 	/*
@@ -890,7 +891,8 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 	if (fixup_vdso_exception(regs, X86_TRAP_PF, error_code, address))
 		return;
 
-	oob_trap_notify(X86_TRAP_PF, regs);
+	if (oob_trap_notify(X86_TRAP_PF, regs))
+		return;
 
 	if (likely(show_unhandled_signals))
 		show_signal_msg(regs, error_code, address, tsk);
@@ -1363,7 +1365,8 @@ void do_user_addr_fault(struct pt_regs *regs,
 	 * event, so that it might switch current to in-band mode if
 	 * need be.
 	 */
-	oob_trap_notify(X86_TRAP_PF, regs);
+	if (oob_trap_notify(X86_TRAP_PF, regs))
+		return;
 
 	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
 
diff --git a/include/linux/dovetail.h b/include/linux/dovetail.h
index 6a244a44ab1f..4bd27531f9e5 100644
--- a/include/linux/dovetail.h
+++ b/include/linux/dovetail.h
@@ -58,14 +58,15 @@ void inband_task_init(struct task_struct *p);
 
 int pipeline_syscall(unsigned int nr, struct pt_regs *regs);
 
-void __oob_trap_notify(unsigned int exception,
+bool __oob_trap_notify(unsigned int exception,
 		       struct pt_regs *regs);
 
-static __always_inline void oob_trap_notify(unsigned int exception,
+static __always_inline bool oob_trap_notify(unsigned int exception,
 					struct pt_regs *regs)
 {
 	if (running_oob() && !test_thread_local_flags(_TLF_OOBTRAP))
-		__oob_trap_notify(exception, regs);
+		return __oob_trap_notify(exception, regs);
+	return false;
 }
 
 void __oob_trap_unwind(unsigned int exception,
@@ -248,7 +249,7 @@ static inline void arch_dovetail_exec_prepare(void)
  * Keep the trap helpers as macros, we might not be able to resolve
  * trap numbers if CONFIG_DOVETAIL is off.
  */
-#define oob_trap_notify(__exception, __regs)	do { } while (0)
+#define oob_trap_notify(__exception, __regs)	false
 #define oob_trap_unwind(__exception, __regs)	do { } while (0)
 
 static inline
diff --git a/kernel/dovetail.c b/kernel/dovetail.c
index 79fd75918b37..167dc61dbcce 100644
--- a/kernel/dovetail.c
+++ b/kernel/dovetail.c
@@ -254,14 +254,16 @@ int pipeline_syscall(unsigned int nr, struct pt_regs *regs)
 	return 0; /* pass syscall down to the in-band dispatcher. */
 }
 
-void __weak handle_oob_trap_entry(unsigned int trapnr, struct pt_regs *regs)
+bool __weak handle_oob_trap_entry(unsigned int trapnr, struct pt_regs *regs)
 {
+	return false;
 }
 
-noinstr void __oob_trap_notify(unsigned int exception,
+noinstr bool __oob_trap_notify(unsigned int exception,
 			       struct pt_regs *regs)
 {
 	unsigned long flags;
+	bool handled;
 
 	/*
 	 * We send a notification about exceptions raised over a
@@ -273,10 +275,11 @@ noinstr void __oob_trap_notify(unsigned int exception,
 		set_thread_local_flags(_TLF_OOBTRAP);
 		flags = hard_local_irq_save();
 		instrumentation_begin();
-		handle_oob_trap_entry(exception, regs);
+		handled = handle_oob_trap_entry(exception, regs);
 		instrumentation_end();
 		hard_local_irq_restore(flags);
 	}
+	return handled;
 }
 
 void __weak handle_oob_trap_exit(unsigned int trapnr, struct pt_regs *regs)
-- 
2.35.3

Re: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled

[Jan Kiszka]

On 08.07.22 09:11, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> This allows to skip reporting a trap to Linux when the OOB handler
> resolved it directly. The handler can signal this condition by returning
> "true". This feature was present in I-pipe as well.
> 
> By intention, not all spots of oob_trap_notify are enabled for this.
> Debug traps are left out as well as asyncpf in kvm - no use cases from
> OOB-perspective.
> 
> NOTE: breaks unprepared users, breaks non-x86.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> I'm happy to add non-x86 support as well if the direction is fine.
> 
> And if we introduce that to dovetail, we will have to add some kernel 
> version checks to Xenomai so that it does the right thing with the right 
> dovetail versions. Feature flags are out of fashion there as far as I 
> understood.
> 

And I forgot to write that this if for v5.19-dovetail-rebase. I have a
5.10 version locally as well, the one we use in a downstream deployment
for some months now.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

RE: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled

[Johannes Kirchmair]

Hello all,

I just recently found the time to test this patch in the context of our rt_signals.
As a matter of fact it works quite well for us.

So is there any intent/possibility to bring this upstream?

I also did some similar changes for arm64. As we also use imx8 boards.
If there is interest in those, I could post them.

Best regards
Johannes

> -----Original Message-----
> From: Jan Kiszka <jan.kiszka@siemens.com>
> Sent: Freitag, 8. Juli 2022 09:13
> To: Xenomai <xenomai@lists.linux.dev>; Philippe Gerum <rpm@xenomai.org>;
> Johannes Kirchmair <johannes.kirchmair@sigmatek.at>; Richard Weinberger
> <richard@nod.at>
> Subject: Re: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled
> 
> On 08.07.22 09:11, Jan Kiszka wrote:
> > From: Jan Kiszka <jan.kiszka@siemens.com>
> >
> > This allows to skip reporting a trap to Linux when the OOB handler
> > resolved it directly. The handler can signal this condition by returning
> > "true". This feature was present in I-pipe as well.
> >
> > By intention, not all spots of oob_trap_notify are enabled for this.
> > Debug traps are left out as well as asyncpf in kvm - no use cases from
> > OOB-perspective.
> >
> > NOTE: breaks unprepared users, breaks non-x86.
> >
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > ---
> >
> > I'm happy to add non-x86 support as well if the direction is fine.
> >
> > And if we introduce that to dovetail, we will have to add some kernel
> > version checks to Xenomai so that it does the right thing with the right
> > dovetail versions. Feature flags are out of fashion there as far as I
> > understood.
> >
> 
> And I forgot to write that this if for v5.19-dovetail-rebase. I have a
> 5.10 version locally as well, the one we use in a downstream deployment
> for some months now.
> 
> Jan
> 
> --
> Siemens AG, Technology
> Competence Center Embedded Linux

RE: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled

[Johannes Kirchmair]

Dear Jan

> -----Original Message-----
> From: Jan Kiszka <jan.kiszka@siemens.com>
> Sent: Freitag, 8. Juli 2022 09:11
> To: Xenomai <xenomai@lists.linux.dev>; Philippe Gerum <rpm@xenomai.org>;
> Johannes Kirchmair <johannes.kirchmair@sigmatek.at>; Richard Weinberger
> <richard@nod.at>
> Subject: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled
> 
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> This allows to skip reporting a trap to Linux when the OOB handler
> resolved it directly. The handler can signal this condition by returning
> "true". This feature was present in I-pipe as well.
> 
> By intention, not all spots of oob_trap_notify are enabled for this.
> Debug traps are left out as well as asyncpf in kvm - no use cases from
> OOB-perspective.
> 
> NOTE: breaks unprepared users, breaks non-x86.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> I'm happy to add non-x86 support as well if the direction is fine.
> 
> And if we introduce that to dovetail, we will have to add some kernel
> version checks to Xenomai so that it does the right thing with the right
> dovetail versions. Feature flags are out of fashion there as far as I
> understood.
> 
>  arch/x86/kernel/traps.c  | 38 ++++++++++++++++++++++----------------
>  arch/x86/mm/fault.c      |  9 ++++++---
>  include/linux/dovetail.h |  9 +++++----
>  kernel/dovetail.c        |  9 ++++++---
>  4 files changed, 39 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 3d04a80a1464..daf222f444f4 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -163,10 +163,12 @@ static void show_signal(struct task_struct *tsk, int signr,
>  }
> 
>  static __always_inline
> -void mark_trap_entry(int trapnr, struct pt_regs *regs)
> +bool mark_trap_entry(int trapnr, struct pt_regs *regs)
>  {
> -	oob_trap_notify(trapnr, regs);
> +	if (oob_trap_notify(trapnr, regs))
> +		return false;
>  	hard_cond_local_irq_enable();
> +	return true;
>  }
> 
>  static __always_inline
> @@ -211,9 +213,8 @@ static void do_error_trap(struct pt_regs *regs, long
> error_code, char *str,
>  {
>  	RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake
> RCU");
> 
> -	mark_trap_entry(trapnr, regs);
> -
> -	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
> +	if (mark_trap_entry(trapnr, regs) &&
> +	    notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
>  			NOTIFY_STOP) {
>  		cond_local_irq_enable(regs);
>  		do_trap(trapnr, signr, str, regs, error_code, sicode, addr);
> @@ -375,10 +376,12 @@ DEFINE_IDTENTRY_RAW(exc_invalid_op)
>  		return;
> 
>  	state = irqentry_enter(regs);
> -	mark_trap_entry(X86_TRAP_UD, regs);
> +	if (!mark_trap_entry(X86_TRAP_UD, regs))
> +		goto mark_exit;
>  	instrumentation_begin();
>  	handle_invalid_op(regs);
>  	instrumentation_end();
> +mark_exit:
>  	mark_trap_exit(X86_TRAP_UD, regs);
>  	irqentry_exit(regs, state);
>  }
> @@ -411,7 +414,8 @@ DEFINE_IDTENTRY_ERRORCODE(exc_alignment_check)
>  {
>  	char *str = "alignment check";
> 
> -	mark_trap_entry(X86_TRAP_AC, regs);
> +	if (!mark_trap_entry(X86_TRAP_AC, regs))
> +		goto mark_exit;
> 
>  	if (notify_die(DIE_TRAP, str, regs, error_code, X86_TRAP_AC, SIGBUS) ==
> NOTIFY_STOP)
>  		goto mark_exit;
> @@ -595,7 +599,8 @@ DEFINE_IDTENTRY_DF(exc_double_fault)
> 
>  DEFINE_IDTENTRY(exc_bounds)
>  {
> -	mark_trap_entry(X86_TRAP_BR, regs);
> +	if (!mark_trap_entry(X86_TRAP_BR, regs))
> +		goto out;
> 
>  	if (notify_die(DIE_TRAP, "bounds", regs, 0,
>  			X86_TRAP_BR, SIGSEGV) == NOTIFY_STOP)
> @@ -806,7 +811,8 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
>  		if (fixup_vdso_exception(regs, X86_TRAP_GP, error_code, 0))
>  			goto exit;
> 
> -		mark_trap_entry(X86_TRAP_GP, regs);
> +		if (!mark_trap_entry(X86_TRAP_GP, regs))
> +			goto mark_exit;
>  		gp_user_force_sig_segv(regs, X86_TRAP_GP, error_code, desc);
>  		goto mark_exit;
>  	}
> @@ -1273,7 +1279,8 @@ static void math_error(struct pt_regs *regs, int trapnr)
>  		if (fixup_exception(regs, trapnr, 0, 0))
>  			goto exit;
> 
> -		mark_trap_entry(trapnr, regs);
> +		if (!mark_trap_entry(trapnr, regs))
> +			goto mark_exit;
>  		task->thread.error_code = 0;
>  		task->thread.trap_nr = trapnr;
> 
> @@ -1300,10 +1307,9 @@ static void math_error(struct pt_regs *regs, int trapnr)
>  	if (fixup_vdso_exception(regs, trapnr, 0, 0))
>  		goto exit;
> 
> -	mark_trap_entry(trapnr, regs);
> -
> -	force_sig_fault(SIGFPE, si_code,
> -			(void __user *)uprobe_get_trap_addr(regs));
> +	if (mark_trap_entry(trapnr, regs))
> +		force_sig_fault(SIGFPE, si_code,
> +				(void __user *)uprobe_get_trap_addr(regs));
>  mark_exit:
>  	mark_trap_exit(trapnr, regs);
>  exit:
> @@ -1416,8 +1422,8 @@ DEFINE_IDTENTRY(exc_device_not_available)
>  		 * to kill the task than getting stuck in a never-ending
>  		 * loop of #NM faults.
>  		 */
> -		mark_trap_entry(X86_TRAP_NM, regs);
> -		die("unexpected #NM exception", regs, 0);
> +		if (mark_trap_entry(X86_TRAP_NM, regs))
> +			die("unexpected #NM exception", regs, 0);
>  		mark_trap_exit(X86_TRAP_NM, regs);
>  	}
>  }
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 834a6edd0be5..1cffd321878f 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -650,7 +650,8 @@ page_fault_oops(struct pt_regs *regs, unsigned long
> error_code,
>  	 * Do not bother unwinding the notification context on
>  	 * CPU/firmware/kernel bug.
>  	 */
> -	oob_trap_notify(X86_TRAP_PF, regs);
> +	if (oob_trap_notify(X86_TRAP_PF, regs))
> +		return;
> 
>  #ifdef CONFIG_VMAP_STACK
>  	/*
> @@ -890,7 +891,8 @@ __bad_area_nosemaphore(struct pt_regs *regs,
> unsigned long error_code,
>  	if (fixup_vdso_exception(regs, X86_TRAP_PF, error_code, address))
>  		return;
> 
> -	oob_trap_notify(X86_TRAP_PF, regs);
> +	if (oob_trap_notify(X86_TRAP_PF, regs))
> +		return;
> 
>  	if (likely(show_unhandled_signals))
>  		show_signal_msg(regs, error_code, address, tsk);
> @@ -1363,7 +1365,8 @@ void do_user_addr_fault(struct pt_regs *regs,
>  	 * event, so that it might switch current to in-band mode if
>  	 * need be.
>  	 */
> -	oob_trap_notify(X86_TRAP_PF, regs);
> +	if (oob_trap_notify(X86_TRAP_PF, regs))
> +		return;
> 
>  	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
> 
> diff --git a/include/linux/dovetail.h b/include/linux/dovetail.h
> index 6a244a44ab1f..4bd27531f9e5 100644
> --- a/include/linux/dovetail.h
> +++ b/include/linux/dovetail.h
> @@ -58,14 +58,15 @@ void inband_task_init(struct task_struct *p);
> 
>  int pipeline_syscall(unsigned int nr, struct pt_regs *regs);
> 
> -void __oob_trap_notify(unsigned int exception,
> +bool __oob_trap_notify(unsigned int exception,
>  		       struct pt_regs *regs);
> 
> -static __always_inline void oob_trap_notify(unsigned int exception,
> +static __always_inline bool oob_trap_notify(unsigned int exception,
>  					struct pt_regs *regs)
>  {
>  	if (running_oob() && !test_thread_local_flags(_TLF_OOBTRAP))
> -		__oob_trap_notify(exception, regs);
> +		return __oob_trap_notify(exception, regs);
> +	return false;
>  }
> 
>  void __oob_trap_unwind(unsigned int exception,
> @@ -248,7 +249,7 @@ static inline void arch_dovetail_exec_prepare(void)
>   * Keep the trap helpers as macros, we might not be able to resolve
>   * trap numbers if CONFIG_DOVETAIL is off.
>   */
> -#define oob_trap_notify(__exception, __regs)	do { } while (0)
> +#define oob_trap_notify(__exception, __regs)	false
>  #define oob_trap_unwind(__exception, __regs)	do { } while (0)
> 
>  static inline
> diff --git a/kernel/dovetail.c b/kernel/dovetail.c
> index 79fd75918b37..167dc61dbcce 100644
> --- a/kernel/dovetail.c
> +++ b/kernel/dovetail.c
> @@ -254,14 +254,16 @@ int pipeline_syscall(unsigned int nr, struct pt_regs
> *regs)
>  	return 0; /* pass syscall down to the in-band dispatcher. */
>  }
> 
> -void __weak handle_oob_trap_entry(unsigned int trapnr, struct pt_regs *regs)
> +bool __weak handle_oob_trap_entry(unsigned int trapnr, struct pt_regs *regs)
>  {
> +	return false;
>  }
> 
> -noinstr void __oob_trap_notify(unsigned int exception,
> +noinstr bool __oob_trap_notify(unsigned int exception,
>  			       struct pt_regs *regs)
>  {
>  	unsigned long flags;
> +	bool handled;

Wouldn’t it be better to have this initialized to false?
So if dovetail_enabled is false, we don’t return a not well defined value?
Or do I miss something. 

Best regards Johannes

> 
>  	/*
>  	 * We send a notification about exceptions raised over a
> @@ -273,10 +275,11 @@ noinstr void __oob_trap_notify(unsigned int exception,
>  		set_thread_local_flags(_TLF_OOBTRAP);
>  		flags = hard_local_irq_save();
>  		instrumentation_begin();
> -		handle_oob_trap_entry(exception, regs);
> +		handled = handle_oob_trap_entry(exception, regs);
>  		instrumentation_end();
>  		hard_local_irq_restore(flags);
>  	}
> +	return handled;
>  }
> 
>  void __weak handle_oob_trap_exit(unsigned int trapnr, struct pt_regs *regs)
> --
> 2.35.3

Re: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled

[Jan Kiszka]

On 09.11.22 08:31, Johannes Kirchmair wrote:
> Hello all,
> 
> I just recently found the time to test this patch in the context of our rt_signals.
> As a matter of fact it works quite well for us.
> 
> So is there any intent/possibility to bring this upstream?
> 
> I also did some similar changes for arm64. As we also use imx8 boards.
> If there is interest in those, I could post them.

Yes, please share. Then we can continue with proposing it for the latest
dovetail port (6.1 now).

Jan

> 
> Best regards
> Johannes
> 
>> -----Original Message-----
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>> Sent: Freitag, 8. Juli 2022 09:13
>> To: Xenomai <xenomai@lists.linux.dev>; Philippe Gerum <rpm@xenomai.org>;
>> Johannes Kirchmair <johannes.kirchmair@sigmatek.at>; Richard Weinberger
>> <richard@nod.at>
>> Subject: Re: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled
>>
>> On 08.07.22 09:11, Jan Kiszka wrote:
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> This allows to skip reporting a trap to Linux when the OOB handler
>>> resolved it directly. The handler can signal this condition by returning
>>> "true". This feature was present in I-pipe as well.
>>>
>>> By intention, not all spots of oob_trap_notify are enabled for this.
>>> Debug traps are left out as well as asyncpf in kvm - no use cases from
>>> OOB-perspective.
>>>
>>> NOTE: breaks unprepared users, breaks non-x86.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>
>>> I'm happy to add non-x86 support as well if the direction is fine.
>>>
>>> And if we introduce that to dovetail, we will have to add some kernel
>>> version checks to Xenomai so that it does the right thing with the right
>>> dovetail versions. Feature flags are out of fashion there as far as I
>>> understood.
>>>
>>
>> And I forgot to write that this if for v5.19-dovetail-rebase. I have a
>> 5.10 version locally as well, the one we use in a downstream deployment
>> for some months now.
>>
>> Jan
>>
>> --
>> Siemens AG, Technology
>> Competence Center Embedded Linux

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled

[Jan Kiszka]

On 09.11.22 08:36, Johannes Kirchmair wrote:
> Dear Jan
> 
>> -----Original Message-----
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>> Sent: Freitag, 8. Juli 2022 09:11
>> To: Xenomai <xenomai@lists.linux.dev>; Philippe Gerum <rpm@xenomai.org>;
>> Johannes Kirchmair <johannes.kirchmair@sigmatek.at>; Richard Weinberger
>> <richard@nod.at>
>> Subject: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled
>>
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> This allows to skip reporting a trap to Linux when the OOB handler
>> resolved it directly. The handler can signal this condition by returning
>> "true". This feature was present in I-pipe as well.
>>
>> By intention, not all spots of oob_trap_notify are enabled for this.
>> Debug traps are left out as well as asyncpf in kvm - no use cases from
>> OOB-perspective.
>>
>> NOTE: breaks unprepared users, breaks non-x86.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> I'm happy to add non-x86 support as well if the direction is fine.
>>
>> And if we introduce that to dovetail, we will have to add some kernel
>> version checks to Xenomai so that it does the right thing with the right
>> dovetail versions. Feature flags are out of fashion there as far as I
>> understood.
>>
>>  arch/x86/kernel/traps.c  | 38 ++++++++++++++++++++++----------------
>>  arch/x86/mm/fault.c      |  9 ++++++---
>>  include/linux/dovetail.h |  9 +++++----
>>  kernel/dovetail.c        |  9 ++++++---
>>  4 files changed, 39 insertions(+), 26 deletions(-)
>>
>> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
>> index 3d04a80a1464..daf222f444f4 100644
>> --- a/arch/x86/kernel/traps.c
>> +++ b/arch/x86/kernel/traps.c
>> @@ -163,10 +163,12 @@ static void show_signal(struct task_struct *tsk, int signr,
>>  }
>>
>>  static __always_inline
>> -void mark_trap_entry(int trapnr, struct pt_regs *regs)
>> +bool mark_trap_entry(int trapnr, struct pt_regs *regs)
>>  {
>> -	oob_trap_notify(trapnr, regs);
>> +	if (oob_trap_notify(trapnr, regs))
>> +		return false;
>>  	hard_cond_local_irq_enable();
>> +	return true;
>>  }
>>
>>  static __always_inline
>> @@ -211,9 +213,8 @@ static void do_error_trap(struct pt_regs *regs, long
>> error_code, char *str,
>>  {
>>  	RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake
>> RCU");
>>
>> -	mark_trap_entry(trapnr, regs);
>> -
>> -	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
>> +	if (mark_trap_entry(trapnr, regs) &&
>> +	    notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
>>  			NOTIFY_STOP) {
>>  		cond_local_irq_enable(regs);
>>  		do_trap(trapnr, signr, str, regs, error_code, sicode, addr);
>> @@ -375,10 +376,12 @@ DEFINE_IDTENTRY_RAW(exc_invalid_op)
>>  		return;
>>
>>  	state = irqentry_enter(regs);
>> -	mark_trap_entry(X86_TRAP_UD, regs);
>> +	if (!mark_trap_entry(X86_TRAP_UD, regs))
>> +		goto mark_exit;
>>  	instrumentation_begin();
>>  	handle_invalid_op(regs);
>>  	instrumentation_end();
>> +mark_exit:
>>  	mark_trap_exit(X86_TRAP_UD, regs);
>>  	irqentry_exit(regs, state);
>>  }
>> @@ -411,7 +414,8 @@ DEFINE_IDTENTRY_ERRORCODE(exc_alignment_check)
>>  {
>>  	char *str = "alignment check";
>>
>> -	mark_trap_entry(X86_TRAP_AC, regs);
>> +	if (!mark_trap_entry(X86_TRAP_AC, regs))
>> +		goto mark_exit;
>>
>>  	if (notify_die(DIE_TRAP, str, regs, error_code, X86_TRAP_AC, SIGBUS) ==
>> NOTIFY_STOP)
>>  		goto mark_exit;
>> @@ -595,7 +599,8 @@ DEFINE_IDTENTRY_DF(exc_double_fault)
>>
>>  DEFINE_IDTENTRY(exc_bounds)
>>  {
>> -	mark_trap_entry(X86_TRAP_BR, regs);
>> +	if (!mark_trap_entry(X86_TRAP_BR, regs))
>> +		goto out;
>>
>>  	if (notify_die(DIE_TRAP, "bounds", regs, 0,
>>  			X86_TRAP_BR, SIGSEGV) == NOTIFY_STOP)
>> @@ -806,7 +811,8 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
>>  		if (fixup_vdso_exception(regs, X86_TRAP_GP, error_code, 0))
>>  			goto exit;
>>
>> -		mark_trap_entry(X86_TRAP_GP, regs);
>> +		if (!mark_trap_entry(X86_TRAP_GP, regs))
>> +			goto mark_exit;
>>  		gp_user_force_sig_segv(regs, X86_TRAP_GP, error_code, desc);
>>  		goto mark_exit;
>>  	}
>> @@ -1273,7 +1279,8 @@ static void math_error(struct pt_regs *regs, int trapnr)
>>  		if (fixup_exception(regs, trapnr, 0, 0))
>>  			goto exit;
>>
>> -		mark_trap_entry(trapnr, regs);
>> +		if (!mark_trap_entry(trapnr, regs))
>> +			goto mark_exit;
>>  		task->thread.error_code = 0;
>>  		task->thread.trap_nr = trapnr;
>>
>> @@ -1300,10 +1307,9 @@ static void math_error(struct pt_regs *regs, int trapnr)
>>  	if (fixup_vdso_exception(regs, trapnr, 0, 0))
>>  		goto exit;
>>
>> -	mark_trap_entry(trapnr, regs);
>> -
>> -	force_sig_fault(SIGFPE, si_code,
>> -			(void __user *)uprobe_get_trap_addr(regs));
>> +	if (mark_trap_entry(trapnr, regs))
>> +		force_sig_fault(SIGFPE, si_code,
>> +				(void __user *)uprobe_get_trap_addr(regs));
>>  mark_exit:
>>  	mark_trap_exit(trapnr, regs);
>>  exit:
>> @@ -1416,8 +1422,8 @@ DEFINE_IDTENTRY(exc_device_not_available)
>>  		 * to kill the task than getting stuck in a never-ending
>>  		 * loop of #NM faults.
>>  		 */
>> -		mark_trap_entry(X86_TRAP_NM, regs);
>> -		die("unexpected #NM exception", regs, 0);
>> +		if (mark_trap_entry(X86_TRAP_NM, regs))
>> +			die("unexpected #NM exception", regs, 0);
>>  		mark_trap_exit(X86_TRAP_NM, regs);
>>  	}
>>  }
>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>> index 834a6edd0be5..1cffd321878f 100644
>> --- a/arch/x86/mm/fault.c
>> +++ b/arch/x86/mm/fault.c
>> @@ -650,7 +650,8 @@ page_fault_oops(struct pt_regs *regs, unsigned long
>> error_code,
>>  	 * Do not bother unwinding the notification context on
>>  	 * CPU/firmware/kernel bug.
>>  	 */
>> -	oob_trap_notify(X86_TRAP_PF, regs);
>> +	if (oob_trap_notify(X86_TRAP_PF, regs))
>> +		return;
>>
>>  #ifdef CONFIG_VMAP_STACK
>>  	/*
>> @@ -890,7 +891,8 @@ __bad_area_nosemaphore(struct pt_regs *regs,
>> unsigned long error_code,
>>  	if (fixup_vdso_exception(regs, X86_TRAP_PF, error_code, address))
>>  		return;
>>
>> -	oob_trap_notify(X86_TRAP_PF, regs);
>> +	if (oob_trap_notify(X86_TRAP_PF, regs))
>> +		return;
>>
>>  	if (likely(show_unhandled_signals))
>>  		show_signal_msg(regs, error_code, address, tsk);
>> @@ -1363,7 +1365,8 @@ void do_user_addr_fault(struct pt_regs *regs,
>>  	 * event, so that it might switch current to in-band mode if
>>  	 * need be.
>>  	 */
>> -	oob_trap_notify(X86_TRAP_PF, regs);
>> +	if (oob_trap_notify(X86_TRAP_PF, regs))
>> +		return;
>>
>>  	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
>>
>> diff --git a/include/linux/dovetail.h b/include/linux/dovetail.h
>> index 6a244a44ab1f..4bd27531f9e5 100644
>> --- a/include/linux/dovetail.h
>> +++ b/include/linux/dovetail.h
>> @@ -58,14 +58,15 @@ void inband_task_init(struct task_struct *p);
>>
>>  int pipeline_syscall(unsigned int nr, struct pt_regs *regs);
>>
>> -void __oob_trap_notify(unsigned int exception,
>> +bool __oob_trap_notify(unsigned int exception,
>>  		       struct pt_regs *regs);
>>
>> -static __always_inline void oob_trap_notify(unsigned int exception,
>> +static __always_inline bool oob_trap_notify(unsigned int exception,
>>  					struct pt_regs *regs)
>>  {
>>  	if (running_oob() && !test_thread_local_flags(_TLF_OOBTRAP))
>> -		__oob_trap_notify(exception, regs);
>> +		return __oob_trap_notify(exception, regs);
>> +	return false;
>>  }
>>
>>  void __oob_trap_unwind(unsigned int exception,
>> @@ -248,7 +249,7 @@ static inline void arch_dovetail_exec_prepare(void)
>>   * Keep the trap helpers as macros, we might not be able to resolve
>>   * trap numbers if CONFIG_DOVETAIL is off.
>>   */
>> -#define oob_trap_notify(__exception, __regs)	do { } while (0)
>> +#define oob_trap_notify(__exception, __regs)	false
>>  #define oob_trap_unwind(__exception, __regs)	do { } while (0)
>>
>>  static inline
>> diff --git a/kernel/dovetail.c b/kernel/dovetail.c
>> index 79fd75918b37..167dc61dbcce 100644
>> --- a/kernel/dovetail.c
>> +++ b/kernel/dovetail.c
>> @@ -254,14 +254,16 @@ int pipeline_syscall(unsigned int nr, struct pt_regs
>> *regs)
>>  	return 0; /* pass syscall down to the in-band dispatcher. */
>>  }
>>
>> -void __weak handle_oob_trap_entry(unsigned int trapnr, struct pt_regs *regs)
>> +bool __weak handle_oob_trap_entry(unsigned int trapnr, struct pt_regs *regs)
>>  {
>> +	return false;
>>  }
>>
>> -noinstr void __oob_trap_notify(unsigned int exception,
>> +noinstr bool __oob_trap_notify(unsigned int exception,
>>  			       struct pt_regs *regs)
>>  {
>>  	unsigned long flags;
>> +	bool handled;
> 
> Wouldn’t it be better to have this initialized to false?
> So if dovetail_enabled is false, we don’t return a not well defined value?
> Or do I miss something. 

Yes, this is a bug.

Jan

> 
> Best regards Johannes
> 
>>
>>  	/*
>>  	 * We send a notification about exceptions raised over a
>> @@ -273,10 +275,11 @@ noinstr void __oob_trap_notify(unsigned int exception,
>>  		set_thread_local_flags(_TLF_OOBTRAP);
>>  		flags = hard_local_irq_save();
>>  		instrumentation_begin();
>> -		handle_oob_trap_entry(exception, regs);
>> +		handled = handle_oob_trap_entry(exception, regs);
>>  		instrumentation_end();
>>  		hard_local_irq_restore(flags);
>>  	}
>> +	return handled;
>>  }
>>
>>  void __weak handle_oob_trap_exit(unsigned int trapnr, struct pt_regs *regs)
>> --
>> 2.35.3

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled

[Philippe Gerum]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> On 09.11.22 08:31, Johannes Kirchmair wrote:
>> Hello all,
>> 
>> I just recently found the time to test this patch in the context of our rt_signals.
>> As a matter of fact it works quite well for us.
>> 
>> So is there any intent/possibility to bring this upstream?
>> 
>> I also did some similar changes for arm64. As we also use imx8 boards.
>> If there is interest in those, I could post them.
>
> Yes, please share. Then we can continue with proposing it for the latest
> dovetail port (6.1 now).
>

There is a slightly different approach which expresses the intent of
that notification hook Dovetail-wise:

- if the real-time core switches in-band upon notification, then the
  in-band kernel code may assume that all regular fixups need to be
  applied next. This is the common case for most traps.

- if the real-time core stays on the out-of-band stage upon
  notification, then the in-band kernel code should assume that all
  required fixups were done from that stage already, returning asap from
  the outer trap handler.

This gives the same flexibility than by expecting the hook to return a
continuation status, but makes sure that regular fixups cannot be
bypassed when/if switching to in-band mode is required to handle the
trap. Rationale: the regular in-band trap handlers know better about the
way to deal with those events from in-band context.

e.g.

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index aed5828f784dc..beae6fbf11541 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -161,10 +161,16 @@ static void show_signal(struct task_struct *tsk, int signr,
 }
 
 static __always_inline
-void mark_trap_entry(int trapnr, struct pt_regs *regs)
+bool mark_trap_entry(int trapnr, struct pt_regs *regs)
 {
 	oob_trap_notify(trapnr, regs);
-	hard_cond_local_irq_enable();
+
+	if (likely(running_inband())) {
+		hard_cond_local_irq_enable();
+		return true;
+	}
+
+	return false;
 }
 
 static __always_inline
@@ -175,9 +181,10 @@ void mark_trap_exit(int trapnr, struct pt_regs *regs)
 }
 
 static __always_inline
-void mark_trap_entry_raw(int trapnr, struct pt_regs *regs)
+bool mark_trap_entry_raw(int trapnr, struct pt_regs *regs)
 {
 	oob_trap_notify(trapnr, regs);
+	return running_inband();
 }
 
 static __always_inline
@@ -209,7 +216,8 @@ static void do_error_trap(struct pt_regs *regs, long error_code, char *str,
 {
 	RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
 
-	mark_trap_entry(trapnr, regs);
+	if (!mark_trap_entry(trapnr, regs))
+		return;
 
 	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
 			NOTIFY_STOP) {
@@ -289,7 +297,8 @@ DEFINE_IDTENTRY_RAW(exc_invalid_op)
 {
 	irqentry_state_t state;
 
-	mark_trap_entry_raw(X86_TRAP_UD, regs);
+	if (!mark_trap_entry_raw(X86_TRAP_UD, regs))
+		return;
 
 	/*
 	 * We use UD2 as a short encoding for 'CALL __WARN', as such
@@ -336,7 +345,8 @@ DEFINE_IDTENTRY_ERRORCODE(exc_alignment_check)
 {
 	char *str = "alignment check";
 
-	mark_trap_entry(X86_TRAP_AC, regs);
+	if (!mark_trap_entry(X86_TRAP_AC, regs))
+		return;
 
 	if (notify_die(DIE_TRAP, str, regs, error_code, X86_TRAP_AC, SIGBUS) == NOTIFY_STOP)
 		goto mark_exit;
@@ -520,7 +530,8 @@ DEFINE_IDTENTRY_DF(exc_double_fault)
 
 DEFINE_IDTENTRY(exc_bounds)
 {
-	mark_trap_entry(X86_TRAP_BR, regs);
+	if (!mark_trap_entry(X86_TRAP_BR, regs))
+		return;
 
 	if (notify_die(DIE_TRAP, "bounds", regs, 0,
 			X86_TRAP_BR, SIGSEGV) == NOTIFY_STOP)
@@ -641,13 +652,15 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
 		if (fixup_iopl_exception(regs))
 			goto exit;
 
-		mark_trap_entry(X86_TRAP_GP, regs);
 		tsk->thread.error_code = error_code;
 		tsk->thread.trap_nr = X86_TRAP_GP;
 
 		if (fixup_vdso_exception(regs, X86_TRAP_GP, error_code, 0))
 			goto exit;
 
+		if (!mark_trap_entry(X86_TRAP_GP, regs))
+			goto exit;
+
 		show_signal(tsk, SIGSEGV, "", desc, regs, error_code);
 		force_sig(SIGSEGV);
 		goto mark_exit;
@@ -668,7 +681,8 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
 	    kprobe_fault_handler(regs, X86_TRAP_GP))
 		goto exit;
 
-	mark_trap_entry(X86_TRAP_GP, regs);
+	if (!mark_trap_entry(X86_TRAP_GP, regs))
+		goto exit;
 
 	ret = notify_die(DIE_GPF, desc, regs, error_code, X86_TRAP_GP, SIGSEGV);
 	if (ret == NOTIFY_STOP)
@@ -740,7 +754,8 @@ DEFINE_IDTENTRY_RAW(exc_int3)
 	if (poke_int3_handler(regs))
 		return;
 
-	mark_trap_entry_raw(X86_TRAP_BP, regs);
+	if (!mark_trap_entry_raw(X86_TRAP_BP, regs))
+		return;
 
 	/*
 	 * irqentry_enter_from_user_mode() uses static_branch_{,un}likely()
@@ -1089,17 +1104,19 @@ static __always_inline void exc_debug_user(struct pt_regs *regs,
 /* IST stack entry */
 DEFINE_IDTENTRY_DEBUG(exc_debug)
 {
-	mark_trap_entry_raw(X86_TRAP_DB, regs);
-	exc_debug_kernel(regs, debug_read_clear_dr6());
-	mark_trap_exit_raw(X86_TRAP_DB, regs);
+	if (mark_trap_entry_raw(X86_TRAP_DB, regs)) {
+		exc_debug_kernel(regs, debug_read_clear_dr6());
+		mark_trap_exit_raw(X86_TRAP_DB, regs);
+	}
 }
 
 /* User entry, runs on regular task stack */
 DEFINE_IDTENTRY_DEBUG_USER(exc_debug)
 {
-	mark_trap_entry_raw(X86_TRAP_DB, regs);
-	exc_debug_user(regs, debug_read_clear_dr6());
-	mark_trap_exit_raw(X86_TRAP_DB, regs);
+	if (mark_trap_entry_raw(X86_TRAP_DB, regs)) {
+		exc_debug_user(regs, debug_read_clear_dr6());
+		mark_trap_exit_raw(X86_TRAP_DB, regs);
+	}
 }
 #else
 /* 32 bit does not have separate entry points. */
@@ -1133,7 +1150,9 @@ static void math_error(struct pt_regs *regs, int trapnr)
 		if (fixup_exception(regs, trapnr, 0, 0))
 			goto exit;
 
-		mark_trap_entry(trapnr, regs);
+		if (!mark_trap_entry(trapnr, regs))
+			goto exit;
+
 		task->thread.error_code = 0;
 		task->thread.trap_nr = trapnr;
 
@@ -1160,7 +1179,8 @@ static void math_error(struct pt_regs *regs, int trapnr)
 	if (fixup_vdso_exception(regs, trapnr, 0, 0))
 		goto exit;
 
-	mark_trap_entry(trapnr, regs);
+	if (!mark_trap_entry(trapnr, regs))
+		goto exit;
 
 	force_sig_fault(SIGFPE, si_code,
 			(void __user *)uprobe_get_trap_addr(regs));
@@ -1238,9 +1258,10 @@ DEFINE_IDTENTRY(exc_device_not_available)
 		 * to kill the task than getting stuck in a never-ending
 		 * loop of #NM faults.
 		 */
-		mark_trap_entry(X86_TRAP_NM, regs);
-		die("unexpected #NM exception", regs, 0);
-		mark_trap_exit(X86_TRAP_NM, regs);
+		if (mark_trap_entry(X86_TRAP_NM, regs)) {
+			die("unexpected #NM exception", regs, 0);
+			mark_trap_exit(X86_TRAP_NM, regs);
+		}
 	}
 }
 
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 75df0fa390cf9..3d5bcac24163b 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -891,6 +891,10 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 		return;
 
 	oob_trap_notify(X86_TRAP_PF, regs);
+	if (!running_inband()) {
+		local_irq_disable_full();
+		return;
+	}
 
 	if (likely(show_unhandled_signals))
 		show_signal_msg(regs, error_code, address, tsk);
@@ -1361,9 +1365,12 @@ void do_user_addr_fault(struct pt_regs *regs,
 	 * At this point, we would have to stop running
 	 * out-of-band. Tell the companion core about the page fault
 	 * event, so that it might switch current to in-band mode if
-	 * need be.
+	 * need be. If it does not, then we may assume that it would
+	 * also handle the fixups.
 	 */
 	oob_trap_notify(X86_TRAP_PF, regs);
+	if (!running_inband())
+		return;
 
 	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
 
-- 
Philippe.

Re: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled

[Jan Kiszka]

On 09.11.22 11:06, Philippe Gerum wrote:
> 
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> On 09.11.22 08:31, Johannes Kirchmair wrote:
>>> Hello all,
>>>
>>> I just recently found the time to test this patch in the context of our rt_signals.
>>> As a matter of fact it works quite well for us.
>>>
>>> So is there any intent/possibility to bring this upstream?
>>>
>>> I also did some similar changes for arm64. As we also use imx8 boards.
>>> If there is interest in those, I could post them.
>>
>> Yes, please share. Then we can continue with proposing it for the latest
>> dovetail port (6.1 now).
>>
> 
> There is a slightly different approach which expresses the intent of
> that notification hook Dovetail-wise:
> 
> - if the real-time core switches in-band upon notification, then the
>   in-band kernel code may assume that all regular fixups need to be
>   applied next. This is the common case for most traps.
> 
> - if the real-time core stays on the out-of-band stage upon
>   notification, then the in-band kernel code should assume that all
>   required fixups were done from that stage already, returning asap from
>   the outer trap handler.
> 
> This gives the same flexibility than by expecting the hook to return a
> continuation status, but makes sure that regular fixups cannot be
> bypassed when/if switching to in-band mode is required to handle the
> trap. Rationale: the regular in-band trap handlers know better about the
> way to deal with those events from in-band context.
> 

Sounds interesting, indeed. Would also obsolete the need to adjust users
to a new calling convention.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

RE: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled

[Johannes Kirchmair]

Hello,

> -----Original Message-----
> From: Jan Kiszka <jan.kiszka@siemens.com>
> Sent: Mittwoch, 9. November 2022 11:39
> To: Philippe Gerum <rpm@xenomai.org>
> Cc: Johannes Kirchmair <johannes.kirchmair@sigmatek.at>; Xenomai
> <xenomai@lists.linux.dev>; Richard Weinberger <richard@nod.at>
> Subject: Re: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled
> 
> CAUTION: External E-Mail !
> 
> On 09.11.22 11:06, Philippe Gerum wrote:
> >
> > Jan Kiszka <jan.kiszka@siemens.com> writes:
> >
> >> On 09.11.22 08:31, Johannes Kirchmair wrote:
> >>> Hello all,
> >>>
> >>> I just recently found the time to test this patch in the context of our
> rt_signals.
> >>> As a matter of fact it works quite well for us.
> >>>
> >>> So is there any intent/possibility to bring this upstream?
> >>>
> >>> I also did some similar changes for arm64. As we also use imx8 boards.
> >>> If there is interest in those, I could post them.
> >>
> >> Yes, please share. Then we can continue with proposing it for the latest
> >> dovetail port (6.1 now).
> >>
> >
> > There is a slightly different approach which expresses the intent of
> > that notification hook Dovetail-wise:
> >
> > - if the real-time core switches in-band upon notification, then the
> >   in-band kernel code may assume that all regular fixups need to be
> >   applied next. This is the common case for most traps.
> >
> > - if the real-time core stays on the out-of-band stage upon
> >   notification, then the in-band kernel code should assume that all
> >   required fixups were done from that stage already, returning asap from
> >   the outer trap handler.
> >
> > This gives the same flexibility than by expecting the hook to return a
> > continuation status, but makes sure that regular fixups cannot be
> > bypassed when/if switching to in-band mode is required to handle the
> > trap. Rationale: the regular in-band trap handlers know better about the
> > way to deal with those events from in-band context.
> >

I like that idea because as I was looking at the arm code, I was not sure what would happen if we are out of band but 
indicate that the kernel should handle the exception. With your idea that’s not possible.

Will have a look at my arm64 patch and try to adapt it.

Best regards 
Johannes

> 
> Sounds interesting, indeed. Would also obsolete the need to adjust users
> to a new calling convention.
> 
> Jan
> 
> --
> Siemens AG, Technology
> Competence Center Embedded Linux

Re: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled

[Philippe Gerum]

Johannes Kirchmair <johannes.kirchmair@sigmatek.at> writes:

> Hello,
>
>> -----Original Message-----
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>> Sent: Mittwoch, 9. November 2022 11:39
>> To: Philippe Gerum <rpm@xenomai.org>
>> Cc: Johannes Kirchmair <johannes.kirchmair@sigmatek.at>; Xenomai
>> <xenomai@lists.linux.dev>; Richard Weinberger <richard@nod.at>
>> Subject: Re: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled
>> 
>> CAUTION: External E-Mail !
>> 
>> On 09.11.22 11:06, Philippe Gerum wrote:
>> >
>> > Jan Kiszka <jan.kiszka@siemens.com> writes:
>> >
>> >> On 09.11.22 08:31, Johannes Kirchmair wrote:
>> >>> Hello all,
>> >>>
>> >>> I just recently found the time to test this patch in the context of our
>> rt_signals.
>> >>> As a matter of fact it works quite well for us.
>> >>>
>> >>> So is there any intent/possibility to bring this upstream?
>> >>>
>> >>> I also did some similar changes for arm64. As we also use imx8 boards.
>> >>> If there is interest in those, I could post them.
>> >>
>> >> Yes, please share. Then we can continue with proposing it for the latest
>> >> dovetail port (6.1 now).
>> >>
>> >
>> > There is a slightly different approach which expresses the intent of
>> > that notification hook Dovetail-wise:
>> >
>> > - if the real-time core switches in-band upon notification, then the
>> >   in-band kernel code may assume that all regular fixups need to be
>> >   applied next. This is the common case for most traps.
>> >
>> > - if the real-time core stays on the out-of-band stage upon
>> >   notification, then the in-band kernel code should assume that all
>> >   required fixups were done from that stage already, returning asap from
>> >   the outer trap handler.
>> >
>> > This gives the same flexibility than by expecting the hook to return a
>> > continuation status, but makes sure that regular fixups cannot be
>> > bypassed when/if switching to in-band mode is required to handle the
>> > trap. Rationale: the regular in-band trap handlers know better about the
>> > way to deal with those events from in-band context.
>> >
>
> I like that idea because as I was looking at the arm code, I was not sure what would happen if we are out of band but 
> indicate that the kernel should handle the exception. With your idea that’s not possible.
>
> Will have a look at my arm64 patch and try to adapt it.
>

Ok, so since the idea is generally agreed, I'll push the x86 and arm
support for this in a couple of days.

-- 
Philippe.

Re: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled

[Richard Weinberger]

Hi!

Sorry for exhuming this old mail thread.

Am Donnerstag, 10. November 2022, 10:00:33 CET schrieb Philippe Gerum:
> >> >>> I also did some similar changes for arm64. As we also use imx8 boards.
> >> >>> If there is interest in those, I could post them.
> >> >>
> >> >> Yes, please share. Then we can continue with proposing it for the latest
> >> >> dovetail port (6.1 now).
> >> >>
> >> >
> >> > There is a slightly different approach which expresses the intent of
> >> > that notification hook Dovetail-wise:
> >> >
> >> > - if the real-time core switches in-band upon notification, then the
> >> >   in-band kernel code may assume that all regular fixups need to be
> >> >   applied next. This is the common case for most traps.
> >> >
> >> > - if the real-time core stays on the out-of-band stage upon
> >> >   notification, then the in-band kernel code should assume that all
> >> >   required fixups were done from that stage already, returning asap from
> >> >   the outer trap handler.
> >> >
> >> > This gives the same flexibility than by expecting the hook to return a
> >> > continuation status, but makes sure that regular fixups cannot be
> >> > bypassed when/if switching to in-band mode is required to handle the
> >> > trap. Rationale: the regular in-band trap handlers know better about the
> >> > way to deal with those events from in-band context.
> >> >
> >
> > I like that idea because as I was looking at the arm code, I was not sure what would happen if we are out of band but 
> > indicate that the kernel should handle the exception. With your idea that’s not possible.
> >
> > Will have a look at my arm64 patch and try to adapt it.
> >
> 
> Ok, so since the idea is generally agreed, I'll push the x86 and arm
> support for this in a couple of days.

Currently, I'm looking into implementing Johannes's RT-Signals for ARM64,
but from what I see on the dovetail side, only x86 has gained support to declare traps handled.

At least on x86, I see that a trap is considered handled if no switch to in-band has occurred.
Did I miss something?

Thanks,
//richard

-- 
​​​​​sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT
UID/VAT Nr: ATU 66964118 | FN: 374287y

Re: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled

[Jan Kiszka]

On 12.02.24 16:04, Richard Weinberger wrote:
> Hi!
> 
> Sorry for exhuming this old mail thread.
> 
> Am Donnerstag, 10. November 2022, 10:00:33 CET schrieb Philippe Gerum:
>>>>>>> I also did some similar changes for arm64. As we also use imx8 boards.
>>>>>>> If there is interest in those, I could post them.
>>>>>>
>>>>>> Yes, please share. Then we can continue with proposing it for the latest
>>>>>> dovetail port (6.1 now).
>>>>>>
>>>>>
>>>>> There is a slightly different approach which expresses the intent of
>>>>> that notification hook Dovetail-wise:
>>>>>
>>>>> - if the real-time core switches in-band upon notification, then the
>>>>>   in-band kernel code may assume that all regular fixups need to be
>>>>>   applied next. This is the common case for most traps.
>>>>>
>>>>> - if the real-time core stays on the out-of-band stage upon
>>>>>   notification, then the in-band kernel code should assume that all
>>>>>   required fixups were done from that stage already, returning asap from
>>>>>   the outer trap handler.
>>>>>
>>>>> This gives the same flexibility than by expecting the hook to return a
>>>>> continuation status, but makes sure that regular fixups cannot be
>>>>> bypassed when/if switching to in-band mode is required to handle the
>>>>> trap. Rationale: the regular in-band trap handlers know better about the
>>>>> way to deal with those events from in-band context.
>>>>>
>>>
>>> I like that idea because as I was looking at the arm code, I was not sure what would happen if we are out of band but 
>>> indicate that the kernel should handle the exception. With your idea that’s not possible.
>>>
>>> Will have a look at my arm64 patch and try to adapt it.
>>>
>>
>> Ok, so since the idea is generally agreed, I'll push the x86 and arm
>> support for this in a couple of days.
> 
> Currently, I'm looking into implementing Johannes's RT-Signals for ARM64,
> but from what I see on the dovetail side, only x86 has gained support to declare traps handled.
> 
> At least on x86, I see that a trap is considered handled if no switch to in-band has occurred.
> Did I miss something?

Yes, I do have some first hacks here as well. ARM and ARM64 is not yet
ready for this, unfortunately.

Jan

-- 
Siemens AG, Technology
Linux Expert Center

Re: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled

[Richard Weinberger]

On Mon, Feb 12, 2024 at 4:12 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >> Ok, so since the idea is generally agreed, I'll push the x86 and arm
> >> support for this in a couple of days.
> >
> > Currently, I'm looking into implementing Johannes's RT-Signals for ARM64,
> > but from what I see on the dovetail side, only x86 has gained support to declare traps handled.
> >
> > At least on x86, I see that a trap is considered handled if no switch to in-band has occurred.
> > Did I miss something?
>
> Yes, I do have some first hacks here as well. ARM and ARM64 is not yet
> ready for this, unfortunately.

Can you please share your hacks?
What is missing? I'll happily continue your work.

-- 
Thanks,
//richard

Re: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1272 bytes --]

On 12.02.24 16:14, Richard Weinberger wrote:
> On Mon, Feb 12, 2024 at 4:12 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> Ok, so since the idea is generally agreed, I'll push the x86 and arm
>>>> support for this in a couple of days.
>>>
>>> Currently, I'm looking into implementing Johannes's RT-Signals for ARM64,
>>> but from what I see on the dovetail side, only x86 has gained support to declare traps handled.
>>>
>>> At least on x86, I see that a trap is considered handled if no switch to in-band has occurred.
>>> Did I miss something?
>>
>> Yes, I do have some first hacks here as well. ARM and ARM64 is not yet
>> ready for this, unfortunately.
> 
> Can you please share your hacks?
> What is missing? I'll happily continue your work.
> 

Just refreshed [1], and the kernel side hacks are attached (mixture of
Johannes and my WIP changes). Blocker is really the level of kernel
enabling we need/want for non-x86. Those changes can be many. Also a
problem is the information forwarding from the exception handler to the
oob callback - missing in many cases, see also the dances needed in
xnarch_setup_trap_info().

Jan

[1]
https://source.denx.de/Xenomai/xenomai/-/commits/wip/fault-signals/?ref_type=heads

-- 
Siemens AG, Technology
Linux Expert Center

[-- Attachment #2: 0001-add-dovtail-signal-setup-function.patch --]
[-- Type: text/x-patch, Size: 5120 bytes --]

From 4873c7f02d5364646ed4fca272b6336aeb3a5cde Mon Sep 17 00:00:00 2001
From: Johannes Kirchmair <johannes.kirchmair@sigmatek.at>
Date: Tue, 9 May 2023 15:11:51 +0200
Subject: [PATCH 1/3] add dovtail signal setup function

add an interface to dovetail for signal frame setup and restoring.

This is useful for implementing rt signals within Xenomai.

The interface makes it possible to use the setup use functions provided
by Linux.

This is just a proof of concept using a very naive approach using most
of the code provided by Linux to setup signals.
The patch should be discussed, especially considering if using the
Linux functions is viable in a rt context.

Also to discuss would be the interface itself,
for convenience I used the kernels struct ksignal for testing. But maybe
we should maintain our own struct, that is not that feature rich.
---
 arch/x86/include/asm/sighandling.h |  3 ++
 arch/x86/kernel/signal.c           | 71 ++++++++++++++++++++++++++++++
 arch/x86/kernel/signal_32.c        |  2 +-
 arch/x86/kernel/signal_64.c        |  2 +-
 include/linux/dovetail.h           |  4 ++
 5 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/sighandling.h b/arch/x86/include/asm/sighandling.h
index e770c4fc47f4..ed6b7fcd7250 100644
--- a/arch/x86/include/asm/sighandling.h
+++ b/arch/x86/include/asm/sighandling.h
@@ -24,4 +24,7 @@ int ia32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
 int x64_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
 int x32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
 
+bool ia32_restore_sigcontext(struct pt_regs *regs, struct sigcontext_32 __user *usc);
+bool restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *usc, unsigned long uc_flags);
+
 #endif /* _ASM_X86_SIGHANDLING_H */
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 65fe2094da59..22db1df10a9d 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -409,3 +409,74 @@ bool sigaltstack_size_valid(size_t ss_size)
 	return true;
 }
 #endif /* CONFIG_DYNAMIC_SIGFRAME */
+
+#ifdef CONFIG_DOVETAIL
+int dovetail_setup_rt_signal_frame(struct ksignal *ksig, struct pt_regs *regs)
+{
+	int ret;
+
+	pagefault_disable();
+
+	if (regs->cs == __USER_CS) {
+	        ret = x64_setup_rt_frame(ksig, regs);
+	} else if (regs->cs == __USER32_CS) {
+	        ksig->ka.sa.sa_flags |= SA_IA32_ABI;
+	        ret = ia32_setup_rt_frame(ksig, regs);
+	} else {
+		ret = -EINVAL;
+	}
+
+	pagefault_enable();
+
+	return ret;
+}
+
+static int dovetail_restore_64_rt_signal_frame(struct pt_regs *regs)
+{
+	struct rt_sigframe __user *frame;
+	unsigned long uc_flags;
+
+	frame = (struct rt_sigframe __user *)(regs->sp - sizeof(long));
+	if (!access_ok(frame, sizeof(*frame)))
+		return -EFAULT;
+	if (__get_user(uc_flags, &frame->uc.uc_flags))
+		return -EFAULT;
+
+	if (!restore_sigcontext(regs, &frame->uc.uc_mcontext, uc_flags))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int dovetail_restore_32_rt_signal_frame(struct pt_regs *regs)
+{
+	struct rt_sigframe_ia32 __user *frame;
+
+	frame = (struct rt_sigframe_ia32 __user *)(regs->sp - 4);
+	if (!access_ok(frame, sizeof(*frame)))
+	        return -EFAULT;
+
+	if (!ia32_restore_sigcontext(regs, &frame->uc.uc_mcontext))
+	        return -EFAULT;
+
+	return 0;
+}
+
+int dovetail_restore_rt_signal_frame(struct pt_regs *regs)
+{
+	int ret;
+
+	pagefault_disable();
+
+	if (regs->cs == __USER_CS)
+		ret = dovetail_restore_64_rt_signal_frame(regs);
+	else if (regs->cs == __USER32_CS)
+		ret = dovetail_restore_32_rt_signal_frame(regs);
+	else
+		ret = -EINVAL;
+
+	pagefault_enable();
+
+	return ret;
+}
+#endif
diff --git a/arch/x86/kernel/signal_32.c b/arch/x86/kernel/signal_32.c
index c12624bc82a3..a494e51e2ec7 100644
--- a/arch/x86/kernel/signal_32.c
+++ b/arch/x86/kernel/signal_32.c
@@ -74,7 +74,7 @@ static inline void reload_segments(struct sigcontext_32 *sc)
 /*
  * Do a signal return; undo the signal stack.
  */
-static bool ia32_restore_sigcontext(struct pt_regs *regs,
+bool ia32_restore_sigcontext(struct pt_regs *regs,
 				    struct sigcontext_32 __user *usc)
 {
 	struct sigcontext_32 sc;
diff --git a/arch/x86/kernel/signal_64.c b/arch/x86/kernel/signal_64.c
index 23d8aaf8d9fd..09ae2437b5f3 100644
--- a/arch/x86/kernel/signal_64.c
+++ b/arch/x86/kernel/signal_64.c
@@ -47,7 +47,7 @@ static void force_valid_ss(struct pt_regs *regs)
 		regs->ss = __USER_DS;
 }
 
-static bool restore_sigcontext(struct pt_regs *regs,
+bool restore_sigcontext(struct pt_regs *regs,
 			       struct sigcontext __user *usc,
 			       unsigned long uc_flags)
 {
diff --git a/include/linux/dovetail.h b/include/linux/dovetail.h
index 366260e3291c..c87665728038 100644
--- a/include/linux/dovetail.h
+++ b/include/linux/dovetail.h
@@ -151,6 +151,10 @@ void oob_trampoline(void);
 
 void arch_inband_task_init(struct task_struct *p);
 
+int dovetail_setup_rt_signal_frame(struct ksignal *ksig, struct pt_regs *regs);
+
+int dovetail_restore_rt_signal_frame(struct pt_regs *regs);
+
 int dovetail_start(void);
 
 void dovetail_stop(void);
-- 
2.35.3


[-- Attachment #3: 0002-arm-signal-frames.patch --]
[-- Type: text/x-patch, Size: 5598 bytes --]

From 7bfdc9830fb38adadb849b16383cb1da4b344178 Mon Sep 17 00:00:00 2001
From: Jan Kiszka <jan.kiszka@siemens.com>
Date: Tue, 17 Oct 2023 14:36:57 +0200
Subject: [PATCH 2/3] arm: signal frames

---
 arch/arm/kernel/signal.c | 39 +++++++++++++++++++++++++++++++++++++
 arch/arm/mm/alignment.c  |  3 ++-
 arch/arm/mm/fault.c      | 42 ++++++++++++++++++++++++----------------
 3 files changed, 66 insertions(+), 18 deletions(-)

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 2631632090c0..6ee172b5e3e3 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -599,6 +599,45 @@ static int do_signal(struct pt_regs *regs, int syscall)
 	return 0;
 }
 
+#ifdef CONFIG_DOVETAIL
+int dovetail_setup_rt_signal_frame(struct ksignal *ksig, struct pt_regs *regs)
+{
+	sigset_t *oldset = sigmask_to_save();
+	int ret;
+
+	pagefault_disable();
+
+	ret = setup_rt_frame(ksig, oldset, regs);
+	ret |= !valid_user_regs(regs);
+
+	pagefault_enable();
+
+	return ret;
+}
+
+int dovetail_restore_rt_signal_frame(struct pt_regs *regs)
+{
+	struct rt_sigframe __user *frame;
+	int ret = 0;
+
+	if (regs->ARM_sp & 7)
+		return -EFAULT;
+
+	frame = (struct rt_sigframe __user *)regs->ARM_sp;
+
+	pagefault_disable();
+
+	if (!access_ok(frame, sizeof (*frame)) ||
+	    restore_sigframe(regs, &frame->sig) ||
+	    restore_altstack(&frame->sig.uc.uc_stack))
+		ret = -EFAULT;
+
+	pagefault_enable();
+
+	return ret;
+}
+#endif
+
 static inline void do_retuser(void)
 {
 	unsigned int thread_flags;
diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
index b03ce1ded8f5..a5bba966d916 100644
--- a/arch/arm/mm/alignment.c
+++ b/arch/arm/mm/alignment.c
@@ -813,7 +813,8 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	if (interrupts_enabled(regs))
 		hard_local_irq_enable();
 
-	mark_trap_entry(ARM_TRAP_ALIGNMENT, regs);
+	if (!mark_cond_trap_entry(ARM_TRAP_ALIGNMENT, regs))
+		return 0;
 
 	instrptr = instruction_pointer(regs);
 
diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index fd628acecd14..c4ef796c2e92 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -39,7 +39,7 @@
  * preempted by an out-of-band activity.
  */
 static inline
-unsigned long fault_entry(int exception, struct pt_regs *regs)
+bool __fault_entry(int exception, struct pt_regs *regs, unsigned long *pflags)
 {
 	unsigned long flags;
 
@@ -47,11 +47,10 @@ unsigned long fault_entry(int exception, struct pt_regs *regs)
 
 	flags = hard_local_save_flags();
 
-	/*
-	 * The companion core must demote the current context to
-	 * in-band stage if running oob on entry.
-	 */
-	mark_trap_entry(exception, regs);
+	if (!mark_cond_trap_entry(exception, regs)) {
+		hard_local_irq_restore(flags);
+		return false;
+	}
 
 	if (raw_irqs_disabled_flags(flags)) {
 		stall_inband();
@@ -60,9 +59,13 @@ unsigned long fault_entry(int exception, struct pt_regs *regs)
 
 	hard_local_irq_enable();
 
-	return flags;
+	*pflags = flags;
+	return true;
 }
 
+#define fault_entry(__exception, __regs, __flags) \
+	__fault_entry(__exception, __regs, &__flags)
+
 static inline
 void fault_exit(int exception, struct pt_regs *regs,
 		unsigned long flags)
@@ -80,7 +83,7 @@ void fault_exit(int exception, struct pt_regs *regs,
 
 #else	/* !CONFIG_IRQ_PIPELINE */
 
-#define fault_entry(__exception, __regs)  ({ 0; })
+#define fault_entry(__exception, __regs, __flags)  (true)
 #define fault_exit(__exception, __regs, __flags)  \
 	do { (void)(__flags); } while (0)
 
@@ -283,9 +286,10 @@ void do_bad_area(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	 * have no context to handle this fault with.
 	 */
 	  if (user_mode(regs)) {
-		irqflags = fault_entry(ARM_TRAP_ACCESS, regs);
-		__do_user_fault(addr, fsr, SIGSEGV, SEGV_MAPERR, regs);
-		fault_exit(ARM_TRAP_ACCESS, regs, irqflags);
+		if (fault_entry(ARM_TRAP_ACCESS, regs, irqflags)) {
+			__do_user_fault(addr, fsr, SIGSEGV, SEGV_MAPERR, regs);
+			fault_exit(ARM_TRAP_ACCESS, regs, irqflags);
+		}
 	  } else
 		/*
 		 * irq_pipeline: kernel faults are either quickly
@@ -323,7 +327,8 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	unsigned long vm_flags = VM_ACCESS_FLAGS;
 	unsigned long irqflags;
 
-	irqflags = fault_entry(ARM_TRAP_ACCESS, regs);
+	if (!fault_entry(ARM_TRAP_ACCESS, regs, irqflags))
+		return 0;
 
 	if (kprobe_page_fault(regs, fsr))
 		goto out;
@@ -561,9 +566,10 @@ do_sect_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 {
 	unsigned long irqflags;
 
-	irqflags = fault_entry(ARM_TRAP_SECTION, regs);
-	do_bad_area(addr, fsr, regs);
-	fault_exit(ARM_TRAP_SECTION, regs, irqflags);
+	if (fault_entry(ARM_TRAP_SECTION, regs, irqflags)) {
+		do_bad_area(addr, fsr, regs);
+		fault_exit(ARM_TRAP_SECTION, regs, irqflags);
+	}
 	return 0;
 }
 #endif /* CONFIG_ARM_LPAE */
@@ -616,7 +622,8 @@ do_DataAbort(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	if (!inf->fn(addr, fsr & ~FSR_LNX_PF, regs))
 		return;
 
-	irqflags = fault_entry(ARM_TRAP_DABT, regs);
+	if (!fault_entry(ARM_TRAP_DABT, regs, irqflags))
+		return;
 	pr_alert("8<--- cut here ---\n");
 	pr_alert("Unhandled fault: %s (0x%03x) at 0x%08lx\n",
 		inf->name, fsr, addr);
@@ -649,7 +656,8 @@ do_PrefetchAbort(unsigned long addr, unsigned int ifsr, struct pt_regs *regs)
 	if (!inf->fn(addr, ifsr | FSR_LNX_PF, regs))
 		return;
 
-	irqflags = fault_entry(ARM_TRAP_PABT, regs);
+	if (!fault_entry(ARM_TRAP_PABT, regs, irqflags))
+		return;
 	pr_alert("Unhandled prefetch abort: %s (0x%03x) at 0x%08lx\n",
 		inf->name, ifsr, addr);
 
-- 
2.35.3


[-- Attachment #4: 0003-arm64-rt-signals.patch --]
[-- Type: text/x-patch, Size: 6040 bytes --]

From 9cb4078aac4e42e6d54d77c9c97140dc7055474e Mon Sep 17 00:00:00 2001
From: Jan Kiszka <jan.kiszka@siemens.com>
Date: Thu, 11 Jan 2024 11:02:34 +0100
Subject: [PATCH 3/3] arm64 rt signals

---
 arch/arm64/kernel/signal.c | 39 ++++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/traps.c  | 22 ++++++++++-----------
 arch/arm64/mm/fault.c      | 20 +++++++++++++------
 3 files changed, 63 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index d2ae16680752..1dd55aa1833f 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -1279,6 +1279,45 @@ static void do_signal(struct pt_regs *regs)
 	restore_saved_sigmask();
 }
 
+#ifdef CONFIG_DOVETAIL
+int dovetail_setup_rt_signal_frame(struct ksignal *ksig, struct pt_regs *regs)
+{
+	sigset_t *oldset = sigmask_to_save();
+	int ret;
+
+	pagefault_disable();
+
+	ret = setup_rt_frame(ksig->sig, ksig, oldset, regs);
+	ret |= !valid_user_regs(&regs->user_regs, current);
+
+	pagefault_enable();
+
+	return ret;
+}
+
+int dovetail_restore_rt_signal_frame(struct pt_regs *regs)
+{
+	struct rt_sigframe __user *frame;
+	int ret = 0;
+
+	if (regs->sp & 15)
+		return -EFAULT;
+
+	frame = (struct rt_sigframe __user *)regs->sp;
+
+	pagefault_disable();
+
+	if (!access_ok(frame, sizeof (*frame)) ||
+	    restore_sigframe(regs, frame) ||
+	    restore_altstack(&frame->uc.uc_stack))
+		ret = -EFAULT;
+
+	pagefault_enable();
+
+	return ret;
+}
+#endif
+
 static inline void do_retuser(void)
 {
 	unsigned long thread_flags;
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 6492162e931c..4c14ee5396d1 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -456,14 +456,8 @@ void do_el0_undef(struct pt_regs *regs, unsigned long esr)
 {
 	u32 insn;
 
-	mark_trap_entry(ARM64_TRAP_UNDI, regs);
-
-	/*
-	 * If the companion core did not switched us to in-band
-	 * context, we may assume that it has handled the trap.
-	 */
-	if (running_oob())
-		goto out_exit;
+	if (!mark_cond_trap_entry(ARM64_TRAP_UNDI, regs))
+		return;
 
 	/* check for AArch32 breakpoint instructions */
 	if (!aarch32_break_handler(regs))
@@ -501,7 +495,8 @@ void do_el1_undef(struct pt_regs *regs, unsigned long esr)
 
 void do_el0_bti(struct pt_regs *regs)
 {
-	mark_trap_entry(ARM64_TRAP_BTI, regs);
+	if (!mark_cond_trap_entry(ARM64_TRAP_BTI, regs))
+		return;
 	force_signal_inject(SIGILL, ILL_ILLOPC, regs->pc, 0);
 	mark_trap_exit(ARM64_TRAP_BTI, regs);
 }
@@ -590,7 +585,8 @@ static void user_cache_maint_handler(unsigned long esr, struct pt_regs *regs)
 	}
 
 	if (ret) {
-		mark_trap_entry(ARM64_TRAP_ACCESS, regs);
+		if (!mark_cond_trap_entry(ARM64_TRAP_ACCESS, regs))
+			return;
 		arm64_notify_segfault(tagged_address);
 		mark_trap_exit(ARM64_TRAP_ACCESS, regs);
 	} else {
@@ -641,7 +637,8 @@ static void mrs_handler(unsigned long esr, struct pt_regs *regs)
 	sysreg = esr_sys64_to_sysreg(esr);
 
 	if (do_emulate_mrs(regs, sysreg, rt) != 0) {
-		mark_trap_entry(ARM64_TRAP_ACCESS, regs);
+		if (!mark_cond_trap_entry(ARM64_TRAP_ACCESS, regs))
+			return;
 		force_signal_inject(SIGILL, ILL_ILLOPC, regs->pc, 0);
 		mark_trap_exit(ARM64_TRAP_ACCESS, regs);
 	}
@@ -884,7 +881,8 @@ void bad_el0_sync(struct pt_regs *regs, int reason, unsigned long esr)
 {
 	unsigned long pc = instruction_pointer(regs);
 
-	mark_trap_entry(ARM64_TRAP_ACCESS, regs);
+	if (!mark_cond_trap_entry(ARM64_TRAP_ACCESS, regs))
+		return;
 	current->thread.fault_address = 0;
 	current->thread.fault_code = esr;
 
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 543600b9cf0b..5870a1d0ff8e 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -493,7 +493,8 @@ static void do_bad_area(unsigned long far, unsigned long esr,
 	if (user_mode(regs)) {
 		const struct fault_info *inf = esr_to_fault_info(esr);
 
-		mark_trap_entry(ARM64_TRAP_ACCESS, regs);
+		if (!mark_cond_trap_entry(ARM64_TRAP_ACCESS, regs))
+			return;
 		set_thread_esr(addr, esr);
 		arm64_force_sig_fault(inf->sig, inf->code, far, inf->name);
 		mark_trap_exit(ARM64_TRAP_ACCESS, regs);
@@ -549,7 +550,8 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
 	if (kprobe_page_fault(regs, esr))
 		return 0;
 
-	mark_trap_entry(ARM64_TRAP_ACCESS, regs);
+	if (!mark_cond_trap_entry(ARM64_TRAP_ACCESS, regs))
+		return 0;
 
 	/*
 	 * If we're in an interrupt or have no user context, we must not take
@@ -624,6 +626,7 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
 	if (fault_signal_pending(fault, regs)) {
 		if (!user_mode(regs))
 			goto no_context;
+		mark_trap_exit(ARM64_TRAP_ACCESS, regs);
 		return 0;
 	}
 lock_mmap:
@@ -645,8 +648,10 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
 	}
 
 	/* The fault is fully completed (including releasing mmap lock) */
-	if (fault & VM_FAULT_COMPLETED)
+	if (fault & VM_FAULT_COMPLETED) {
+		mark_trap_exit(ARM64_TRAP_ACCESS, regs);
 		return 0;
+	}
 
 	if (fault & VM_FAULT_RETRY) {
 		mm_flags |= FAULT_FLAG_TRIED;
@@ -747,7 +752,8 @@ static int do_sea(unsigned long far, unsigned long esr, struct pt_regs *regs)
 	const struct fault_info *inf;
 	unsigned long siaddr;
 
-	mark_trap_entry(ARM64_TRAP_SEA, regs);
+	if (!mark_cond_trap_entry(ARM64_TRAP_SEA, regs))
+		return 0;
 
 	inf = esr_to_fault_info(esr);
 
@@ -864,7 +870,8 @@ void do_mem_abort(unsigned long far, unsigned long esr, struct pt_regs *regs)
 	if (!inf->fn(far, esr, regs))
 		return;
 
-	mark_trap_entry(ARM64_TRAP_ACCESS, regs);
+	if (!mark_cond_trap_entry(ARM64_TRAP_ACCESS, regs))
+		return;
 
 	if (!user_mode(regs))
 		die_kernel_fault(inf->name, addr, esr, regs);
@@ -882,7 +889,8 @@ NOKPROBE_SYMBOL(do_mem_abort);
 
 void do_sp_pc_abort(unsigned long addr, unsigned long esr, struct pt_regs *regs)
 {
-	mark_trap_entry(ARM64_TRAP_ALIGN, regs);
+	if (!mark_cond_trap_entry(ARM64_TRAP_ALIGN, regs))
+		return;
 
 	arm64_notify_die("SP/PC alignment exception", regs, SIGBUS, BUS_ADRALN,
 			 addr, esr);
-- 
2.35.3

Re: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled

[Richard Weinberger]

Jan,

On Mon, Feb 12, 2024 at 5:12 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >> Yes, I do have some first hacks here as well. ARM and ARM64 is not yet
> >> ready for this, unfortunately.
> >
> > Can you please share your hacks?
> > What is missing? I'll happily continue your work.
> >
>
> Just refreshed [1], and the kernel side hacks are attached (mixture of
> Johannes and my WIP changes). Blocker is really the level of kernel
> enabling we need/want for non-x86. Those changes can be many. Also a
> problem is the information forwarding from the exception handler to the
> oob callback - missing in many cases, see also the dances needed in
> xnarch_setup_trap_info().

In commit 7e13c6997 ("rt-signal: kernel refactorings") I see the
following change:

diff --git a/kernel/cobalt/dovetail/kevents.c b/kernel/cobalt/dovetail/kevents.c
index 5394c85d0..275a9f726 100644
--- a/kernel/cobalt/dovetail/kevents.c
+++ b/kernel/cobalt/dovetail/kevents.c
@@ -56,7 +56,8 @@ void handle_oob_trap_entry(unsigned int trapnr,
struct pt_regs *regs)
               xnsched_run();
       }

-       if (xnthread_handle_rt_signals(trapnr, regs) == 0)
+       if (xnthread_test_state(thread, XNUSER) &&
+           xnthread_handle_rt_signals(trapnr, regs))
               return;

       /*

I guess the idea behind is making sure we handle only faults from user mode.
But isn't XNUSER also present if a thread is executing a cobalt syscall?
So, I think instead of just testing for XNUSER we need to check using
user_mode(regs) too.

What do you think?

-- 
Thanks,
//richard

Re: [RFC][PATCH] x86: dovetail: Permit to declare a trap handled

[Jan Kiszka]

On 28.03.24 08:28, Richard Weinberger wrote:
> Jan,
> 
> On Mon, Feb 12, 2024 at 5:12 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> Yes, I do have some first hacks here as well. ARM and ARM64 is not yet
>>>> ready for this, unfortunately.
>>>
>>> Can you please share your hacks?
>>> What is missing? I'll happily continue your work.
>>>
>>
>> Just refreshed [1], and the kernel side hacks are attached (mixture of
>> Johannes and my WIP changes). Blocker is really the level of kernel
>> enabling we need/want for non-x86. Those changes can be many. Also a
>> problem is the information forwarding from the exception handler to the
>> oob callback - missing in many cases, see also the dances needed in
>> xnarch_setup_trap_info().
> 
> In commit 7e13c6997 ("rt-signal: kernel refactorings") I see the
> following change:
> 
> diff --git a/kernel/cobalt/dovetail/kevents.c b/kernel/cobalt/dovetail/kevents.c
> index 5394c85d0..275a9f726 100644
> --- a/kernel/cobalt/dovetail/kevents.c
> +++ b/kernel/cobalt/dovetail/kevents.c
> @@ -56,7 +56,8 @@ void handle_oob_trap_entry(unsigned int trapnr,
> struct pt_regs *regs)
>                xnsched_run();
>        }
> 
> -       if (xnthread_handle_rt_signals(trapnr, regs) == 0)
> +       if (xnthread_test_state(thread, XNUSER) &&
> +           xnthread_handle_rt_signals(trapnr, regs))
>                return;
> 
>        /*
> 
> I guess the idea behind is making sure we handle only faults from user mode.
> But isn't XNUSER also present if a thread is executing a cobalt syscall?
> So, I think instead of just testing for XNUSER we need to check using
> user_mode(regs) too.
> 
> What do you think?
> 

I think the idea was to exclude kernel threads, but I'm not 100% sure
anymore. I agree that testing for user_mode should have that effect as
well and exclude kernel oopses - if we don't filter them elsewhere already.

Jan

-- 
Siemens AG, Technology
Linux Expert Center