Re: port-xen/58561 (panic: kernel diagnostic assertion, "x86_read_psl() == 0" failed: file, "/home/netbsd/10/src/sys/arch/x86/x86/pmap.c", line 3581)

To: bouyer%netbsd.org@localhost, gnats-admin%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost, campbell+netbsd%mumble.net@localhost
Subject: Re: port-xen/58561 (panic: kernel diagnostic assertion, "x86_read_psl() == 0" failed: file, "/home/netbsd/10/src/sys/arch/x86/x86/pmap.c", line 3581)
From: "Manuel Bouyer via gnats" <gnats-admin%NetBSD.org@localhost>
Date: Thu, 15 Jan 2026 10:30:02 +0000 (UTC)

The following reply was made to PR port-xen/58561; it has been noted by GNATS.

From: Manuel Bouyer <bouyer%antioche.eu.org@localhost>
To: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Cc: Konrad Schroder <perseant%hhhh.org@localhost>, gnats-bugs%NetBSD.org@localhost,
        port-xen-maintainer%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost,
        gnats-admin%netbsd.org@localhost, campbell+netbsd%mumble.net@localhost, cherry%NetBSD.org@localhost
Subject: Re: port-xen/58561 (panic: kernel diagnostic assertion,
 "x86_read_psl() == 0" failed: file,
 "/home/netbsd/10/src/sys/arch/x86/x86/pmap.c", line 3581)
