Re: [PATCH v7 05/12] xen/arm: gicv3: implement handling of GICv3.1 eSPI

[Julien Grall <julien@xxxxxxx>]

Hi Leonid,

On 05/09/2025 11:27, Leonid Komarianskyi wrote:
> On 05.09.25 10:22, Julien Grall wrote:
> > Hi Leonid,
> >
> > On 04/09/2025 21:01, Leonid Komarianskyi wrote:
> > > +#ifdef CONFIG_GICV3_ESPI
> > > +unsigned int gic_number_espis(void)
> > > +{
> > > +    return gic_hw_ops->info->nr_espi;
> > > +}
> > > +
> > > +static void __init gicv3_dist_espi_common_init(uint32_t type)
> > > +{
> > > +    unsigned int espi_nr, i;
> > > +
> > > +    espi_nr = min(1024U, GICD_TYPER_ESPIS_NUM(type));
> > > +    gicv3_info.nr_espi = espi_nr;
> > > +    /* The GIC HW doesn't support eSPI, so we can leave from here */
> > > +    if ( gicv3_info.nr_espi == 0 )
> > > +        return;
> > > +
> > > +    printk("GICv3: %d eSPI lines\n", gicv3_info.nr_espi);
> > Style: nr_espi is unsigned. So it should be %u. Can be fixed on commit
> > if there is nothing else major to change.
> >
> > > +
> > > +    /* The configuration for eSPIs is similar to that for regular
> > > SPIs */
> > > +    for ( i = 0; i < espi_nr; i += 16 )
> > > +        writel_relaxed(0, GICD + GICD_ICFGRnE + (i / 16) * 4);
> > > +
> > > +    for ( i = 0; i < espi_nr; i += 4 )
> > > +        writel_relaxed(GIC_PRI_IRQ_ALL,
> > > +                       GICD + GICD_IPRIORITYRnE + (i / 4) * 4);
> > > +
> > > +    for ( i = 0; i < espi_nr; i += 32 )
> > > +    {
> > > +        writel_relaxed(GENMASK(31, 0), GICD + GICD_ICENABLERnE + (i /
> > > 32) * 4);
> > Sorry I only spotted now.
> >
> > The goal of gicv3_dist_espi_common_init() is to make sure the GIC is in
> > a sane state for Xen (the previous OS or firmware may have left some
> > state). Now the firmware may decide to use eSPI but not Xen (e.g.
> > because CONFIG_ESPI=n).
> >
> > This would means we may have eSPI interrupts that may be enabled and
> > pending. So as soon as we re-enable the GIC we may receive interrupts we
> > can't handle. So I think we may need to initialize the eSPI part of the
> > distributor unconditionally. What do you think?
> >
> >
> > This could be handled as a follow-up but within the timeframe of Xen
> > 4.21. So for this patch:
> >
> > Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > Cheers,
> Yes, that's a really good point about firmware initialization - I did
> not think about that. In that case, we just need to move the nr_espi
> field out of the ifdef, remove the stubs for
> gicv3_dist_espi_common_init() and gicv3_dist_espi_init_aff(), and remove
> the ifdef for these functions. The verifications at the beginning of
> gicv3_dist_espi_common_init() should work correctly, simply returning 0
> on platforms without eSPI.
I don't think we need all of these. We just need to ensure the 
interrupts are disabled and deactivated. So no need to reset the 
affinity or even moving the field moving the field out of the ifdef.
Cheers,

--
Julien Grall