Re: git: 914a53570750 - main - amd64: move efirt trap checks into the helper

On Sat, Mar 14, 2026 at 12:15:33PM +0100, A FreeBSD User wrote:
> Am Tage des Herren Sat, 14 Mar 2026 10:13:15 +0100
> "Herbert J. Skuhra" <herbert@gojira.at> schrieb:
> 
> > On Fri, 13 Mar 2026 23:51:06 +0100, Konstantin Belousov wrote:
> > > 
> > > The branch main has been updated by kib:
> > > 
> > > URL: https://cgit.FreeBSD.org/src/commit/?id=914a53570750ce5a104a5870403d7669656fddc3
> > > 
> > > commit 914a53570750ce5a104a5870403d7669656fddc3
> > > Author:     Konstantin Belousov <kib@FreeBSD.org>
> > > AuthorDate: 2026-03-11 11:53:52 +0000
> > > Commit:     Konstantin Belousov <kib@FreeBSD.org>
> > > CommitDate: 2026-03-13 22:47:13 +0000
> > > 
> > >     amd64: move efirt trap checks into the helper
> > >     
> > >     Reviewed by:    imp, jhb
> > >     Sponsored by:   The FreeBSD Foundation
> > >     MFC after:      1 week
> > >     Differential revision:  https://reviews.freebsd.org/D55808
> > > ---
> > >  sys/amd64/amd64/trap.c | 55 ++++++++++++++++++++++++--------------------------
> > >  1 file changed, 26 insertions(+), 29 deletions(-)  
> > 
> > This is causing a kernel panic here.
> > 
> 
> me too.

My polite answer is that the messages do not provide useful information.
I got a useful trace from Peter Holm, and I think I know what is going
on there. My current patch is below, I will commit it after Peter'
confirmation.

If you have a different issue, you should report it in a way that allows
to diagnose the problem.

From 7097dd1ec28472594a6fbb2f5bd8b6f88459f0e9 Mon Sep 17 00:00:00 2001
From: Konstantin Belousov <kib@FreeBSD.org>
Date: Sat, 14 Mar 2026 13:40:07 +0200
Subject: [PATCH] amd64: do reset %rip after page fault if pcb_onfault is set

for any kernel page fault, and not only for EFIRT case.

Reported by:	pho
Fixes:	914a53570750ce5a104a5870403d7669656fddc3
Sponsored by:	The FreeBSD Foundation
MFC after:	1 week
---
 sys/amd64/amd64/trap.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/sys/amd64/amd64/trap.c b/sys/amd64/amd64/trap.c
index 4bf56226d076..3a9323936d2d 100644
--- a/sys/amd64/amd64/trap.c
+++ b/sys/amd64/amd64/trap.c
@@ -219,15 +219,19 @@ trap_uprintf_signal(struct thread *td, struct trapframe *frame, register_t addr,
 }
 
 static bool
-trap_check_efirt(struct thread *td, struct trapframe *frame)
+trap_check_pcb_onfault(struct thread *td, struct trapframe *frame)
 {
-	/*
-	 * Most likely, EFI RT faulted.  This check prevents
-	 * kdb from handling breakpoints set on the BIOS text,
-	 * if such option is ever needed.
-	 */
-	if ((td->td_pflags & TDP_EFIRT) != 0 &&
-	    curpcb->pcb_onfault != NULL) {
+	bool res = false;
+
+	if (curpcb->pcb_onfault == NULL)
+		return (res);
+
+	if (__predict_false((td->td_pflags & TDP_EFIRT) != 0)) {
+		/*
+		 * Most likely, EFI RT faulted.  This check prevents
+		 * kdb from handling breakpoints set on the BIOS text,
+		 * if such option is ever needed.
+		 */
 		u_long cnt = atomic_fetchadd_long(&cnt_efirt_faults, 1);
 
 		if ((print_efirt_faults == 1 && cnt == 0) ||
@@ -236,10 +240,13 @@ trap_check_efirt(struct thread *td, struct trapframe *frame)
 			    traptype_to_msg(frame->tf_trapno));
 			trap_diag(frame, 0);
 		}
-		frame->tf_rip = (long)curpcb->pcb_onfault;
-		return (true);
+		res = true;
+	} else if (frame->tf_trapno == T_PAGEFLT) {
+		res = true;
 	}
-	return (false);
+	if (res)
+		frame->tf_rip = (register_t)curpcb->pcb_onfault;
+	return (res);
 }
 
 static void
@@ -494,7 +501,7 @@ trap(struct trapframe *frame)
 		KASSERT(cold || td->td_ucred != NULL,
 		    ("kernel trap doesn't have ucred"));
 
-		if (type != T_PAGEFLT && trap_check_efirt(td, frame))
+		if (type != T_PAGEFLT && trap_check_pcb_onfault(td, frame))
 			return;
 
 		switch (type) {
@@ -904,7 +911,7 @@ trap_pfault(struct trapframe *frame, bool usermode, int *signo, int *ucode)
 		return (1);
 after_vmfault:
 	if (td->td_intr_nesting_level == 0 &&
-	    trap_check_efirt(td, frame))
+	    trap_check_pcb_onfault(td, frame))
 		return (0);
 	trap_fatal(frame, eva);
 	return (-1);
-- 
2.53.0