Re: [PATCH 8/9] s390/percpu: Provide arch_this_cpu_write() implementation

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Critical logic inversion in `percpu_exit()` migration fixup corrupts percpu pointers.
- [Low] Unused output constraint `[old__]` in `arch_this_cpu_write` inline assembly.
--

--- Patch [8]: [PATCH 8/9] s390/percpu: Provide arch_this_cpu_write() implementation ---
commit 2a32543383d043fdfafc28f075daae72847d527d
Author: Heiko Carstens <[email protected]>
s390/percpu: Provide arch_this_cpu_write() implementation

This commit provides an s390 specific implementation of
arch_this_cpu_write() instead of using the generic variant. It aims to
reduce the overhead of raw_local_irq_save/restore pairs and reduces the
text size of the kernel image.

> diff --git a/arch/s390/include/asm/percpu.h b/arch/s390/include/asm/percpu.h
> index 35e48d193d993..7a411b75bfa62 100644
> --- a/arch/s390/include/asm/percpu.h
> +++ b/arch/s390/include/asm/percpu.h
[ ... ]
> @@ -271,6 +271,38 @@
>  #define this_cpu_read_4(pcp) arch_this_cpu_read(pcp, "l")
>  #define this_cpu_read_8(pcp) arch_this_cpu_read(pcp, "lg")
>  
> +#define arch_this_cpu_write(pcp, val, op)				\
> +{									\
> +	unsigned long lc_pcpr, lc_pcpo;					\
> +	typedef typeof(pcp) pcp_op_T__;					\
> +	pcp_op_T__ val__ = (val);					\
> +	pcp_op_T__ old__, *ptr__;					\
> +									\
> +	lc_pcpr = offsetof(struct lowcore, percpu_register);		\
> +	lc_pcpo = offsetof(struct lowcore, percpu_offset);		\
> +	ptr__ = &(pcp);							\
> +	asm_inline volatile(						\
> +		MVIY_PERCPU("%[disppcpr]", "%[dispaltpcpr]", "%[ptr__]")\
> +		AG_ALT("%[disppcpo]", "%[dispaltpcpo]", "%[ptr__]")	\
> +		op "    %[val__],0(%[ptr__])\n"				\
> +		MVIY_ALT("%[disppcpr]", "%[dispaltpcpr]", "%%r0")	\
> +		: [old__] "=&d" (old__),				\

[Severity: Low]
Does this output constraint unnecessarily tie up a general-purpose register?
The compiler is forced to allocate a register for old__, but the inline
assembly string never references %[old__].

> +		  [ptr__] "+&a" (ptr__), "=m" (*ptr__),			\
> +		  "=m" (((struct lowcore *)0)->percpu_register)		\
> +		: [val__] "d" (val__),					\
[ ... ]

Looking at how these new percpu sections interact with migration fixups in
arch/s390/include/asm/entry-percpu.h, there might be a regression with the
instruction check:

arch/s390/include/asm/entry-percpu.h:percpu_exit() {
	...
	/*
	 * Within a percpu code section and process has been migrated to
	 * a different CPU. Check if the percpu base register needs to be
	 * updated. This is the case if the PSW does not point to the ADD
	 * instruction within the section
	 * - AG %rx,percpu_offset_in_lowcore(%r0,%r0)
	 * which adds the percpu offset to the percpu base register.
	 */
	if ((*(u16 *)psw_bits(regs->psw).ia & 0xff0f) != 0xe300)
		return;
	...
}

[Severity: Critical]
Is the condition in this check inverted? The fixup skips updating the offset
if the instruction is not the AG instruction (0xe300).

If a task executing the new arch_this_cpu_write() is interrupted and migrated
to another CPU, it seems the correct logic would be to return if the
instruction is the AG instruction (meaning it hasn't executed yet and no
fixup is needed), and proceed with the fixup if it is not the AG instruction
(meaning the AG instruction has already executed and the offset needs
updating).

Could this inverted logic result in skipping the fixup when interrupted after
the AG instruction, or applying it incorrectly when interrupted before the AG
instruction, leading to corrupted pointers or updates to the wrong CPU's
percpu variable?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8