Date: Thu, 15 Jan 2026 11:25:33 +0100

 On Wed, Jan 14, 2026 at 11:00:10PM +0000, Taylor R Campbell wrote:
 > > Date: Tue, 13 Jan 2026 15:50:10 +0100
 > > From: Manuel Bouyer <bouyer%antioche.eu.org@localhost>
 > > 
 > > Here's the updated patch
 > 
 > Thanks for working on this!  Some small issues:
 
 thanks for the review,
 
 > 
 > > --- amd64/amd64/cpufunc.S	6 Sep 2025 02:53:22 -0000	1.70
 > > +++ amd64/amd64/cpufunc.S	13 Jan 2026 14:48:25 -0000
 > > @@ -184,7 +184,7 @@ ENTRY(wbinvd)
 > >  END(wbinvd)
 > >  
 > >  ENTRY(setusergs)
 > > -	CLI(ax)
 > > +	CLI2(ax, rdi)
 > >  	swapgs
 > >  	movw	%di, %gs
 > >  	swapgs
 > 
 > I think this can't be right.  %rdi is obviously not a free scratch
 > register: it's the first argument register.  But also, this code isn't
 > even used in Xen builds, is it?  Xen has its own setusergs in
 > xenfunc.c, doesn't it?
 
 Right, I changed it to use %rsi. Also as you noted this is not used for
 Xen at this time (which is why I didn't notice I guess), but I think it's
 better to be correct and use the appropriate macro here (even if it always
 end up being a single cli instruction at this time)
 
 > 
 > https://nxr.netbsd.org/xref/src/sys/arch/xen/x86/xenfunc.c?r=1.29#270
 > 
 > > --- amd64/amd64/spl.S	1 Mar 2023 08:38:50 -0000	1.49
 > > +++ amd64/amd64/spl.S	13 Jan 2026 14:48:25 -0000
 > > ...
 > > @@ -355,7 +356,7 @@ IDTVEC(doreti)
 > >  1:
 > >  	movl    %ebx,%eax
 > >  	movq	CPUVAR(IUNMASK)(,%rax,8),%rax
 > > -	CLI(si)
 > > +	CLI2(si, r14)
 > >  	andq	CPUVAR(IPENDING),%rax
 > >  	jz	2f
 > >  	bsrq	%rax,%rax		/* slow, but not worth optimizing */
 > > @@ -378,7 +379,7 @@ IDTVEC(doreti)
 > >  	movq	%rsp,%rdi
 > >  	KMSAN_INIT_ARG(8)
 > >  	call	_C_LABEL(trap)
 > > -	CLI(si)
 > > +	CLI2(si, rdi)
 > >  	jmp	.Ldoreti_checkast
 > >  3:
 > >  	CHECK_DEFERRED_SWITCH
 > > @@ -389,6 +390,6 @@ IDTVEC(doreti)
 > >  9:
 > >  	STI(si)
 > >  	call	_C_LABEL(do_pmap_load)
 > > -	CLI(si)
 > > +	CLI2(si, rdi)
 > >  	jmp	.Ldoreti_checkast		/* recheck ASTs */
 > >  IDTVEC_END(doreti)
 > 
 > Not immediately clear these registers are free here, can we
 > double-check and document in a comment exactly what registers are
 > involved on entry and exit to doreti?
 
 I added "rsi, rdi and r14 - free for local use". rsi was already used here, 
 r14 is also used by CHECK_ASTPENDING()/CLEAR_ASTPENDING() and rdi for the
 call to trap().
 
 > 
 > > --- amd64/include/frameasm.h	30 Jul 2022 14:11:00 -0000	1.55
 > > +++ amd64/include/frameasm.h	13 Jan 2026 14:48:25 -0000
 > > ...
 > > + * ci_ilevel is nit 0, or
 > 
 > Nit: not `nit', `not'!
 > 
 > > +	cmpl $0, L_NOPREEMPT(%r ## temp_reg);			\
 > > +	jne 199f;						\
 > > ...
 > > +199:	movq CPUVAR(VCPU),%r ## temp_reg ;			\
 > >  	movb $1,EVTCHN_UPCALL_MASK(%r ## temp_reg);
 > 
 > Would be nice if we could make this work so the static branch
 > prediction (forward jump not taken) takes the right path here, but I
 > guess it's a pain because this is really doing
 > 
 > 	KASSERT(l->l_nopreempt != 0 || curcpu()->ci_ilevel != 0 || ...)
 
 yes, that's the problem. Also it's DIAGNOSTIC code so I prefer to
 keep it easy to read.
 
 > 
 > > + 	movq $panicstr,%r ## temp_reg ;			 	\
 > 
 > Should this use _C_LABEL for consistency?
 
 Fixed
 
 > 
 > > +	cmpq $0, 0(%r ## temp_reg);				\
 > 
 > I always get confused about C symbols in MOV assembly directives.
 > Does temp_reg at this point hold a (possibly null) pointer to the
 > first character of the panic string, or a (possibly null) pointer to
 > the _pointer to_ first character of the panic string?
 
 temp_reg holds the address of the pointer (that is, &panicstr), and this
 cannot be NULL. So the cmpq checks the pointer itself.
 I know this code works because I went there multiple times while debugging :)
 
 > 
 > Shouldn't this be one of the two options?
 > 
 > 	leaq	$panicstr,%rtmp
 > 	cmpq	$0, 0(%rtmp)
 
 I guess this would check *panicstr, that is, the 8 first character of the
 panicstr string
 > 
 > or
 > 
 > 	movq	$panicstr,%rtmp
 > 	cmpq	$0,%rtmp
 
 that would check &panicstr, which cannot be NULL
 
 > 
 > > +	movq _C_LABEL(cli_panic), %rdi;				\
 > > +	call _C_LABEL(panic);					\
 > 
 > Have you tested provoking this path to make sure it leads to a
 > sensible-looking panic?
 
 Ho yes, multiple times :)
 
 > 
 > > +/* CLI() with preemtion disabled */
 > > +#define CLI2(temp_reg, temp_reg2) \
 > > + 	movq CPUVAR(CURLWP),% ## temp_reg2 ;			\
 > > +	incl L_NOPREEMPT(% ## temp_reg2);			\
 > > + 	movq CPUVAR(VCPU),%r ## temp_reg ;			\
 > > +	movb $1,EVTCHN_UPCALL_MASK(%r ## temp_reg);		\
 > > +	decl L_NOPREEMPT(% ## temp_reg2);
 > 
 > Or, as an alternative to all the above issues...  What if we just make
 > CLI work using a single temporary register?  Requires one extra load
 > to reuse the register but I bet modern x86 CPUs will do enough fancy
 > register renaming and caching internally that it makes essentially no
 > difference except perhaps for a few bytes of code size:
 
 Yes, you're probably right it wouldn't make much difference.
 But we always have an extra free register, except in syscall entries.
 I choose to save an instruction most of the time.
 
 > 
 > 	movq	CPUVAR(curlwp),%r ## temp_reg;
 > 	incl	L_NOPREEMPT(%r ## temp_reg);
 > 	movq	CPUVAR(VCPU),%r ## temp_reg2;
 > 	movb	$1,EVTCHN_UPCALL_MASK(%r ## temp_reg);
 > 	movq	CPUVAR(curlwp),%r ## temp_reg;
 > 	decl	L_NOPREEMPT(%r ## temp_reg);
 > 
 > > --- xen/x86/xen_intr.c	25 Feb 2023 00:32:13 -0000	1.31
 > > +++ xen/x86/xen_intr.c	13 Jan 2026 14:48:26 -0000
 > > @@ -540,3 +547,9 @@ __strong_alias(cpu_intr_init, xen_cpu_in
 > >  __strong_alias(intr_allocate_io_intrsource, xen_intr_allocate_io_intrsource);
 > >  __strong_alias(intr_free_io_intrsource, xen_intr_free_io_intrsource);
 > >  #endif /* XENPV */
 > > +
 > > +
 > > +#if defined(DIAGNOSTIC) && defined(XENPV)
 > > +const char *cli_panic = "cli with preemtion";
 > > +const char *sti_panic = "sti with interrupts enabled";
 > > +#endif
 > 
 > const char cli_panic[] = "cli with preemtion";
 > const char sti_panic[] = "sti with interrupts enabled";
 > 
 > Don't need a mutable pointer to the immutable string in memory -- just
 > a symbol for the string is enough.
 
 Yes, but how do we do this in C ?
 
 -- 
 Manuel Bouyer <bouyer%antioche.eu.org@localhost>
      NetBSD: 26 ans d'experience feront toujours la difference
 --

Prev by Date: Re: port-xen/58561 (panic: kernel diagnostic assertion, "x86_read_psl() == 0" failed: file, "/home/netbsd/10/src/sys/arch/x86/x86/pmap.c", line 3581)
Next by Date: Re: kern/59922: iwm0 - Intel Dual Band Wireless AC 8265 (rev. 0x78) - stopped working on NetBSD 11.0_BETA
Previous by Thread: Re: port-xen/58561 (panic: kernel diagnostic assertion, "x86_read_psl() == 0" failed: file, "/home/netbsd/10/src/sys/arch/x86/x86/pmap.c", line 3581)
Next by Thread: PR/59883 CVS commit: src/lib/libedit
Indexes:

Home | Main Index | Thread Index | Old Index