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

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:

quoted

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?

quoted

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

quoted

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

quoted

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


[...]

quoted

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

quoted

+/*
+ * 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.