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 <riastradh%NetBSD.org@localhost>]

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

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

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?

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

> + 	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_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.