xen/arch/arm/gic-v2.c | 16 +++++++++++++++- xen/arch/arm/include/asm/gic.h | 25 +++++++++++++++++++++++-- 2 files changed, 38 insertions(+), 3 deletions(-)
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 the architected bypass-disable
bits 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, then setting the bypass-disable bits and
clearing the group-enable bits that are architecturally defined for the
current GICC_CTLR view before writing the value back. When Security
Extensions are implemented Xen accesses the Non-secure copy of
GICC_CTLR, where IRQBypDisGrp1 and FIQBypDisGrp1 are at bits [6:5] and
bits [8:7] are reserved.
Without Security Extensions there is no separate Secure/Non-secure CPU
interface view, so disabling both group-enable bits affects the shared
interface state. This is still appropriate for the CPU shutdown path,
which is expected to stop normal interrupt delivery through the interface
and rely only on the architecturally separate wakeup event signaling.
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>
---
Changes in v2:
- derive the shutdown masks from the active GICC_CTLR layout
- use the Non-secure GICC_CTLR layout when GICD_TYPER.SecurityExtn is set
- stop writing reserved bits [8:7] on Security Extensions systems
---
xen/arch/arm/gic-v2.c | 16 +++++++++++++++-
xen/arch/arm/include/asm/gic.h | 25 +++++++++++++++++++++++--
2 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 014f955967..241c1ff5c5 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -408,7 +408,21 @@ static void gicv2_cpu_init(void)
static void gicv2_cpu_disable(void)
{
- writel_gicc(0x0, GICC_CTLR);
+ uint32_t ctlr = readl_gicc(GICC_CTLR);
+
+ if ( readl_gicd(GICD_TYPER) & GICD_TYPE_SEC )
+ {
+ ctlr |= GICC_NS_CTLR_BYPASS_DISABLE_GRP1_MASK;
+ ctlr &= ~GICC_CTL_ENABLE;
+ }
+ else
+ {
+ ctlr |= GICC_CTLR_BYPASS_DISABLE_GRP0_MASK |
+ GICC_CTLR_BYPASS_DISABLE_GRP1_MASK;
+ ctlr &= ~(GICC_CTL_ENABLE | GICC_CTL_ENABLE_GRP1);
+ }
+
+ 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..ff22dea40d 100644
--- a/xen/arch/arm/include/asm/gic.h
+++ b/xen/arch/arm/include/asm/gic.h
@@ -102,8 +102,29 @@
#define GICD_TYPE_SEC 0x400
#define GICD_TYPER_DVIS (1U << 18)
-#define GICC_CTL_ENABLE 0x1
-#define GICC_CTL_EOI (0x1 << 9)
+/*
+ * Xen runs in the Non-secure world. When Security Extensions are present,
+ * Xen accesses the Non-secure GICC_CTLR view, where bit[0] is EnableGrp1
+ * and bits[6:5] are the Group 1 bypass-disable bits. Otherwise Xen sees the
+ * common GICC_CTLR layout, where bit[0] is EnableGrp0, bit[1] is EnableGrp1,
+ * bits[6:5] are the Group 0 bypass-disable bits, and bits[8:7] are the
+ * Group 1 bypass-disable bits.
+ */
+#define GICC_CTL_ENABLE (0x1 << 0)
+#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_CTLR_BYPASS_DISABLE_GRP0_MASK \
+ (GICC_CTL_FIQBypDisGrp0 | GICC_CTL_IRQBypDisGrp0)
+#define GICC_CTLR_BYPASS_DISABLE_GRP1_MASK \
+ (GICC_CTL_FIQBypDisGrp1 | GICC_CTL_IRQBypDisGrp1)
+#define GICC_NS_CTLR_BYPASS_DISABLE_GRP1_MASK \
+ GICC_CTLR_BYPASS_DISABLE_GRP0_MASK
+
+#define GICC_CTL_EOI (0x1 << 9)
#define GICC_IA_IRQ 0x03ff
#define GICC_IA_CPU_MASK 0x1c00
--
2.43.0
On 28-Apr-26 13:57, 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 the architected bypass-disable
> bits 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, then setting the bypass-disable bits and
> clearing the group-enable bits that are architecturally defined for the
> current GICC_CTLR view before writing the value back. When Security
> Extensions are implemented Xen accesses the Non-secure copy of
> GICC_CTLR, where IRQBypDisGrp1 and FIQBypDisGrp1 are at bits [6:5] and
> bits [8:7] are reserved.
>
> Without Security Extensions there is no separate Secure/Non-secure CPU
> interface view, so disabling both group-enable bits affects the shared
> interface state. This is still appropriate for the CPU shutdown path,
> which is expected to stop normal interrupt delivery through the interface
> and rely only on the architecturally separate wakeup event signaling.
>
> 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>
>
> ---
> Changes in v2:
> - derive the shutdown masks from the active GICC_CTLR layout
> - use the Non-secure GICC_CTLR layout when GICD_TYPER.SecurityExtn is set
> - stop writing reserved bits [8:7] on Security Extensions systems
> ---
> xen/arch/arm/gic-v2.c | 16 +++++++++++++++-
> xen/arch/arm/include/asm/gic.h | 25 +++++++++++++++++++++++--
> 2 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 014f955967..241c1ff5c5 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -408,7 +408,21 @@ static void gicv2_cpu_init(void)
>
> static void gicv2_cpu_disable(void)
> {
> - writel_gicc(0x0, GICC_CTLR);
> + uint32_t ctlr = readl_gicc(GICC_CTLR);
> +
> + if ( readl_gicd(GICD_TYPER) & GICD_TYPE_SEC )
> + {
> + ctlr |= GICC_NS_CTLR_BYPASS_DISABLE_GRP1_MASK;
> + ctlr &= ~GICC_CTL_ENABLE;
> + }
> + else
> + {
> + ctlr |= GICC_CTLR_BYPASS_DISABLE_GRP0_MASK |
> + GICC_CTLR_BYPASS_DISABLE_GRP1_MASK;
> + ctlr &= ~(GICC_CTL_ENABLE | GICC_CTL_ENABLE_GRP1);
> + }
I don't understand why you want to set both G0 and G1,
Bits 5-6 in the NS view control Group 1, while the same bits in the
Secure/single-security-state view control Group 0. So in the latter case you
don't need to set G1. Without security extensions all interrupts are G0 and with
security extensions (NS access) all interrupts are G1. The spec guarantees the
functional mapping.
~Michal
> +
> + 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..ff22dea40d 100644
> --- a/xen/arch/arm/include/asm/gic.h
> +++ b/xen/arch/arm/include/asm/gic.h
> @@ -102,8 +102,29 @@
> #define GICD_TYPE_SEC 0x400
> #define GICD_TYPER_DVIS (1U << 18)
>
> -#define GICC_CTL_ENABLE 0x1
> -#define GICC_CTL_EOI (0x1 << 9)
> +/*
> + * Xen runs in the Non-secure world. When Security Extensions are present,
> + * Xen accesses the Non-secure GICC_CTLR view, where bit[0] is EnableGrp1
> + * and bits[6:5] are the Group 1 bypass-disable bits. Otherwise Xen sees the
> + * common GICC_CTLR layout, where bit[0] is EnableGrp0, bit[1] is EnableGrp1,
> + * bits[6:5] are the Group 0 bypass-disable bits, and bits[8:7] are the
> + * Group 1 bypass-disable bits.
> + */
> +#define GICC_CTL_ENABLE (0x1 << 0)
> +#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_CTLR_BYPASS_DISABLE_GRP0_MASK \
> + (GICC_CTL_FIQBypDisGrp0 | GICC_CTL_IRQBypDisGrp0)
> +#define GICC_CTLR_BYPASS_DISABLE_GRP1_MASK \
> + (GICC_CTL_FIQBypDisGrp1 | GICC_CTL_IRQBypDisGrp1)
> +#define GICC_NS_CTLR_BYPASS_DISABLE_GRP1_MASK \
> + GICC_CTLR_BYPASS_DISABLE_GRP0_MASK
> +
> +#define GICC_CTL_EOI (0x1 << 9)
>
> #define GICC_IA_IRQ 0x03ff
> #define GICC_IA_CPU_MASK 0x1c00
Hi Michal,
On Wed, Apr 29, 2026 at 11:20 AM Orzel, Michal <michal.orzel@amd.com> wrote:
>
>
>
> On 28-Apr-26 13:57, 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 the architected bypass-disable
> > bits 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, then setting the bypass-disable bits and
> > clearing the group-enable bits that are architecturally defined for the
> > current GICC_CTLR view before writing the value back. When Security
> > Extensions are implemented Xen accesses the Non-secure copy of
> > GICC_CTLR, where IRQBypDisGrp1 and FIQBypDisGrp1 are at bits [6:5] and
> > bits [8:7] are reserved.
> >
> > Without Security Extensions there is no separate Secure/Non-secure CPU
> > interface view, so disabling both group-enable bits affects the shared
> > interface state. This is still appropriate for the CPU shutdown path,
> > which is expected to stop normal interrupt delivery through the interface
> > and rely only on the architecturally separate wakeup event signaling.
> >
> > 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>
> >
> > ---
> > Changes in v2:
> > - derive the shutdown masks from the active GICC_CTLR layout
> > - use the Non-secure GICC_CTLR layout when GICD_TYPER.SecurityExtn is set
> > - stop writing reserved bits [8:7] on Security Extensions systems
> > ---
> > xen/arch/arm/gic-v2.c | 16 +++++++++++++++-
> > xen/arch/arm/include/asm/gic.h | 25 +++++++++++++++++++++++--
> > 2 files changed, 38 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index 014f955967..241c1ff5c5 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -408,7 +408,21 @@ static void gicv2_cpu_init(void)
> >
> > static void gicv2_cpu_disable(void)
> > {
> > - writel_gicc(0x0, GICC_CTLR);
> > + uint32_t ctlr = readl_gicc(GICC_CTLR);
> > +
> > + if ( readl_gicd(GICD_TYPER) & GICD_TYPE_SEC )
> > + {
> > + ctlr |= GICC_NS_CTLR_BYPASS_DISABLE_GRP1_MASK;
> > + ctlr &= ~GICC_CTL_ENABLE;
> > + }
> > + else
> > + {
> > + ctlr |= GICC_CTLR_BYPASS_DISABLE_GRP0_MASK |
> > + GICC_CTLR_BYPASS_DISABLE_GRP1_MASK;
> > + ctlr &= ~(GICC_CTL_ENABLE | GICC_CTL_ENABLE_GRP1);
> > + }
> I don't understand why you want to set both G0 and G1,
> Bits 5-6 in the NS view control Group 1, while the same bits in the
> Secure/single-security-state view control Group 0. So in the latter case you
> don't need to set G1. Without security extensions all interrupts are G0 and with
> security extensions (NS access) all interrupts are G1. The spec guarantees the
> functional mapping.
I agree that this is not about Xen using both interrupt groups during
normal operation.
There are two separate points here.
For the group-enable bits, Xen currently only enables bit 0 in
gicv2_cpu_init(). So, in today's code, EnableGrp1 is expected to be clear
already. However, the old shutdown path wrote 0 to GICC_CTLR, which also
cleared every group-enable bit visible in the current view. Since this
patch changes the shutdown path from a plain zero write to a
read/modify/write, I wanted to preserve that part of the old shutdown
semantics and avoid leaving any normal interrupt delivery enabled in the
common GICC_CTLR view.
For the bypass-disable bits, the reason for setting both groups in the
single-security-state/common view is the GICv2 bypass logic, not normal
interrupt delivery. Once the group-enable bits are clear, the CPU
interface is no longer driving the physical IRQ/FIQ outputs through
normal GIC delivery. At that point, the bypass-disable bits decide
whether those outputs are deasserted or driven by the legacy inputs.
For example, with EnableGrp1 == 0, EnableGrp0 == 0 and FIQEn == 0,
Table 2-2 requires IRQBypDisGrp1 to be set for the IRQ output to be
deasserted. Similarly, Table 2-3 requires both FIQBypDisGrp0 and
FIQBypDisGrp1 to be set for the FIQ output to be deasserted. This is why
the common-view case disables the bypass paths for both groups.
This is also not meant to make FIQ a supported delivery mode for Xen. It
is the opposite: when the CPU interface is disabled, the final state
should not allow the physical FIQ output to be driven by the legacy
bypass input. Arm32 has some fallback plumbing for FIQ exceptions, but Xen
does not configure FIQ as its normal GICv2 interrupt delivery mode.
So the intent is:
* with Security Extensions, touch only the Non-secure view bits visible
to Xen;
* without Security Extensions, preserve the old "no normal delivery"
shutdown behaviour, while changing the bypass-disable bits so that
the physical outputs are deasserted rather than falling back to
legacy bypass.
If you prefer, I can also make v3 more conservative and only clear the
group-enable bit that Xen currently sets in gicv2_cpu_init(), i.e.
EnableGrp0 in the common view / EnableGrp1 in the Non-secure view. The
bypass-disable bits would still be set for all bypass paths visible in
the current GICC_CTLR view, because that part is about the physical
IRQ/FIQ outputs after normal delivery has been disabled, not about which
interrupt group Xen normally uses.
Thanks,
Mykola
>
> ~Michal
>
>
> > +
> > + 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..ff22dea40d 100644
> > --- a/xen/arch/arm/include/asm/gic.h
> > +++ b/xen/arch/arm/include/asm/gic.h
> > @@ -102,8 +102,29 @@
> > #define GICD_TYPE_SEC 0x400
> > #define GICD_TYPER_DVIS (1U << 18)
> >
> > -#define GICC_CTL_ENABLE 0x1
> > -#define GICC_CTL_EOI (0x1 << 9)
> > +/*
> > + * Xen runs in the Non-secure world. When Security Extensions are present,
> > + * Xen accesses the Non-secure GICC_CTLR view, where bit[0] is EnableGrp1
> > + * and bits[6:5] are the Group 1 bypass-disable bits. Otherwise Xen sees the
> > + * common GICC_CTLR layout, where bit[0] is EnableGrp0, bit[1] is EnableGrp1,
> > + * bits[6:5] are the Group 0 bypass-disable bits, and bits[8:7] are the
> > + * Group 1 bypass-disable bits.
> > + */
> > +#define GICC_CTL_ENABLE (0x1 << 0)
> > +#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_CTLR_BYPASS_DISABLE_GRP0_MASK \
> > + (GICC_CTL_FIQBypDisGrp0 | GICC_CTL_IRQBypDisGrp0)
> > +#define GICC_CTLR_BYPASS_DISABLE_GRP1_MASK \
> > + (GICC_CTL_FIQBypDisGrp1 | GICC_CTL_IRQBypDisGrp1)
> > +#define GICC_NS_CTLR_BYPASS_DISABLE_GRP1_MASK \
> > + GICC_CTLR_BYPASS_DISABLE_GRP0_MASK
> > +
> > +#define GICC_CTL_EOI (0x1 << 9)
> >
> > #define GICC_IA_IRQ 0x03ff
> > #define GICC_IA_CPU_MASK 0x1c00
>
On 01-May-26 09:22, Mykola Kvach wrote:
> Hi Michal,
>
> On Wed, Apr 29, 2026 at 11:20 AM Orzel, Michal <michal.orzel@amd.com> wrote:
>>
>>
>>
>> On 28-Apr-26 13:57, 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 the architected bypass-disable
>>> bits 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, then setting the bypass-disable bits and
>>> clearing the group-enable bits that are architecturally defined for the
>>> current GICC_CTLR view before writing the value back. When Security
>>> Extensions are implemented Xen accesses the Non-secure copy of
>>> GICC_CTLR, where IRQBypDisGrp1 and FIQBypDisGrp1 are at bits [6:5] and
>>> bits [8:7] are reserved.
>>>
>>> Without Security Extensions there is no separate Secure/Non-secure CPU
>>> interface view, so disabling both group-enable bits affects the shared
>>> interface state. This is still appropriate for the CPU shutdown path,
>>> which is expected to stop normal interrupt delivery through the interface
>>> and rely only on the architecturally separate wakeup event signaling.
>>>
>>> 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>
>>>
>>> ---
>>> Changes in v2:
>>> - derive the shutdown masks from the active GICC_CTLR layout
>>> - use the Non-secure GICC_CTLR layout when GICD_TYPER.SecurityExtn is set
>>> - stop writing reserved bits [8:7] on Security Extensions systems
>>> ---
>>> xen/arch/arm/gic-v2.c | 16 +++++++++++++++-
>>> xen/arch/arm/include/asm/gic.h | 25 +++++++++++++++++++++++--
>>> 2 files changed, 38 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>>> index 014f955967..241c1ff5c5 100644
>>> --- a/xen/arch/arm/gic-v2.c
>>> +++ b/xen/arch/arm/gic-v2.c
>>> @@ -408,7 +408,21 @@ static void gicv2_cpu_init(void)
>>>
>>> static void gicv2_cpu_disable(void)
>>> {
>>> - writel_gicc(0x0, GICC_CTLR);
>>> + uint32_t ctlr = readl_gicc(GICC_CTLR);
>>> +
>>> + if ( readl_gicd(GICD_TYPER) & GICD_TYPE_SEC )
>>> + {
>>> + ctlr |= GICC_NS_CTLR_BYPASS_DISABLE_GRP1_MASK;
>>> + ctlr &= ~GICC_CTL_ENABLE;
>>> + }
>>> + else
>>> + {
>>> + ctlr |= GICC_CTLR_BYPASS_DISABLE_GRP0_MASK |
>>> + GICC_CTLR_BYPASS_DISABLE_GRP1_MASK;
>>> + ctlr &= ~(GICC_CTL_ENABLE | GICC_CTL_ENABLE_GRP1);
>>> + }
>> I don't understand why you want to set both G0 and G1,
>> Bits 5-6 in the NS view control Group 1, while the same bits in the
>> Secure/single-security-state view control Group 0. So in the latter case you
>> don't need to set G1. Without security extensions all interrupts are G0 and with
>> security extensions (NS access) all interrupts are G1. The spec guarantees the
>> functional mapping.
>
> I agree that this is not about Xen using both interrupt groups during
> normal operation.
>
> There are two separate points here.
>
> For the group-enable bits, Xen currently only enables bit 0 in
> gicv2_cpu_init(). So, in today's code, EnableGrp1 is expected to be clear
> already. However, the old shutdown path wrote 0 to GICC_CTLR, which also
> cleared every group-enable bit visible in the current view. Since this
> patch changes the shutdown path from a plain zero write to a
> read/modify/write, I wanted to preserve that part of the old shutdown
> semantics and avoid leaving any normal interrupt delivery enabled in the
> common GICC_CTLR view.
>
> For the bypass-disable bits, the reason for setting both groups in the
> single-security-state/common view is the GICv2 bypass logic, not normal
> interrupt delivery. Once the group-enable bits are clear, the CPU
> interface is no longer driving the physical IRQ/FIQ outputs through
> normal GIC delivery. At that point, the bypass-disable bits decide
> whether those outputs are deasserted or driven by the legacy inputs.
>
> For example, with EnableGrp1 == 0, EnableGrp0 == 0 and FIQEn == 0,
> Table 2-2 requires IRQBypDisGrp1 to be set for the IRQ output to be
> deasserted. Similarly, Table 2-3 requires both FIQBypDisGrp0 and
> FIQBypDisGrp1 to be set for the FIQ output to be deasserted. This is why
> the common-view case disables the bypass paths for both groups.
>
> This is also not meant to make FIQ a supported delivery mode for Xen. It
> is the opposite: when the CPU interface is disabled, the final state
> should not allow the physical FIQ output to be driven by the legacy
> bypass input. Arm32 has some fallback plumbing for FIQ exceptions, but Xen
> does not configure FIQ as its normal GICv2 interrupt delivery mode.
>
> So the intent is:
>
> * with Security Extensions, touch only the Non-secure view bits visible
> to Xen;
> * without Security Extensions, preserve the old "no normal delivery"
> shutdown behaviour, while changing the bypass-disable bits so that
> the physical outputs are deasserted rather than falling back to
> legacy bypass.
>
> If you prefer, I can also make v3 more conservative and only clear the
> group-enable bit that Xen currently sets in gicv2_cpu_init(), i.e.
> EnableGrp0 in the common view / EnableGrp1 in the Non-secure view. The
> bypass-disable bits would still be set for all bypass paths visible in
> the current GICC_CTLR view, because that part is about the physical
> IRQ/FIQ outputs after normal delivery has been disabled, not about which
> interrupt group Xen normally uses.
The bypass argument is valid according to the tables. However, clearing G1 is
unnecessary, so it should be dropped. I can do that on commit if you agree. With
that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
~Michal
On Mon, May 4, 2026 at 10:19 AM Orzel, Michal <michal.orzel@amd.com> wrote:
>
>
>
> On 01-May-26 09:22, Mykola Kvach wrote:
> > Hi Michal,
> >
> > On Wed, Apr 29, 2026 at 11:20 AM Orzel, Michal <michal.orzel@amd.com> wrote:
> >>
> >>
> >>
> >> On 28-Apr-26 13:57, 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 the architected bypass-disable
> >>> bits 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, then setting the bypass-disable bits and
> >>> clearing the group-enable bits that are architecturally defined for the
> >>> current GICC_CTLR view before writing the value back. When Security
> >>> Extensions are implemented Xen accesses the Non-secure copy of
> >>> GICC_CTLR, where IRQBypDisGrp1 and FIQBypDisGrp1 are at bits [6:5] and
> >>> bits [8:7] are reserved.
> >>>
> >>> Without Security Extensions there is no separate Secure/Non-secure CPU
> >>> interface view, so disabling both group-enable bits affects the shared
> >>> interface state. This is still appropriate for the CPU shutdown path,
> >>> which is expected to stop normal interrupt delivery through the interface
> >>> and rely only on the architecturally separate wakeup event signaling.
> >>>
> >>> 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>
> >>>
> >>> ---
> >>> Changes in v2:
> >>> - derive the shutdown masks from the active GICC_CTLR layout
> >>> - use the Non-secure GICC_CTLR layout when GICD_TYPER.SecurityExtn is set
> >>> - stop writing reserved bits [8:7] on Security Extensions systems
> >>> ---
> >>> xen/arch/arm/gic-v2.c | 16 +++++++++++++++-
> >>> xen/arch/arm/include/asm/gic.h | 25 +++++++++++++++++++++++--
> >>> 2 files changed, 38 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> >>> index 014f955967..241c1ff5c5 100644
> >>> --- a/xen/arch/arm/gic-v2.c
> >>> +++ b/xen/arch/arm/gic-v2.c
> >>> @@ -408,7 +408,21 @@ static void gicv2_cpu_init(void)
> >>>
> >>> static void gicv2_cpu_disable(void)
> >>> {
> >>> - writel_gicc(0x0, GICC_CTLR);
> >>> + uint32_t ctlr = readl_gicc(GICC_CTLR);
> >>> +
> >>> + if ( readl_gicd(GICD_TYPER) & GICD_TYPE_SEC )
> >>> + {
> >>> + ctlr |= GICC_NS_CTLR_BYPASS_DISABLE_GRP1_MASK;
> >>> + ctlr &= ~GICC_CTL_ENABLE;
> >>> + }
> >>> + else
> >>> + {
> >>> + ctlr |= GICC_CTLR_BYPASS_DISABLE_GRP0_MASK |
> >>> + GICC_CTLR_BYPASS_DISABLE_GRP1_MASK;
> >>> + ctlr &= ~(GICC_CTL_ENABLE | GICC_CTL_ENABLE_GRP1);
> >>> + }
> >> I don't understand why you want to set both G0 and G1,
> >> Bits 5-6 in the NS view control Group 1, while the same bits in the
> >> Secure/single-security-state view control Group 0. So in the latter case you
> >> don't need to set G1. Without security extensions all interrupts are G0 and with
> >> security extensions (NS access) all interrupts are G1. The spec guarantees the
> >> functional mapping.
> >
> > I agree that this is not about Xen using both interrupt groups during
> > normal operation.
> >
> > There are two separate points here.
> >
> > For the group-enable bits, Xen currently only enables bit 0 in
> > gicv2_cpu_init(). So, in today's code, EnableGrp1 is expected to be clear
> > already. However, the old shutdown path wrote 0 to GICC_CTLR, which also
> > cleared every group-enable bit visible in the current view. Since this
> > patch changes the shutdown path from a plain zero write to a
> > read/modify/write, I wanted to preserve that part of the old shutdown
> > semantics and avoid leaving any normal interrupt delivery enabled in the
> > common GICC_CTLR view.
> >
> > For the bypass-disable bits, the reason for setting both groups in the
> > single-security-state/common view is the GICv2 bypass logic, not normal
> > interrupt delivery. Once the group-enable bits are clear, the CPU
> > interface is no longer driving the physical IRQ/FIQ outputs through
> > normal GIC delivery. At that point, the bypass-disable bits decide
> > whether those outputs are deasserted or driven by the legacy inputs.
> >
> > For example, with EnableGrp1 == 0, EnableGrp0 == 0 and FIQEn == 0,
> > Table 2-2 requires IRQBypDisGrp1 to be set for the IRQ output to be
> > deasserted. Similarly, Table 2-3 requires both FIQBypDisGrp0 and
> > FIQBypDisGrp1 to be set for the FIQ output to be deasserted. This is why
> > the common-view case disables the bypass paths for both groups.
> >
> > This is also not meant to make FIQ a supported delivery mode for Xen. It
> > is the opposite: when the CPU interface is disabled, the final state
> > should not allow the physical FIQ output to be driven by the legacy
> > bypass input. Arm32 has some fallback plumbing for FIQ exceptions, but Xen
> > does not configure FIQ as its normal GICv2 interrupt delivery mode.
> >
> > So the intent is:
> >
> > * with Security Extensions, touch only the Non-secure view bits visible
> > to Xen;
> > * without Security Extensions, preserve the old "no normal delivery"
> > shutdown behaviour, while changing the bypass-disable bits so that
> > the physical outputs are deasserted rather than falling back to
> > legacy bypass.
> >
> > If you prefer, I can also make v3 more conservative and only clear the
> > group-enable bit that Xen currently sets in gicv2_cpu_init(), i.e.
> > EnableGrp0 in the common view / EnableGrp1 in the Non-secure view. The
> > bypass-disable bits would still be set for all bypass paths visible in
> > the current GICC_CTLR view, because that part is about the physical
> > IRQ/FIQ outputs after normal delivery has been disabled, not about which
> > interrupt group Xen normally uses.
> The bypass argument is valid according to the tables. However, clearing G1 is
> unnecessary, so it should be dropped. I can do that on commit if you agree. With
> that:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Yes, I agree. Dropping the unnecessary clearing of G1 on commit is fine with me.
Thank you for the review and for taking care of the commit.
Best regards,
Mykola
>
> ~Michal
>
Hi Mykola,
> On 28 Apr 2026, at 12:57, 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 the architected bypass-disable
> bits 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, then setting the bypass-disable bits and
> clearing the group-enable bits that are architecturally defined for the
> current GICC_CTLR view before writing the value back. When Security
> Extensions are implemented Xen accesses the Non-secure copy of
> GICC_CTLR, where IRQBypDisGrp1 and FIQBypDisGrp1 are at bits [6:5] and
> bits [8:7] are reserved.
>
> Without Security Extensions there is no separate Secure/Non-secure CPU
> interface view, so disabling both group-enable bits affects the shared
> interface state. This is still appropriate for the CPU shutdown path,
> which is expected to stop normal interrupt delivery through the interface
> and rely only on the architecturally separate wakeup event signaling.
>
> 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>
Looks good to me
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Cheers,
Luca
© 2016 - 2026 Red Hat, Inc.