Re: [PATCH 3/5] irqchip/gic-v5: Synchronize CPU interface disable

On Mon, 2026-08-10 at 10:54 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but it leaves
> critical architectural violations unaddressed due to a
> misunderstanding of ARM64 requirements (as noted in the commit
> message). Missing mandatory `isb()` instructions immediately after
> writing to control-plane system registers (`ICC_*_EL1`). The ARM64
> architecture subsystem guidelines mandate that every write to a
> control-plane system register MUST be followed by an `isb()` as the
> very next instruction. The `isb()` synchronizes the local CPU
> pipeline; it does not wait for external interrupt delivery. The patch
> author explicitly omitted an `isb()` after writing to
> `SYS_ICC_CR0_EL1` in `gicv5_cpu_enable_interrupts()`, claiming
> 'interrupt delivery is asynchronous'. Furthermore, there are
> preexisting violations across multiple functions
> (`gicv5_cpu_enable_interrupts()`, `gicv5_ppi_priority_init()`, and
> `write_ppi_sysreg_s()`) where writes to `ICC_*_EL1` registers are
> batched with intervening instructions before an eventual `isb()`, or
> lack one entirely.
> --
> 
> --- Patch [3]: [PATCH 3/5] irqchip/gic-v5: Synchronize CPU interface
> disable ---
> commit 55c441d845df8718ca0aac7ff0ab6a4a351c19dc
> Author: Sascha Bischoff <[email protected]>
> irqchip/gic-v5: Synchronize CPU interface disable
> 
> This commit adds a missing ISB instruction after the ICC_CR0_EL1
> write in
> gicv5_cpu_disable_interrupts() to ensure interrupts are disabled
> before
> the function returns.
> 
> Link:
> https://sashiko.dev/#/patchset/[email protected]?part=6
> 
> > Add an ISB after the ICC_CR0_EL1 write to ensure interrupts are
> > disabled before the function returns. No corresponding ISB is added
> > when enabling the interface, as interrupt delivery is asynchronous
> > and
> > there is no obvious benefit to waiting for it.
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does omitting the ISB
> leave the CPU pipeline in an undefined state?

No. This really isn't an issue.

> 
> The ARM64 architecture requires an isb() immediately after writing to
> control-plane system registers like ICC_*_EL1 to synchronize the
> local CPU
> pipeline, not just to wait for external interrupt delivery.
> 
> > diff --git a/drivers/irqchip/irq-gic-v5.c b/drivers/irqchip/irq-
> > gic-v5.c
> > index e9d1795235a66..6a99c541b559b 100644
> > --- a/drivers/irqchip/irq-gic-v5.c
> > +++ b/drivers/irqchip/irq-gic-v5.c
> > @@ -983,6 +983,7 @@ static void gicv5_cpu_disable_interrupts(void)
> >  
> >  	cr0 = FIELD_PREP(ICC_CR0_EL1_EN, 0);
> >  	write_sysreg_s(cr0, SYS_ICC_CR0_EL1);
> > +	isb();
> >  }
> >  
> >  static void gicv5_cpu_enable_interrupts(void)
> 
> [Severity: High]
> This is a pre-existing issue, but should
> gicv5_cpu_enable_interrupts()
> and other related functions also be updated to include the mandatory
> isb()
> instructions?

They are really not mandatory.

> 
> For example, in gicv5_cpu_enable_interrupts(), writes to registers
> like
> SYS_ICC_PPI_ENABLER0_EL1 and SYS_ICC_CR0_EL1 lack an immediate isb()
> after each write.
> 
> Similarly, are the batched writes in gicv5_ppi_priority_init() and
> the
> state updates in write_ppi_sysreg_s() missing their required isb()
> synchronizations before returning? 
> 
> Placing a return instruction or intervening instructions before an
> isb()
> can cause the CPU to observe undefined pipeline state.
> 

No. Won't fix.

Thanks,
Sascha