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

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?

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?

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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3