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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

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.

>
> > 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.
> >
>
> This is a fair point, but I think it is better to clarify with the Arm
> maintainers first whether the SPI/eSPI initialization order should be
> changed in a separate preparatory patch, as the code for this patch
> depends on the answer. As mentioned above, I could not find such a

My suggestion to skip the extra eSPI configuration when
CONFIG_GICV3_ESPI=n was just a minor cleanup suggestion. Those writes
look unnecessary, but I don't see any harm in keeping them. There is
no need to change this in the current patch. It can be left as is or
cleaned up later.

Reviewed-by: Mykola Kvach <mykola_kvach@xxxxxxxx>

> restriction for eSPIs; it applies only to regular SPIs. If the
> maintainers agree to change the sequence for both SPIs and eSPIs, I can
> prepare a separate patch and update this patch accordingly.
>
>
> Best regards,
> Leonid.

Best regards,
Mykola