[PATCH] irqchip/gic-v5: Fix GIC CDEOI instruction encoding

[PATCH] irqchip/gic-v5: Fix GIC CDEOI instruction encoding

[Lorenzo Pieralisi]

The GIC CDEOI system instruction requires the Rt field to be set to 0b11111
otherwise the instruction behaviour becomes CONSTRAINED UNPREDICTABLE.

Currenly, its usage is encoded as a system register write, with an immediate 0
value:

write_sysreg_s(0, GICV5_OP_GIC_CDEOI)

Whilst this might turn out to work if the compiler encodes the immediate 0
value into an XZR register in the MSR operation (ie that corresponds to
Rt == 0b11111), it is not reliable and actually it does not work when the
kernel is compiled with LLVM that does not yet understand the asm inline
constraints enabling direct XZR usage for system instruction encodings
(in write_sysreg_s()).

Rename the __SYS_BARRIER_INSN macro and use it to generate the required
GIC CDEOI encoding instead of relying on write_sysreg_s() with an immediate
0 value, fixing the issue.

Fixes: 7ec80fb3f025 ("irqchip/gic-v5: Add GICv5 PPI support")
Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: stable@vger.kernel.org
Cc: Sascha Bischoff <sascha.bischoff@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/sysreg.h | 20 +++++++++++++++-----
 drivers/irqchip/irq-gic-v5.c    |  4 ++--
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 6455db1b54fd..6cf8c46ddde5 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -113,14 +113,14 @@
 /* Register-based PAN access, for save/restore purposes */
 #define SYS_PSTATE_PAN			sys_reg(3, 0, 4, 2, 3)
 
-#define __SYS_BARRIER_INSN(op0, op1, CRn, CRm, op2, Rt)			\
+#define __SYS_INSN(op0, op1, CRn, CRm, op2, Rt)				\
 	__emit_inst(0xd5000000					|	\
 		    sys_insn((op0), (op1), (CRn), (CRm), (op2))	|	\
 		    ((Rt) & 0x1f))
 
-#define SB_BARRIER_INSN			__SYS_BARRIER_INSN(0, 3, 3, 0, 7, 31)
-#define GSB_SYS_BARRIER_INSN		__SYS_BARRIER_INSN(1, 0, 12, 0, 0, 31)
-#define GSB_ACK_BARRIER_INSN		__SYS_BARRIER_INSN(1, 0, 12, 0, 1, 31)
+#define SB_BARRIER_INSN			__SYS_INSN(0, 3, 3, 0, 7, 31)
+#define GSB_SYS_BARRIER_INSN		__SYS_INSN(1, 0, 12, 0, 0, 31)
+#define GSB_ACK_BARRIER_INSN		__SYS_INSN(1, 0, 12, 0, 1, 31)
 
 /* Data cache zero operations */
 #define SYS_DC_ISW			sys_insn(1, 0, 7, 6, 2)
@@ -1075,7 +1075,6 @@
 #define GICV5_OP_GIC_CDDIS		sys_insn(1, 0, 12, 1, 0)
 #define GICV5_OP_GIC_CDHM		sys_insn(1, 0, 12, 2, 1)
 #define GICV5_OP_GIC_CDEN		sys_insn(1, 0, 12, 1, 1)
-#define GICV5_OP_GIC_CDEOI		sys_insn(1, 0, 12, 1, 7)
 #define GICV5_OP_GIC_CDPEND		sys_insn(1, 0, 12, 1, 4)
 #define GICV5_OP_GIC_CDPRI		sys_insn(1, 0, 12, 1, 2)
 #define GICV5_OP_GIC_CDRCFG		sys_insn(1, 0, 12, 1, 5)
@@ -1129,6 +1128,17 @@
 #define gicr_insn(insn)			read_sysreg_s(GICV5_OP_GICR_##insn)
 #define gic_insn(v, insn)		write_sysreg_s(v, GICV5_OP_GIC_##insn)
 
