From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id AE092331EBD for ; Tue, 28 Jul 2026 15:54:05 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785254048; cv=none; b=nku//b7+DrjPKxw5t5/mtYDAwznibLX43P0ZfdVXUn6Mo1g0smFuD4Syn2E4hdThsAXOQ+uSMWNUzPNt/x9DjcXWqiqor4/cewIbJksfCuK2yAHoxSHw7uSUh4zn47ZnWpsRevnIzpBfm2D3u757AflEicLFwc/nv0CRKgZK00E= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785254048; c=relaxed/simple; bh=q6wYkZC/S7niIdrsVebuDR2WJR/5r2MKu2teOHVHvWE=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=mBTGu8xFddrmLgTVkgQTsYe8hZ/sCEmi40eA9/QmQw1OfyTlif0sq+l0TzDdAGT/n2GHAUJGx36JYrruO3if9kZHMZ2OlIvhqPuI6GMaPstdG/5dOqY4VHp9m/nQJt5yTSmQgW0tC5wZKh42Dn6LdfnWPb/FW/AN5EXvQLycjkA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b=pXnKqSBw; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b="pXnKqSBw" Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id DED67169E; Tue, 28 Jul 2026 08:54:00 -0700 (PDT) Received: from J2N7QTR9R3 (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 6A7F93F66F; Tue, 28 Jul 2026 08:54:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=arm.com; s=foss; t=1785254045; bh=q6wYkZC/S7niIdrsVebuDR2WJR/5r2MKu2teOHVHvWE=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=pXnKqSBwnzNmM53UQAu+WKc5j3k171ZXSRYAa1EwXXYSCEFi97MgHkzWa51TkYWz8 Lrtyy8QQFnYbYPQ31laTvJm2HavsBO8H2mZ2QjVcvBG+1vhMoDHy4CMDUt2uauDMrf QLvsDb+fzP0CcirNwQGf9V8zQOGejJaxmkOQimH0= Date: Tue, 28 Jul 2026 16:53:52 +0100 From: Mark Rutland To: Peter Zijlstra Cc: linux-arm-kernel@lists.infradead.org, ada.coupriediaz@arm.com, ardb@kernel.org, catalin.marinas@arm.com, hca@linux.ibm.com, linux-kernel@vger.kernel.org, maz@kernel.org, ruanjinjie@huawei.com, vladimir.murzin@arm.com, will@kernel.org, yang@os.amperecomputing.com Subject: Re: [RFC PATCH 06/13] arm64: percpu: Add infrastructure for preemptible this_cpu_*() ops Message-ID: References: <20260728123859.2911495-1-mark.rutland@arm.com> <20260728123859.2911495-7-mark.rutland@arm.com> <20260728141958.GV751831@noisy.programming.kicks-ass.net> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260728141958.GV751831@noisy.programming.kicks-ass.net> 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: > > > > | : > > | 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 _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.