[PATCH] irqchip/gic-v3: Remove the ISB between AP0Rn and AP1Rn

[PATCH] irqchip/gic-v3: Remove the ISB between AP0Rn and AP1Rn

[Li Dou]

ARM GICv3 specification says(in section 4.8.4 "System register access to
the Active Priorities registers"):

	an ISB is not required between each write to ICC_AP0R<n>_EL1,
	Secure ICC_AP1R<n>_EL1, and Non-secure ICC_AP1R<n>_EL1.

It means we can use one ISB after resetting AP0Rn and AP1Rn for context
synchronization. So just remove the previous one.

Signed-off-by: Li Dou <li8d6kernel@163.com>
---
 drivers/irqchip/irq-gic-v3.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 6e1fa5b247fc..3c48f46abb25 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1205,8 +1205,6 @@ static void gic_cpu_sys_reg_init(void)
 		case 4:
 			write_gicreg(0, ICC_AP0R0_EL1);
 		}
-
-		isb();
 	}
 
 	switch(pribits) {
-- 
2.43.0

Re: [PATCH] irqchip/gic-v3: Remove the ISB between AP0Rn and AP1Rn

[Marc Zyngier]

On Mon, 24 Aug 2026 03:34:03 +0100,
Li Dou <li8d6kernel@163.com> wrote:
> 
> ARM GICv3 specification says(in section 4.8.4 "System register access to
> the Active Priorities registers"):
> 
> 	an ISB is not required between each write to ICC_AP0R<n>_EL1,
> 	Secure ICC_AP1R<n>_EL1, and Non-secure ICC_AP1R<n>_EL1.
> 
> It means we can use one ISB after resetting AP0Rn and AP1Rn for context
> synchronization. So just remove the previous one.

I'm afraid you have misinterpreted the spec.

Within each of the AP0Rn, Secure AP1Rn and Non-Secure AP1Rn classes,
for any value of 'n', there is no need for any synchronisation.

However, between classes, you absolutely need an ISB. And this is the
reason why, just above the note you quote:

<quote>
Writes to these registers in any order other than the following can
result in UNPREDICTABLE behavior:

1. ICC_AP0R<n>_EL1.
2. Secure ICC_AP1R<n>_EL1.
3. Non-secure ICC_AP1R<n>_EL1.
</quote>

While you can perfectly write each individual register of (1), (2) or
(3) without any synchronisation, you absolutely must have a CSE
*between* each of (1), (2) and (3). Linux being non-secure only, you
only deal with (1) and (3), and you must have an ISB in between.
Without it, reordering can happen and you end-up violating the above
rule.

To conclude, this patch does not optimise anything. Not only this is
something that only happens once per CPU boot (aka *never*), it
instead introduces a very subtle ordering bug.

It therefore must not be applied.

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [PATCH] irqchip/gic-v3: Remove the ISB between AP0Rn and AP1Rn

[Li Dou]

Hi Marc,

On Mon, 24 Aug 2026 17:34:34 +0100, Marc Zyngier wrote:
>On Mon, 24 Aug 2026 03:34:03 +0100,
>Li Dou <li8d6kernel@163.com> wrote:
>> 
>> ARM GICv3 specification says(in section 4.8.4 "System register access to
>> the Active Priorities registers"):
>> 
>> 	an ISB is not required between each write to ICC_AP0R<n>_EL1,
>> 	Secure ICC_AP1R<n>_EL1, and Non-secure ICC_AP1R<n>_EL1.
>> 
>> It means we can use one ISB after resetting AP0Rn and AP1Rn for context
>> synchronization. So just remove the previous one.
>
>I'm afraid you have misinterpreted the spec.
>
>Within each of the AP0Rn, Secure AP1Rn and Non-Secure AP1Rn classes,
>for any value of 'n', there is no need for any synchronisation.
>
>However, between classes, you absolutely need an ISB. And this is the
>reason why, just above the note you quote:
>
><quote>
>Writes to these registers in any order other than the following can
>result in UNPREDICTABLE behavior:
>
>1. ICC_AP0R<n>_EL1.
>2. Secure ICC_AP1R<n>_EL1.
>3. Non-secure ICC_AP1R<n>_EL1.
></quote>
>
>While you can perfectly write each individual register of (1), (2) or
>(3) without any synchronisation, you absolutely must have a CSE
>*between* each of (1), (2) and (3). Linux being non-secure only, you
>only deal with (1) and (3), and you must have an ISB in between.
>Without it, reordering can happen and you end-up violating the above
>rule.

The spec sometimes is overly convoluted. Thanks for the clarification.

Best regards,
Li

Re: [PATCH] irqchip/gic-v3: Remove the ISB between AP0Rn and AP1Rn

[Marc Zyngier]

On Tue, 25 Aug 2026 04:04:54 +0100,
Li Dou <li8d6kernel@163.com> wrote:
> 
> Hi Marc,
> 
> On Mon, 24 Aug 2026 17:34:34 +0100, Marc Zyngier wrote:
> >On Mon, 24 Aug 2026 03:34:03 +0100,
> >Li Dou <li8d6kernel@163.com> wrote:
> >> 
> >> ARM GICv3 specification says(in section 4.8.4 "System register access to
> >> the Active Priorities registers"):
> >> 
> >> 	an ISB is not required between each write to ICC_AP0R<n>_EL1,
> >> 	Secure ICC_AP1R<n>_EL1, and Non-secure ICC_AP1R<n>_EL1.
> >> 
> >> It means we can use one ISB after resetting AP0Rn and AP1Rn for context
> >> synchronization. So just remove the previous one.
> >
> >I'm afraid you have misinterpreted the spec.
> >
> >Within each of the AP0Rn, Secure AP1Rn and Non-Secure AP1Rn classes,
> >for any value of 'n', there is no need for any synchronisation.
> >
> >However, between classes, you absolutely need an ISB. And this is the
> >reason why, just above the note you quote:
> >
> ><quote>
> >Writes to these registers in any order other than the following can
> >result in UNPREDICTABLE behavior:
> >
> >1. ICC_AP0R<n>_EL1.
> >2. Secure ICC_AP1R<n>_EL1.
> >3. Non-secure ICC_AP1R<n>_EL1.
> ></quote>
> >
> >While you can perfectly write each individual register of (1), (2) or
> >(3) without any synchronisation, you absolutely must have a CSE
> >*between* each of (1), (2) and (3). Linux being non-secure only, you
> >only deal with (1) and (3), and you must have an ISB in between.
> >Without it, reordering can happen and you end-up violating the above
> >rule.
> 
> The spec sometimes is overly convoluted. Thanks for the clarification.

The spec is pretty clear in this particular case. It outlines the
order by which group of registers must be written to, and therefore
there is only one way to interpret the accompanying note.

As for any spec, you need to read the whole thing, and not take a
particular statement out of context.

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.