+/*
+ * GIC CDEOI encoding requires Rt to be 0b11111.
+ * gic_insn() with an immediate value of 0 cannot be used to encode it
+ * because some compilers do not follow asm inline constraints in
+ * write_sysreg_s() to turn an immediate 0 value into an XZR as
+ * MSR source register.
+ * Use __SYS_INSN to specify its precise encoding explicitly.
+ */
+#define GICV5_CDEOI_INSN		__SYS_INSN(1, 0, 12, 1, 7, 31)
+#define gic_cdeoi()			asm volatile(GICV5_CDEOI_INSN)
+
 #define ARM64_FEATURE_FIELD_BITS	4
 
 #ifdef __ASSEMBLY__
diff --git a/drivers/irqchip/irq-gic-v5.c b/drivers/irqchip/irq-gic-v5.c
index 41ef286c4d78..b607c943c47d 100644
--- a/drivers/irqchip/irq-gic-v5.c
+++ b/drivers/irqchip/irq-gic-v5.c
@@ -218,14 +218,14 @@ static void gicv5_hwirq_eoi(u32 hwirq_id, u8 hwirq_type)
 
 	gic_insn(cddi, CDDI);
 
-	gic_insn(0, CDEOI);
+	gic_cdeoi();
 }
 
 static void gicv5_ppi_irq_eoi(struct irq_data *d)
 {
 	/* Skip deactivate for forwarded PPI interrupts */
 	if (irqd_is_forwarded_to_vcpu(d)) {
-		gic_insn(0, CDEOI);
+		gic_cdeoi();
 		return;
 	}
 
-- 
2.48.0

Re: [PATCH] irqchip/gic-v5: Fix GIC CDEOI instruction encoding

[Catalin Marinas]

On Mon, Oct 06, 2025 at 12:07:58PM +0200, Lorenzo Pieralisi wrote:
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 6455db1b54fd..6cf8c46ddde5 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -113,14 +113,14 @@
>  /* Register-based PAN access, for save/restore purposes */
>  #define SYS_PSTATE_PAN			sys_reg(3, 0, 4, 2, 3)
>  
> -#define __SYS_BARRIER_INSN(op0, op1, CRn, CRm, op2, Rt)			\
> +#define __SYS_INSN(op0, op1, CRn, CRm, op2, Rt)				\
>  	__emit_inst(0xd5000000					|	\
>  		    sys_insn((op0), (op1), (CRn), (CRm), (op2))	|	\
>  		    ((Rt) & 0x1f))
>  
> -#define SB_BARRIER_INSN			__SYS_BARRIER_INSN(0, 3, 3, 0, 7, 31)
> -#define GSB_SYS_BARRIER_INSN		__SYS_BARRIER_INSN(1, 0, 12, 0, 0, 31)
> -#define GSB_ACK_BARRIER_INSN		__SYS_BARRIER_INSN(1, 0, 12, 0, 1, 31)
> +#define SB_BARRIER_INSN			__SYS_INSN(0, 3, 3, 0, 7, 31)
> +#define GSB_SYS_BARRIER_INSN		__SYS_INSN(1, 0, 12, 0, 0, 31)
> +#define GSB_ACK_BARRIER_INSN		__SYS_INSN(1, 0, 12, 0, 1, 31)
>  
>  /* Data cache zero operations */
>  #define SYS_DC_ISW			sys_insn(1, 0, 7, 6, 2)
> @@ -1075,7 +1075,6 @@
>  #define GICV5_OP_GIC_CDDIS		sys_insn(1, 0, 12, 1, 0)
>  #define GICV5_OP_GIC_CDHM		sys_insn(1, 0, 12, 2, 1)
>  #define GICV5_OP_GIC_CDEN		sys_insn(1, 0, 12, 1, 1)
> -#define GICV5_OP_GIC_CDEOI		sys_insn(1, 0, 12, 1, 7)
>  #define GICV5_OP_GIC_CDPEND		sys_insn(1, 0, 12, 1, 4)
>  #define GICV5_OP_GIC_CDPRI		sys_insn(1, 0, 12, 1, 2)
>  #define GICV5_OP_GIC_CDRCFG		sys_insn(1, 0, 12, 1, 5)
> @@ -1129,6 +1128,17 @@
>  #define gicr_insn(insn)			read_sysreg_s(GICV5_OP_GICR_##insn)
>  #define gic_insn(v, insn)		write_sysreg_s(v, GICV5_OP_GIC_##insn)
>  
> +/*
> + * GIC CDEOI encoding requires Rt to be 0b11111.
> + * gic_insn() with an immediate value of 0 cannot be used to encode it
> + * because some compilers do not follow asm inline constraints in
> + * write_sysreg_s() to turn an immediate 0 value into an XZR as
> + * MSR source register.
> + * Use __SYS_INSN to specify its precise encoding explicitly.
> + */
> +#define GICV5_CDEOI_INSN		__SYS_INSN(1, 0, 12, 1, 7, 31)
> +#define gic_cdeoi()			asm volatile(GICV5_CDEOI_INSN)

Would something like this work? Completely untested (and build still
going):

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 6604fd6f33f4..7aa962f7bdd6 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -1234,7 +1234,10 @@
 #define write_sysreg_s(v, r) do {					\
 	u64 __val = (u64)(v);						\
 	u32 __maybe_unused __check_r = (u32)(r);			\
-	asm volatile(__msr_s(r, "%x0") : : "rZ" (__val));		\
+	if (__builtin_constant_p(__val) && __val == 0)			\
+		asm volatile(__msr_s(r, "xzr"));			\
+	else								\
+		asm volatile(__msr_s(r, "%x0") : : "r" (__val));	\
 } while (0)
 
 /*

-- 
Catalin

Re: [PATCH] irqchip/gic-v5: Fix GIC CDEOI instruction encoding

[Lorenzo Pieralisi]

On Mon, Oct 06, 2025 at 02:30:04PM +0100, Catalin Marinas wrote:
> On Mon, Oct 06, 2025 at 12:07:58PM +0200, Lorenzo Pieralisi wrote:
> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > index 6455db1b54fd..6cf8c46ddde5 100644
> > --- a/arch/arm64/include/asm/sysreg.h
> > +++ b/arch/arm64/include/asm/sysreg.h
> > @@ -113,14 +113,14 @@
> >  /* Register-based PAN access, for save/restore purposes */
> >  #define SYS_PSTATE_PAN			sys_reg(3, 0, 4, 2, 3)
> >  
> > -#define __SYS_BARRIER_INSN(op0, op1, CRn, CRm, op2, Rt)			\
> > +#define __SYS_INSN(op0, op1, CRn, CRm, op2, Rt)				\
> >  	__emit_inst(0xd5000000					|	\
> >  		    sys_insn((op0), (op1), (CRn), (CRm), (op2))	|	\
> >  		    ((Rt) & 0x1f))
> >  
> > -#define SB_BARRIER_INSN			__SYS_BARRIER_INSN(0, 3, 3, 0, 7, 31)
> > -#define GSB_SYS_BARRIER_INSN		__SYS_BARRIER_INSN(1, 0, 12, 0, 0, 31)
> > -#define GSB_ACK_BARRIER_INSN		__SYS_BARRIER_INSN(1, 0, 12, 0, 1, 31)
> > +#define SB_BARRIER_INSN			__SYS_INSN(0, 3, 3, 0, 7, 31)
> > +#define GSB_SYS_BARRIER_INSN		__SYS_INSN(1, 0, 12, 0, 0, 31)
> > +#define GSB_ACK_BARRIER_INSN		__SYS_INSN(1, 0, 12, 0, 1, 31)
> >  
> >  /* Data cache zero operations */
> >  #define SYS_DC_ISW			sys_insn(1, 0, 7, 6, 2)
> > @@ -1075,7 +1075,6 @@
> >  #define GICV5_OP_GIC_CDDIS		sys_insn(1, 0, 12, 1, 0)
> >  #define GICV5_OP_GIC_CDHM		sys_insn(1, 0, 12, 2, 1)
> >  #define GICV5_OP_GIC_CDEN		sys_insn(1, 0, 12, 1, 1)
> > -#define GICV5_OP_GIC_CDEOI		sys_insn(1, 0, 12, 1, 7)
> >  #define GICV5_OP_GIC_CDPEND		sys_insn(1, 0, 12, 1, 4)
> >  #define GICV5_OP_GIC_CDPRI		sys_insn(1, 0, 12, 1, 2)
> >  #define GICV5_OP_GIC_CDRCFG		sys_insn(1, 0, 12, 1, 5)
> > @@ -1129,6 +1128,17 @@
> >  #define gicr_insn(insn)			read_sysreg_s(GICV5_OP_GICR_##insn)
> >  #define gic_insn(v, insn)		write_sysreg_s(v, GICV5_OP_GIC_##insn)
> >  
> > +/*
> > + * GIC CDEOI encoding requires Rt to be 0b11111.
> > + * gic_insn() with an immediate value of 0 cannot be used to encode it
> > + * because some compilers do not follow asm inline constraints in
> > + * write_sysreg_s() to turn an immediate 0 value into an XZR as
> > + * MSR source register.
> > + * Use __SYS_INSN to specify its precise encoding explicitly.
> > + */
> > +#define GICV5_CDEOI_INSN		__SYS_INSN(1, 0, 12, 1, 7, 31)
> > +#define gic_cdeoi()			asm volatile(GICV5_CDEOI_INSN)
> 
> Would something like this work? Completely untested (and build still
> going):

