From: Mykola Kvach <mykola_kvach@epam.com>

The GICv2 CPU shutdown path currently writes 0 to GICC_CTLR.

Per IHI0048B.b section 2.3.1, clearing IRQBypDisGrp{0,1} and
FIQBypDisGrp{0,1} selects bypass rather than deasserted interrupt
outputs when the CPU interface stops driving them. Tables 2-2 and 2-3
show that a zeroed GICC_CTLR can fall back to the legacy IRQ/FIQ inputs
instead of fully disabling the interface.

Fix this by reading GICC_CTLR, setting the bypass-disable bits, and
clearing both group-enable bits before writing the value back. Keep the
existing GICC_CTL_ENABLE definition for the init path and use a separate
mask for the shutdown-side group-enable handling.

Section 2.3.2 also states that wakeup event signals remain available
even when both GIC interrupt signaling and interrupt bypass are
disabled, so disabling bypass does not break the power-management use
case, i.e. suspend modes.

Fixes: 5e40a1b4351e ("arm: SMP CPU shutdown")
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
 xen/arch/arm/gic-v2.c          |  7 ++++++-
 xen/arch/arm/include/asm/gic.h | 21 +++++++++++++++++++--
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index b23e72a3d0..22aa25bad0 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -408,7 +408,12 @@ static void gicv2_cpu_init(void)
 
 static void gicv2_cpu_disable(void)
 {
-    writel_gicc(0x0, GICC_CTLR);
+    uint32_t ctlr = readl_gicc(GICC_CTLR);
+
+    ctlr |= GICC_CTL_BYP_DIS_MASK;
+    ctlr &= ~GICC_CTL_ENABLE_GRP_MASK;
+
+    writel_gicc(ctlr, GICC_CTLR);
 }
 
 static void gicv2_hyp_init(void)
diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
index 8e713aa477..da285adb83 100644
--- a/xen/arch/arm/include/asm/gic.h
+++ b/xen/arch/arm/include/asm/gic.h
@@ -102,8 +102,25 @@
 #define GICD_TYPE_SEC   0x400
 #define GICD_TYPER_DVIS (1U << 18)
 
-#define GICC_CTL_ENABLE 0x1
-#define GICC_CTL_EOI    (0x1 << 9)
+/*
+ * Keep the legacy name for bit[0]. In the Non-secure view of a GICv2 with
+ * Security Extensions this is the Group 1 enable bit; otherwise it is the
+ * Group 0.
+ */
+#define GICC_CTL_ENABLE        (0x1 << 0)
+/* Bit[1] is the second group-enable bit when separate group enables exist. */
+#define GICC_CTL_ENABLE_GRP1   (0x1 << 1)
+#define GICC_CTL_FIQBypDisGrp0 (0x1 << 5)
+#define GICC_CTL_IRQBypDisGrp0 (0x1 << 6)
+#define GICC_CTL_FIQBypDisGrp1 (0x1 << 7)
+#define GICC_CTL_IRQBypDisGrp1 (0x1 << 8)
+#define GICC_CTL_EOI           (0x1 << 9)
+
+/* Shutdown clears both possible group-enable bits, regardless of layout. */
+#define GICC_CTL_ENABLE_GRP_MASK (GICC_CTL_ENABLE | GICC_CTL_ENABLE_GRP1)
+#define GICC_CTL_BYP_DIS_MASK                          \
+    (GICC_CTL_FIQBypDisGrp0 | GICC_CTL_IRQBypDisGrp0 | \
+     GICC_CTL_FIQBypDisGrp1 | GICC_CTL_IRQBypDisGrp1)
 
 #define GICC_IA_IRQ       0x03ff
 #define GICC_IA_CPU_MASK  0x1c00
-- 
2.43.0

Hi Mykola,

