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)

["Taylor R Campbell via gnats" <gnats-admin%NetBSD.org@localhost>]

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

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: Manuel Bouyer <bouyer%antioche.eu.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: Wed, 14 Jan 2026 23:00:10 +0000

 > Date: Tue, 13 Jan 2026 15:50:10 +0100
 > From: Manuel Bouyer <bouyer%antioche.eu.org@localhost>
 >=20
 > Here's the updated patch
 
 Thanks for working on this!  Some small issues:
 
 > --- 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)
 > =20
 >  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?
 
 https://nxr.netbsd.org/xref/src/sys/arch/xen/x86/xenfunc.c?r=3D1.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?
 
 > --- 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 !=3D 0 || curcpu()->ci_ilevel !=3D 0 || ...)
 
 > + 	movq $panicstr,%r ## temp_reg ;			 	\
 
 Should this use _C_LABEL for consistency?
 
 > +	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?
 
 Shouldn't this be one of the two options?
 
 	leaq	$panicstr,%rtmp
 	cmpq	$0, 0(%rtmp)
 
 or
 
 	movq	$panicstr,%rtmp
 	cmpq	$0,%rtmp
 
 > +	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?
 
 > +/* 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:
 
 	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_intrsou=
 rce);
 >  __strong_alias(intr_free_io_intrsource, xen_intr_free_io_intrsource);
 >  #endif /* XENPV */
 > +
 > +
 > +#if defined(DIAGNOSTIC) && defined(XENPV)
 > +const char *cli_panic =3D "cli with preemtion";
 > +const char *sti_panic =3D "sti with interrupts enabled";
 > +#endif
 
 const char cli_panic[] =3D "cli with preemtion";
 const char sti_panic[] =3D "sti with interrupts enabled";
 
 Don't need a mutable pointer to the immutable string in memory -- just
 a symbol for the string is enough.