I tested the GIC CDEOI code generated with GCC/LLVM and it works.

My only remark there is that even as the code in mainline stands with
GCC, it is not very clear that we rely on implicit XZR generation to
make sure the instruction encoding generated is correct - it looks
like a bit of a stretch to reuse a sysreg write with immediate value == 0
to generate a system instruction write with Rt == 0b11111, it works
but it is a bit opaque or at least not straighforward to grok.

Obviously the patch below improves LLVM code generation too in the process.

I don't know what's best - I admit I am on the fence on this one.

Thanks !
Lorenzo

> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 6604fd6f33f4..7aa962f7bdd6 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -1234,7 +1234,10 @@
>  #define write_sysreg_s(v, r) do {					\
>  	u64 __val = (u64)(v);						\
>  	u32 __maybe_unused __check_r = (u32)(r);			\
> -	asm volatile(__msr_s(r, "%x0") : : "rZ" (__val));		\
> +	if (__builtin_constant_p(__val) && __val == 0)			\
> +		asm volatile(__msr_s(r, "xzr"));			\
> +	else								\
> +		asm volatile(__msr_s(r, "%x0") : : "r" (__val));	\
>  } while (0)
>  
>  /*
> 
> -- 
> Catalin

Re: [PATCH] irqchip/gic-v5: Fix GIC CDEOI instruction encoding

[Catalin Marinas]

On Mon, Oct 06, 2025 at 05:00:56PM +0200, Lorenzo Pieralisi wrote:
> On Mon, Oct 06, 2025 at 02:30:04PM +0100, Catalin Marinas wrote:
> > On Mon, Oct 06, 2025 at 12:07:58PM +0200, Lorenzo Pieralisi wrote:
> > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > > index 6455db1b54fd..6cf8c46ddde5 100644
> > > --- a/arch/arm64/include/asm/sysreg.h
> > > +++ b/arch/arm64/include/asm/sysreg.h
> > > @@ -113,14 +113,14 @@
> > >  /* Register-based PAN access, for save/restore purposes */
> > >  #define SYS_PSTATE_PAN			sys_reg(3, 0, 4, 2, 3)
> > >  
> > > -#define __SYS_BARRIER_INSN(op0, op1, CRn, CRm, op2, Rt)			\
> > > +#define __SYS_INSN(op0, op1, CRn, CRm, op2, Rt)				\
> > >  	__emit_inst(0xd5000000					|	\
> > >  		    sys_insn((op0), (op1), (CRn), (CRm), (op2))	|	\
> > >  		    ((Rt) & 0x1f))
> > >  
> > > -#define SB_BARRIER_INSN			__SYS_BARRIER_INSN(0, 3, 3, 0, 7, 31)
> > > -#define GSB_SYS_BARRIER_INSN		__SYS_BARRIER_INSN(1, 0, 12, 0, 0, 31)
> > > -#define GSB_ACK_BARRIER_INSN		__SYS_BARRIER_INSN(1, 0, 12, 0, 1, 31)
> > > +#define SB_BARRIER_INSN			__SYS_INSN(0, 3, 3, 0, 7, 31)
> > > +#define GSB_SYS_BARRIER_INSN		__SYS_INSN(1, 0, 12, 0, 0, 31)
> > > +#define GSB_ACK_BARRIER_INSN		__SYS_INSN(1, 0, 12, 0, 1, 31)
> > >  
> > >  /* Data cache zero operations */
> > >  #define SYS_DC_ISW			sys_insn(1, 0, 7, 6, 2)
> > > @@ -1075,7 +1075,6 @@
> > >  #define GICV5_OP_GIC_CDDIS		sys_insn(1, 0, 12, 1, 0)
> > >  #define GICV5_OP_GIC_CDHM		sys_insn(1, 0, 12, 2, 1)
> > >  #define GICV5_OP_GIC_CDEN		sys_insn(1, 0, 12, 1, 1)
> > > -#define GICV5_OP_GIC_CDEOI		sys_insn(1, 0, 12, 1, 7)
> > >  #define GICV5_OP_GIC_CDPEND		sys_insn(1, 0, 12, 1, 4)
> > >  #define GICV5_OP_GIC_CDPRI		sys_insn(1, 0, 12, 1, 2)
> > >  #define GICV5_OP_GIC_CDRCFG		sys_insn(1, 0, 12, 1, 5)
> > > @@ -1129,6 +1128,17 @@
> > >  #define gicr_insn(insn)			read_sysreg_s(GICV5_OP_GICR_##insn)
> > >  #define gic_insn(v, insn)		write_sysreg_s(v, GICV5_OP_GIC_##insn)
> > >  
> > > +/*
> > > + * GIC CDEOI encoding requires Rt to be 0b11111.
> > > + * gic_insn() with an immediate value of 0 cannot be used to encode it
> > > + * because some compilers do not follow asm inline constraints in
> > > + * write_sysreg_s() to turn an immediate 0 value into an XZR as
> > > + * MSR source register.
> > > + * Use __SYS_INSN to specify its precise encoding explicitly.
> > > + */
> > > +#define GICV5_CDEOI_INSN		__SYS_INSN(1, 0, 12, 1, 7, 31)
> > > +#define gic_cdeoi()			asm volatile(GICV5_CDEOI_INSN)
> > 
> > Would something like this work? Completely untested (and build still
> > going):
> 
> I tested the GIC CDEOI code generated with GCC/LLVM and it works.
> 
> My only remark there is that even as the code in mainline stands with
> GCC, it is not very clear that we rely on implicit XZR generation to
> make sure the instruction encoding generated is correct - it looks
> like a bit of a stretch to reuse a sysreg write with immediate value == 0
> to generate a system instruction write with Rt == 0b11111, it works
> but it is a bit opaque or at least not straighforward to grok.
> 
> Obviously the patch below improves LLVM code generation too in the process.
> 
> I don't know what's best - I admit I am on the fence on this one.

