Re: [PATCH v2 04/10] xen/arm/irq: add handling for IRQs in the eSPI range

[Julien Grall <julien@xxxxxxx>]

On 21/08/2025 17:59, Volodymyr Babchuk wrote:
> Julien Grall <julien@xxxxxxx> writes:
>
> > Hi,
> >
> > On 21/08/2025 16:59, Volodymyr Babchuk wrote:
> > > Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx> writes:
> > >
> > > > Currently, Xen does not support eSPI interrupts, leading
> > > > to a data abort when such interrupts are defined in the DTS.
> > > >
> > > > This patch introduces a separate array to initialize up to
> > > > 1024 interrupt descriptors in the eSPI range and adds the
> > > > necessary defines and helper function. These changes lay the
> > > > groundwork for future implementation of full eSPI interrupt
> > > > support. As this GICv3.1 feature is not required by all vendors,
> > > > all changes are guarded by ifdefs, depending on the corresponding
> > > > Kconfig option.
> > > I don't think that it is a good idea to hide this feature under
> > > Kconfig
> > > option, as this will increase number of different build variants.
> > > I believe that runtime check for GICD_TYPER.ESPI should be
> >    sufficient,> but maintainers can correct me there.
> >
> > I haven't seen many board with ESPI available. So I think it would be
> > better if this is under a Kconfig because not everyone may want to
> > have the code.
> Probably, we can expect more in the future...
Well yes. But I was under the impression this the preferred approach. 
See the discussion about disabling 32-bit support on 64-bit:
20250723075835.3993182-1-grygorii_strashko@xxxxxxxx

 Anyways, after reviewing
> all patches in the series, I can see that code will be littered with
> #ifdef CONFIG_GICV3_ESPI, which, probably, not a good thing.
The solution is to provide wrappers to reduce the number of #ifdef. I 
haven't checked all the places.
> > [...]
> >
> > > >    struct irq_desc;
> > > >    struct irqaction;
> > > > @@ -55,6 +71,15 @@ static inline bool is_lpi(unsigned int irq)
> > > >        return irq >= LPI_OFFSET;
> > > >    }
> > > >    +static inline bool is_espi(unsigned int irq)
> > > > +{
> > > > +#ifdef CONFIG_GICV3_ESPI
> > > > +    return (irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID);
> > > > +#else
> > > > +    return false;
> > > > +#endif
> > > > +}
> > > > +
> > > >    #define domain_pirq_to_irq(d, pirq) (pirq)
> > > >      bool is_assignable_irq(unsigned int irq);
> > > > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > > > index 50e57aaea7..9bc72fbbc9 100644
> > > > --- a/xen/arch/arm/irq.c
> > > > +++ b/xen/arch/arm/irq.c
> > > > @@ -19,7 +19,11 @@
> > > >    #include <asm/gic.h>
> > > >    #include <asm/vgic.h>
> > > >    +#ifdef CONFIG_GICV3_ESPI
> > > > +const unsigned int nr_irqs = ESPI_MAX_INTID + 1;
> > > > +#else
> > > >    const unsigned int nr_irqs = NR_IRQS;
> > > > +#endif
> > > >      static unsigned int local_irqs_type[NR_LOCAL_IRQS];
> > > >    static DEFINE_SPINLOCK(local_irqs_type_lock);
> > > > @@ -46,6 +50,9 @@ void irq_end_none(struct irq_desc *irq)
> > > >    }
> > > >      static irq_desc_t irq_desc[NR_IRQS - NR_LOCAL_IRQS];
> > > > +#ifdef CONFIG_GICV3_ESPI
> > > > +static irq_desc_t espi_desc[NR_IRQS];
> > By how much will this increase the Xen binary?
> I am wondering this also. The struct is cache aligned, so it will take
> at least 128 bytes. The whole array will take at least 128kb. Not
> great, not terrible. As this should go to .bss, it should not increase
> the binary itself.
I guess "binary" was the wrong word. I was referring to the size of the 
Xen in memory. On my setup Xen is 1448kb. Here you would increase ~9% of 
resident size. This seems quite steep for a feature that is not often used.
> Maybe it is better to allocate this dynamically? I do understand that we
> want to get rid of as many dynamic allocs as possible, but maybe in this
> case it will be okay.
This is up to Leonid. I don't think this is strictly necessary in order 
to get the eSPI support. However, until this is solved CONFIG_GICV3_EPSI 
*must not* be on by default as this is done in this patch.
> As a bonus point, we can't leave this pointer
> present even if CONFIG_GICV3_ESPI=n, which will simplify some code in
> latter patches.
Did you intend to say "We can leave" rather than "we can't leave"?

Cheers,

--
Julien Grall