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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

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>

~Michal