My concern is other cases where we may rely on this, so we might as well
go with a generic approach than fixing each case individually. If that's
the only case, I'll leave it to you and Marc do decide whichever you
prefer.

I assume we don't need this for write_sysreg(), those are accesses to
named registers gas already knows about, not used to generate new
instructions.

-- 
Catalin

Re: [PATCH] irqchip/gic-v5: Fix GIC CDEOI instruction encoding

[Lorenzo Pieralisi]

On Mon, Oct 06, 2025 at 04:24:18PM +0100, Catalin Marinas wrote:
> On Mon, Oct 06, 2025 at 05:00:56PM +0200, Lorenzo Pieralisi wrote:
> > On Mon, Oct 06, 2025 at 02:30:04PM +0100, Catalin Marinas wrote:
> > > On Mon, Oct 06, 2025 at 12:07:58PM +0200, Lorenzo Pieralisi wrote:
> > > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > > > index 6455db1b54fd..6cf8c46ddde5 100644
> > > > --- a/arch/arm64/include/asm/sysreg.h
> > > > +++ b/arch/arm64/include/asm/sysreg.h
> > > > @@ -113,14 +113,14 @@
> > > >  /* Register-based PAN access, for save/restore purposes */
> > > >  #define SYS_PSTATE_PAN			sys_reg(3, 0, 4, 2, 3)
> > > >  
> > > > -#define __SYS_BARRIER_INSN(op0, op1, CRn, CRm, op2, Rt)			\
> > > > +#define __SYS_INSN(op0, op1, CRn, CRm, op2, Rt)				\
> > > >  	__emit_inst(0xd5000000					|	\
> > > >  		    sys_insn((op0), (op1), (CRn), (CRm), (op2))	|	\
> > > >  		    ((Rt) & 0x1f))
> > > >  
> > > > -#define SB_BARRIER_INSN			__SYS_BARRIER_INSN(0, 3, 3, 0, 7, 31)
> > > > -#define GSB_SYS_BARRIER_INSN		__SYS_BARRIER_INSN(1, 0, 12, 0, 0, 31)
> > > > -#define GSB_ACK_BARRIER_INSN		__SYS_BARRIER_INSN(1, 0, 12, 0, 1, 31)
> > > > +#define SB_BARRIER_INSN			__SYS_INSN(0, 3, 3, 0, 7, 31)
> > > > +#define GSB_SYS_BARRIER_INSN		__SYS_INSN(1, 0, 12, 0, 0, 31)
> > > > +#define GSB_ACK_BARRIER_INSN		__SYS_INSN(1, 0, 12, 0, 1, 31)
> > > >  
> > > >  /* Data cache zero operations */
> > > >  #define SYS_DC_ISW			sys_insn(1, 0, 7, 6, 2)
> > > > @@ -1075,7 +1075,6 @@
> > > >  #define GICV5_OP_GIC_CDDIS		sys_insn(1, 0, 12, 1, 0)
> > > >  #define GICV5_OP_GIC_CDHM		sys_insn(1, 0, 12, 2, 1)
> > > >  #define GICV5_OP_GIC_CDEN		sys_insn(1, 0, 12, 1, 1)
> > > > -#define GICV5_OP_GIC_CDEOI		sys_insn(1, 0, 12, 1, 7)
> > > >  #define GICV5_OP_GIC_CDPEND		sys_insn(1, 0, 12, 1, 4)
> > > >  #define GICV5_OP_GIC_CDPRI		sys_insn(1, 0, 12, 1, 2)
> > > >  #define GICV5_OP_GIC_CDRCFG		sys_insn(1, 0, 12, 1, 5)
> > > > @@ -1129,6 +1128,17 @@
> > > >  #define gicr_insn(insn)			read_sysreg_s(GICV5_OP_GICR_##insn)
> > > >  #define gic_insn(v, insn)		write_sysreg_s(v, GICV5_OP_GIC_##insn)
> > > >  
> > > > +/*
> > > > + * GIC CDEOI encoding requires Rt to be 0b11111.
> > > > + * gic_insn() with an immediate value of 0 cannot be used to encode it
> > > > + * because some compilers do not follow asm inline constraints in
> > > > + * write_sysreg_s() to turn an immediate 0 value into an XZR as
> > > > + * MSR source register.
> > > > + * Use __SYS_INSN to specify its precise encoding explicitly.
> > > > + */
> > > > +#define GICV5_CDEOI_INSN		__SYS_INSN(1, 0, 12, 1, 7, 31)
> > > > +#define gic_cdeoi()			asm volatile(GICV5_CDEOI_INSN)
> > > 
> > > Would something like this work? Completely untested (and build still
> > > going):
> > 
> > I tested the GIC CDEOI code generated with GCC/LLVM and it works.
> > 
> > My only remark there is that even as the code in mainline stands with
> > GCC, it is not very clear that we rely on implicit XZR generation to
> > make sure the instruction encoding generated is correct - it looks
> > like a bit of a stretch to reuse a sysreg write with immediate value == 0
> > to generate a system instruction write with Rt == 0b11111, it works
> > but it is a bit opaque or at least not straighforward to grok.
> > 
> > Obviously the patch below improves LLVM code generation too in the process.
> > 
> > I don't know what's best - I admit I am on the fence on this one.
> 
> My concern is other cases where we may rely on this, so we might as well
> go with a generic approach than fixing each case individually. If that's
> the only case, I'll leave it to you and Marc do decide whichever you
> prefer.

