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)

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

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
--