Re: [PATCH v2] xen/arm: gic-v2: disable interrupt bypass on CPU shutdown

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

On Mon, May 4, 2026 at 10:19 AM Orzel, Michal <michal.orzel@xxxxxxx> 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@xxxxxxx> wrote:
> >>
> >>
> >>
> >> On 28-Apr-26 13:57, Mykola Kvach wrote:
> >>> From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >>>
> >>> 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@xxxxxxxx>
> >>>
> >>> ---
> >>> 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@xxxxxxx>

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
>