I will take your patch - added comments and rewrote the log for v2, with
your Suggested-by (did not give you authorship let me know if that's OK
please).

One thing to mention, I added a Fixes: tag that goes back to the initial
GICv5 commit, I don't know whether it is fixing more than that, it does
not look like by a quick grep through kernel code but I am not sure.

Thanks,
Lorenzo

Re: [PATCH] irqchip/gic-v5: Fix GIC CDEOI instruction encoding

[Catalin Marinas]

On Tue, Oct 07, 2025 at 10:58:12AM +0200, Lorenzo Pieralisi wrote:
> On Mon, Oct 06, 2025 at 04:24:18PM +0100, Catalin Marinas wrote:
> > On Mon, Oct 06, 2025 at 05:00:56PM +0200, Lorenzo Pieralisi wrote:
> > > My only remark there is that even as the code in mainline stands with
> > > GCC, it is not very clear that we rely on implicit XZR generation to
> > > make sure the instruction encoding generated is correct - it looks
> > > like a bit of a stretch to reuse a sysreg write with immediate value == 0
> > > to generate a system instruction write with Rt == 0b11111, it works
> > > but it is a bit opaque or at least not straighforward to grok.
> > > 
> > > Obviously the patch below improves LLVM code generation too in the process.
> > > 
> > > I don't know what's best - I admit I am on the fence on this one.
> > 
> > My concern is other cases where we may rely on this, so we might as well
> > go with a generic approach than fixing each case individually. If that's
> > the only case, I'll leave it to you and Marc do decide whichever you
> > prefer.
> 
> I will take your patch - added comments and rewrote the log for v2, with
> your Suggested-by (did not give you authorship let me know if that's OK
> please).