> On 10 Apr 2026, at 10:36, Mykola Kvach <xakep.amatop@gmail.com> wrote:
> 
> From: Mykola Kvach <mykola_kvach@epam.com>
> 
> The GICv2 CPU shutdown path currently writes 0 to GICC_CTLR.
> 
> Per IHI0048B.b section 2.3.1, clearing IRQBypDisGrp{0,1} and
> FIQBypDisGrp{0,1} selects bypass rather than deasserted interrupt
> outputs when the CPU interface stops driving them. Tables 2-2 and 2-3
> show that a zeroed GICC_CTLR can fall back to the legacy IRQ/FIQ inputs
> instead of fully disabling the interface.
> 
> Fix this by reading GICC_CTLR, setting the bypass-disable bits, and
> clearing both group-enable bits before writing the value back. Keep the
> existing GICC_CTL_ENABLE definition for the init path and use a separate
> mask for the shutdown-side group-enable handling.
> 
> Section 2.3.2 also states that wakeup event signals remain available
> even when both GIC interrupt signaling and interrupt bypass are
> disabled, so disabling bypass does not break the power-management use
> case, i.e. suspend modes.
> 
> Fixes: 5e40a1b4351e ("arm: SMP CPU shutdown")
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> ---
> xen/arch/arm/gic-v2.c          |  7 ++++++-
> xen/arch/arm/include/asm/gic.h | 21 +++++++++++++++++++--
> 2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index b23e72a3d0..22aa25bad0 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -408,7 +408,12 @@ static void gicv2_cpu_init(void)
> 
> static void gicv2_cpu_disable(void)
> {
> -    writel_gicc(0x0, GICC_CTLR);
> +    uint32_t ctlr = readl_gicc(GICC_CTLR);
> +
> +    ctlr |= GICC_CTL_BYP_DIS_MASK;

If the GIC v2 implementation includes the Security Extensions, the bit 7-8
are reserved, but now we are unconditionally writing on them.

Cheers,
Luca

Hi Luca,

Thank you for the review.


On Mon, Apr 20, 2026 at 2:42 PM Luca Fancellu <Luca.Fancellu@arm.com> wrote:
>
> Hi Mykola,
>
> > On 10 Apr 2026, at 10:36, Mykola Kvach <xakep.amatop@gmail.com> wrote:
> >
> > From: Mykola Kvach <mykola_kvach@epam.com>
> >
> > The GICv2 CPU shutdown path currently writes 0 to GICC_CTLR.
> >
> > Per IHI0048B.b section 2.3.1, clearing IRQBypDisGrp{0,1} and
> > FIQBypDisGrp{0,1} selects bypass rather than deasserted interrupt
> > outputs when the CPU interface stops driving them. Tables 2-2 and 2-3
> > show that a zeroed GICC_CTLR can fall back to the legacy IRQ/FIQ inputs
> > instead of fully disabling the interface.
> >
> > Fix this by reading GICC_CTLR, setting the bypass-disable bits, and
> > clearing both group-enable bits before writing the value back. Keep the
> > existing GICC_CTL_ENABLE definition for the init path and use a separate
> > mask for the shutdown-side group-enable handling.
> >
> > Section 2.3.2 also states that wakeup event signals remain available
> > even when both GIC interrupt signaling and interrupt bypass are
> > disabled, so disabling bypass does not break the power-management use
> > case, i.e. suspend modes.
> >
> > Fixes: 5e40a1b4351e ("arm: SMP CPU shutdown")
> > Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> > ---
> > xen/arch/arm/gic-v2.c          |  7 ++++++-
> > xen/arch/arm/include/asm/gic.h | 21 +++++++++++++++++++--
> > 2 files changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index b23e72a3d0..22aa25bad0 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -408,7 +408,12 @@ static void gicv2_cpu_init(void)
> >
> > static void gicv2_cpu_disable(void)
> > {
> > -    writel_gicc(0x0, GICC_CTLR);
> > +    uint32_t ctlr = readl_gicc(GICC_CTLR);
> > +
> > +    ctlr |= GICC_CTL_BYP_DIS_MASK;
>
> If the GIC v2 implementation includes the Security Extensions, the bit 7-8
> are reserved, but now we are unconditionally writing on them.

You are right.

I had assumed that, since these bits are reserved in that view, writing
them would be harmless. However, the specification does not say that
they are WI, so this is not something the patch should rely on.

Looking at it again, there may be a similar layout-dependent issue in
cpu_init() as well, since the current write there also forces all other
bits to zero apart from the ones explicitly set.

I will rework this patch accordingly so that cpu_disable() only updates
the bits that are architecturally defined for the current GICC_CTLR
layout, and I will take another look at the init path separately.


Best regards,
Mykola

>
> Cheers,
> Luca
>

Hmm, this landed in my junk folder.

On 10/04/2026 11:36, Mykola Kvach wrote:
> From: Mykola Kvach <mykola_kvach@epam.com>
> 
> The GICv2 CPU shutdown path currently writes 0 to GICC_CTLR.
> 
> Per IHI0048B.b section 2.3.1, clearing IRQBypDisGrp{0,1} and
> FIQBypDisGrp{0,1} selects bypass rather than deasserted interrupt
> outputs when the CPU interface stops driving them. Tables 2-2 and 2-3
> show that a zeroed GICC_CTLR can fall back to the legacy IRQ/FIQ inputs
> instead of fully disabling the interface.
> 
> Fix this by reading GICC_CTLR, setting the bypass-disable bits, and
> clearing both group-enable bits before writing the value back. Keep the
> existing GICC_CTL_ENABLE definition for the init path and use a separate
> mask for the shutdown-side group-enable handling.
IIUC we don't need to worry about not setting the bypass-disable bits in cpu
init (we only set group 0 and EOI) because they are relevant only when the bit 0
is disabled i.e. the path this patch changes?

> 
> Section 2.3.2 also states that wakeup event signals remain available
> even when both GIC interrupt signaling and interrupt bypass are
> disabled, so disabling bypass does not break the power-management use
> case, i.e. suspend modes.
> 
> Fixes: 5e40a1b4351e ("arm: SMP CPU shutdown")
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Hi Michal,

Thank you for the review.

On Mon, Apr 20, 2026 at 11:38 AM Orzel, Michal <michal.orzel@amd.com> wrote:
>
> Hmm, this landed in my junk folder.
>
> On 10/04/2026 11:36, Mykola Kvach wrote:
> > From: Mykola Kvach <mykola_kvach@epam.com>
> >
> > The GICv2 CPU shutdown path currently writes 0 to GICC_CTLR.
> >
> > Per IHI0048B.b section 2.3.1, clearing IRQBypDisGrp{0,1} and
> > FIQBypDisGrp{0,1} selects bypass rather than deasserted interrupt
> > outputs when the CPU interface stops driving them. Tables 2-2 and 2-3
> > show that a zeroed GICC_CTLR can fall back to the legacy IRQ/FIQ inputs
> > instead of fully disabling the interface.
> >
> > Fix this by reading GICC_CTLR, setting the bypass-disable bits, and
> > clearing both group-enable bits before writing the value back. Keep the
> > existing GICC_CTL_ENABLE definition for the init path and use a separate
> > mask for the shutdown-side group-enable handling.
> IIUC we don't need to worry about not setting the bypass-disable bits in cpu
> init (we only set group 0 and EOI) because they are relevant only when the bit 0
> is disabled i.e. the path this patch changes?

Yes, that is my understanding as well.

In cpu_init(), Xen enables the CPU interface by programming
GICC_CTL_ENABLE | GICC_CTL_EOI. In that state, the GIC CPU interface
drives the interrupt outputs, so leaving the bypass-disable bits clear
does not cause fallback to the legacy bypass path.

The issue is specific to cpu_disable(): once the group-enable bits are
cleared, leaving the bypass-disable bits clear can select the legacy
IRQ/FIQ bypass inputs instead of fully deasserting the outputs.

>
> >
> > Section 2.3.2 also states that wakeup event signals remain available
> > even when both GIC interrupt signaling and interrupt bypass are
> > disabled, so disabling bypass does not break the power-management use
> > case, i.e. suspend modes.
> >
> > Fixes: 5e40a1b4351e ("arm: SMP CPU shutdown")
> > Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>
> ~Michal
>

Best regards,
Mykola