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

[Philippe Gerum <rpm@xenomai.org>]

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.