Re: [RFC PATCH 06/13] arm64: percpu: Add infrastructure for preemptible this_cpu_*() ops

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Jul 28, 2026 at 01:38:52PM +0100, Mark Rutland wrote:

> With the scheme added in this patch, this can be compiled as:
> 
> | <outline_this_cpu_add_u64>:
> |        mrs     x2, sp_el0
> |        mov     x4, #0xc80

Perhaps add a comment to that like:

  |        mov     x4, #0xc80 # {x0, x4, x3}

to make the literal less magical?

> |        strh    w4, [x2, #20]

Anyway, interrupt can hit here, at which point it will:

  x4 := this_cpu_offset
  x3 := x0 + x4

> |        mrs     x4, tpidr_el1
> |        add     x3, x0, x4

Which it then recomputes, and so is harmless.

> | 1:     ldxr    x6, [x3]
> |        add     x6, x6, x1
> |        stxr    w5, x6, [x3]

IIRC any interrupt in between LL/SC will cause the stxr to fail anyway,
irrespective of the fixup, so goto 1b we go.

> |        cbnz    w5, 1b

If we get here and interrupts happens, we recompute pointlessly, no harm
done.

Anyway, per this sequence there is no point in ever doing the fixup. So
perhaps give a better example?

> |        strh    wzr, [x2, #20]
> |        ret
> 
> TODO: Save/restore the PCPU GPRs in __sdei_asm_handler(). This will
> require some mechanical rework to the __sdei_asm_handler() assembly.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>

> @@ -51,6 +53,47 @@ static inline unsigned long __kern_my_cpu_offset(void)
>  	return off;
>  }
>  
> +#define PCPU_GPR_PCP			GENMASK(4, 0)
> +#define PCPU_GPR_OFF			GENMASK(9, 5)
> +#define PCPU_GPR_ADDR			GENMASK(14, 10)

I hate those macros, I can never remember what they all do. Now, esp.
since you then not use them here:

> +#define __VAL_PCPU_GPRS(pcp, off, addr)				\
> +	"("							\
> +		"(.L__gpr_num_" pcp  " << 0) | "		\
> +		"(.L__gpr_num_" off  " << 5) | "		\
> +		"(.L__gpr_num_" addr " << 10)"			\
> +	")"

... continue comment at irqentry_exit_pcpu_adjust().

> +#define ____PCPU_GPRS_BEGIN(gprs, pcp, off, addr)			\
> +	__DEFINE_ASM_GPR_NUMS						\
> +	__DEFINE_ASM_GPR_ALIASES					\
> +	"	mov w" off ", #" __VAL_PCPU_GPRS(pcp, off, addr) "\n"	\
> +	"	strh	w" off ", " gprs "\n"				\
> +	__KERN_ASM_CPU_OFFSET(off) "\n"

Can this macro also generate a readable comment for those few of us
building the .i file ?

> +/*
> + * Where the context being returned to had an active percpu GPR critical
> + * section, ensure that the offset and address GPRs are updated to match the
> + * current CPU.
> + *
> + * For simplicity we always update the GPRs when a critical section is active
> + * and preemption was *possible*, regardless of whether preemption actually
> + * occurred. Where preemption did not occur, the updates are redundant but not
> + * harmful.
> + */
> +static __always_inline void irqentry_exit_pcpu_adjust(struct pt_regs *regs)
> +{
> +	int reg_pcp, reg_off, reg_addr;
> +	unsigned long pcp, off, addr;
> +	u16 gprs = regs->pcpu_gprs;
> +
> +	/*
> +	 * Zero means no active PCPU GPRs. As the PCPU GPRs must be distinct,
> +	 * a PCPU critical section cannot possibly use {x0,x0,x0}.
> +	 */
> +	if (likely(!gprs))
> +		return;
> +
> +	reg_pcp  = FIELD_GET(PCPU_GPR_PCP,  gprs);
> +	reg_off  = FIELD_GET(PCPU_GPR_OFF,  gprs);
> +	reg_addr = FIELD_GET(PCPU_GPR_ADDR, gprs);

Would it not be more readable to write this like:

	reg_pcp  = (grps >>  0) & 0x1f;
	reg_off  = (grps >>  5) & 0x1f;
	reg_addr = (grps >> 10) & 0x1f;

To better match __VAL_PCPU_GRPS() ?

> +	pcp = pt_regs_read_reg(regs, reg_pcp);
> +
> +	off = __kern_my_cpu_offset();
> +	pt_regs_write_reg(regs, reg_off, off);
> +
> +	addr = pcp + off;
> +	pt_regs_write_reg(regs, reg_addr, addr);
> +}