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

[Mark Rutland <mark.rutland@arm.com>]

On Tue, Jul 28, 2026 at 04:19:58PM +0200, Peter Zijlstra wrote:
> 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]

Sure, I'll make that: 

|        mov     x4, #0xc80 // __VAL_PCPU_GPRS(x0, x4, x3)

... to match the code we use below.

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

Ah. :/

We use them pretty commonly throughout arch/arm64/ (e.g. for all the
sysreg stuff), so the core arm64 folk are all familiar with them.

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

I was being lazy here relative to what we do for asm-extable.h. How
about I match the pattern there, and have:

| #define PCPU_GPR_PCP_SHIFT              0
| #define PCPU_GPR_PCP                    GENMASK(4, 0)
| #define PCPU_GPR_OFF_SHIFT              5
| #define PCPU_GPR_OFF                    GENMASK(9, 5)
| #define PCPU_GPR_ADDR_SHIFT             10
| #define PCPU_GPR_ADDR                   GENMASK(14, 10)
| 
| #define __VAL_PCPU_GPRS(pcp, off, addr)                                                 \
|         "("                                                                             \
|                 "(.L__gpr_num_" pcp  " << " __stringify(PCPU_GPR_PCP_SHIFT) ") | "      \
|                 "(.L__gpr_num_" off  " << " __stringify(PCPU_GPR_OFF_SHIFT) ") | "      \
|                 "(.L__gpr_num_" addr " << " __stringify(PCPU_GPR_ADDR_SHIFT) ")"       \
|         ")"

... if that's any better?

Regardless of how we define these, I really want to keep the values in one
place (e.g. one header).


[...]

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

Do you mean for __VAL_PCPU_GPRS, or ____PCPU_GPRS_BEGIN() ?

I can happily add comments for either (and the corresponding ENDs).

> > +/*
> > + * 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() ?

I wrote it this way to keep the field definitions in one place, and to
match what we do elsewhere, e.g.

| static bool ex_handler_uaccess_err_zero(const struct exception_table_entry *ex,
|                                         struct pt_regs *regs)
| {
|         int reg_err = FIELD_GET(EX_DATA_REG_ERR, ex->data);
|         int reg_zero = FIELD_GET(EX_DATA_REG_ZERO, ex->data);
| 
|         pt_regs_write_reg(regs, reg_err, -EFAULT);
|         pt_regs_write_reg(regs, reg_zero, 0); 
| 
|         regs->pc = get_ex_fixup(ex);
|         return true;
| }

My preference would be as-is (or with the proposal above), but I'll
defer to Will and Catalin.

If we do open-code the shifts, I'd strongly prefer that we have
<foo>_SHIFT constants to keep those in sync, e.g.

	reg_pcp	 = (gprs >> PCPU_GRP_PCP_SHIFT)  & 0x1f;
	reg_off	 = (gprs >> PCPU_GRP_OFF_SHIFT)  & 0x1f;
	reg_addr = (gprs >> PCPU_GRP_ADDR_SHIFT) & 0x1f;

Mark.