That's absolutely fine.

> One thing to mention, I added a Fixes: tag that goes back to the initial
> GICv5 commit, I don't know whether it is fixing more than that, it does
> not look like by a quick grep through kernel code but I am not sure.

This would do. If we find other problems, we'll backport it.

-- 
Catalin

Re: [PATCH] irqchip/gic-v5: Fix GIC CDEOI instruction encoding

[Marc Zyngier]

On Mon, 06 Oct 2025 14:30:04 +0100,
Catalin Marinas <catalin.marinas@arm.com> wrote:
> 
> On Mon, Oct 06, 2025 at 12:07:58PM +0200, Lorenzo Pieralisi wrote:
> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > index 6455db1b54fd..6cf8c46ddde5 100644
> > --- a/arch/arm64/include/asm/sysreg.h
> > +++ b/arch/arm64/include/asm/sysreg.h
> > @@ -113,14 +113,14 @@
> >  /* Register-based PAN access, for save/restore purposes */
> >  #define SYS_PSTATE_PAN			sys_reg(3, 0, 4, 2, 3)
> >  
> > -#define __SYS_BARRIER_INSN(op0, op1, CRn, CRm, op2, Rt)			\
> > +#define __SYS_INSN(op0, op1, CRn, CRm, op2, Rt)				\
> >  	__emit_inst(0xd5000000					|	\
> >  		    sys_insn((op0), (op1), (CRn), (CRm), (op2))	|	\
> >  		    ((Rt) & 0x1f))
> >  
> > -#define SB_BARRIER_INSN			__SYS_BARRIER_INSN(0, 3, 3, 0, 7, 31)
> > -#define GSB_SYS_BARRIER_INSN		__SYS_BARRIER_INSN(1, 0, 12, 0, 0, 31)
> > -#define GSB_ACK_BARRIER_INSN		__SYS_BARRIER_INSN(1, 0, 12, 0, 1, 31)
> > +#define SB_BARRIER_INSN			__SYS_INSN(0, 3, 3, 0, 7, 31)
> > +#define GSB_SYS_BARRIER_INSN		__SYS_INSN(1, 0, 12, 0, 0, 31)
> > +#define GSB_ACK_BARRIER_INSN		__SYS_INSN(1, 0, 12, 0, 1, 31)
> >  
> >  /* Data cache zero operations */
> >  #define SYS_DC_ISW			sys_insn(1, 0, 7, 6, 2)
> > @@ -1075,7 +1075,6 @@
> >  #define GICV5_OP_GIC_CDDIS		sys_insn(1, 0, 12, 1, 0)
> >  #define GICV5_OP_GIC_CDHM		sys_insn(1, 0, 12, 2, 1)
> >  #define GICV5_OP_GIC_CDEN		sys_insn(1, 0, 12, 1, 1)
> > -#define GICV5_OP_GIC_CDEOI		sys_insn(1, 0, 12, 1, 7)
> >  #define GICV5_OP_GIC_CDPEND		sys_insn(1, 0, 12, 1, 4)
> >  #define GICV5_OP_GIC_CDPRI		sys_insn(1, 0, 12, 1, 2)
> >  #define GICV5_OP_GIC_CDRCFG		sys_insn(1, 0, 12, 1, 5)
> > @@ -1129,6 +1128,17 @@
> >  #define gicr_insn(insn)			read_sysreg_s(GICV5_OP_GICR_##insn)
> >  #define gic_insn(v, insn)		write_sysreg_s(v, GICV5_OP_GIC_##insn)
> >  
> > +/*
> > + * GIC CDEOI encoding requires Rt to be 0b11111.
> > + * gic_insn() with an immediate value of 0 cannot be used to encode it
> > + * because some compilers do not follow asm inline constraints in
> > + * write_sysreg_s() to turn an immediate 0 value into an XZR as
> > + * MSR source register.
> > + * Use __SYS_INSN to specify its precise encoding explicitly.
> > + */
> > +#define GICV5_CDEOI_INSN		__SYS_INSN(1, 0, 12, 1, 7, 31)
> > +#define gic_cdeoi()			asm volatile(GICV5_CDEOI_INSN)
> 
> Would something like this work? Completely untested (and build still
> going):
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 6604fd6f33f4..7aa962f7bdd6 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -1234,7 +1234,10 @@
>  #define write_sysreg_s(v, r) do {					\
>  	u64 __val = (u64)(v);						\
>  	u32 __maybe_unused __check_r = (u32)(r);			\
> -	asm volatile(__msr_s(r, "%x0") : : "rZ" (__val));		\
> +	if (__builtin_constant_p(__val) && __val == 0)			\
> +		asm volatile(__msr_s(r, "xzr"));			\
> +	else								\
> +		asm volatile(__msr_s(r, "%x0") : : "r" (__val));	\
>  } while (0)

Ah, looks much better, and keeps the crap localised. The only thing is
that we lose the 'Z' constraint, and that reduces the incentive for
LLVM to have a better codegen. Maybe add a comment for a good measure?

Anyway, FWIW:

Acked-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.