Re: [PATCH v2] xen/arm: gicv3: initialize eSPI unconditionally

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

On 29-Sep-26 17:14, Mykola Kvach wrote:
> Hi Leonid,
> 
> On Wed, Sep 23, 2026 at 8:55 PM Leonid Komarianskyi
> <Leonid_Komarianskyi@xxxxxxxx> wrote:
>>
>> Hello Mykola,
>>
>> Thank you for your review.
>>
>> On 9/23/26 11:04, Mykola Kvach wrote:
>>> Hi Leonid,
>>>
>>> Thank you for the patch.
>>>
>>> On Tue, Sep 22, 2026 at 9:59 PM Leonid Komarianskyi
>>> <Leonid_Komarianskyi@xxxxxxxx> wrote:
>>>>
>>>> Since the firmware may initialize eSPIs before Xen, and without
>>>> CONFIG_GICV3_ESPI enabled, Xen would not reinitialize them properly
>>>> during boot. In such cases, once the GIC is re-enabled in Xen,
>>>> interrupts may be received that cannot be handled.
>>>>
>>>> To ensure proper operation on hardware with eSPI feature, even when the 
>>>> eSPI
>>>> config is disabled, gicv3_dist_espi_common_init() should be invoked
>>>> regardless of whether CONFIG_GICV3_ESPI is enabled or not. This will not
>>>> affect hardware without eSPI support, as the function checks if the
>>>> hardware supports eSPIs by reading the GICD_TYPER.ESPI field (using
>>>> GICD_TYPER_ESPIS_NUM macro), which indicates whether the extended SPI
>>>> range is supported. If the hardware does not support eSPI, the function
>>>> will not perform any actions.
>>>>
>>>> There are no functional changes for setups where CONFIG_GICV3_ESPI=y.
>>>>
>>>> Suggested-by: Julien Grall <jgrall@xxxxxxxxxx>
>>>> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@xxxxxxxx>
>>>> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
>>>> ---
>>>> Changes in v2:
>>>> - rebased on the current staging
>>>> - placed Suggested-by tag first to keep tags in chronological order
>>>> - added Acked-by from Julien Grall
>>>>
>>>> This is a follow-up patch related to the discussion:
>>>> https://lore.kernel.org/xen-devel/820704d0-4047-4f02-a058-01daba2765f1@xxxxxxx/
>>>>
>>>> Sending v2 with the requested changes, as I only now noticed
>>>> that this patch has not been merged yet.
>>>> ---
>>>>   xen/arch/arm/gic-v3.c                  | 32 ++++++++++++++------------
>>>>   xen/arch/arm/include/asm/gic_v3_defs.h |  2 --
>>>>   2 files changed, 17 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>>>> index acdac22953..463769d77b 100644
>>>> --- a/xen/arch/arm/gic-v3.c
>>>> +++ b/xen/arch/arm/gic-v3.c
>>>> @@ -703,17 +703,32 @@ unsigned int gic_number_espis(void)
>>>>       return gic_hw_ops->info->nr_espi;
>>>>   }
>>>>
>>>> +static void __init gicv3_dist_espi_init_aff(uint64_t affinity)
>>>> +{
>>>> +    unsigned int i;
>>>> +
>>>> +    for ( i = 0; i < gicv3_info.nr_espi; i++ )
>>>> +        writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTERnE + i * 
>>>> 8);
>>>> +}
>>>> +#else
>>>> +
>>>> +static void __init gicv3_dist_espi_init_aff(uint64_t affinity) { }
>>>> +#endif
>>>> +
>>>>   static void __init gicv3_dist_espi_common_init(uint32_t type)
>>>
>>> I think the ordering in gicv3_dist_espi_common_init() needs to be
>>> revisited now that this function is also called with
>>> CONFIG_GICV3_ESPI=n.
>>>
>>> The motivation for this patch is that firmware may have left an eSPI
>>> enabled. However, we currently program GICD_ICFGRnE before clearing
>>> the corresponding enable bit in GICD_ICENABLERnE.
>>>
>>> The GIC architecture requires an interrupt to be individually disabled
>>> before changing Int_config; otherwise the behavior is UNPREDICTABLE.
>>> See Arm IHI 0069H.b, section 12.9.9 (GICD_ICFGR<n>).
>>>
>>
>> Section 12.9.9 describes GICD_ICFGR<n>, i.e. the regular SPI range, and
>> it indeed contains this requirement. However, the code in
>> gicv3_dist_espi_common_init() programs GICD_ICFGR<n>E, which is
>> described in 12.9.10, and I could not find an equivalent requirement there.
> 
> Thanks for checking. You're right, the eSPI section does not state
> the same requirement. Sorry for the confusion.
> 
>>
>> The only related rule I found that also covers the extended SPI range is
>> in Arm IHI 0069H.b section 4.5:
>>
>> "Changing the configuration of an interrupt from level-sensitive to
>> edge-triggered, or from edge-triggered to level-sensitive, when there is
>> a pending interrupt, leaves the interrupt in an UNKNOWN state."
>>
>> That rule is about the pending state rather than the enable state, though.
>>
>> Also, the current eSPI initialization sequence mirrors the one used for
>> regular SPIs in gicv3_dist_init(), where the requirement from 12.9.9
>> does apply. So if we decide to reorder the initialization, I think it
>> should be done for regular SPIs as well, ideally in a separate
>> preparatory patch.
> 
> For regular SPIs, let's wait for the maintainers' feedback. I think
> any fix there should be a separate patch and should not block this
> one.
I agree. That's the pre-existing issue and can be fixed in a follow-up patch.

~Michal