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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

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>).

We also rely on the same requirement in gic_set_irq_type().

So shouldn't we disable/deactivate all eSPIs before programming
GICD_ICFGRnE? Linux also initializes the extended SPI range in this
order: ICENABLERnE/ICACTIVERnE first, followed by IGROUPRnE,
ICFGRnE and IPRIORITYRnE.

This issue already seems to exist for the CONFIG_GICV3_ESPI=y path,
but this patch makes it relevant to the newly added CONFIG=n path,
where an eSPI left enabled by firmware is precisely the case we are
trying to handle.

Also, we could disable/deactivate eSPIs for all builds, while keeping
the rest of the eSPI configuration under CONFIG_GICV3_ESPI. This would
avoid accessing the other eSPI registers in builds without eSPI
support, unless there is a particular reason to initialize them there.

Best regards,
Mykola