Re: [PATCH v3 1/4] xen/arm: make is_espi() a pure range predicate

[Mykola Kvach <mykola_kvach@xxxxxxxx>]

Hi Michal,

Thank you for the review.

On Fri, Sep 11, 2026 at 01:03:28PM +0200, Orzel, Michal wrote:
> 
> 
> On 18-Aug-26 13:32, Mykola Kvach wrote:
> > is_espi() currently changes its result according to CONFIG_GICV3_ESPI
> > and asserts when an eSPI INTID is passed to a build without eSPI
> > support. This makes a range predicate carry configuration policy and
> > causes callers to depend on its hidden side effects.
> > 
> > Make is_espi() report only whether an INTID is in the architectural
> > eSPI range. Gate eSPI handling explicitly at call sites and preserve
> > the debug checks on paths where an eSPI is invalid without compiled-in
> > support.
> > 
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> > Changes in v3:
> > - New preparatory cleanup requested during review.
> > ---
> >  xen/arch/arm/gic.c             |  5 ++++-
> >  xen/arch/arm/include/asm/irq.h | 11 -----------
> >  xen/arch/arm/vgic.c            |  4 ++--
> >  3 files changed, 6 insertions(+), 14 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 078049e741..075e1d2c50 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -348,7 +348,10 @@ void gic_interrupt(struct cpu_user_regs *regs, int 
> > is_fiq)
> >          /* Reading IRQ will ACK it */
> >          irq = gic_hw_ops->read_irq();
> >  
> > -        if ( likely(irq >= GIC_SGI_STATIC_MAX && irq < 1020) || 
> > is_espi(irq) )
> > +        ASSERT(IS_ENABLED(CONFIG_GICV3_ESPI) || !is_espi(irq));
> > +
> > +        if ( likely(irq >= GIC_SGI_STATIC_MAX && irq < 1020) ||
> > +             (IS_ENABLED(CONFIG_GICV3_ESPI) && is_espi(irq)) )
> Take a look at LPIs that are also protected by CONFIG option. We don't ASSERT
> because they are gone in a release build. We want to BUG() for eSPIs same as 
> for
> LPIs if we cannot continue with this condition (we haven't configured/enabled
> them, so it's impossible condition where something went wrong). Here you 
> should
> just BUG().

I will replace ASSERT with BUG_ON() and remove the extra config
check in IRQ dispatch.

> 
> >          {
> >              isb();
> >              do_IRQ(regs, irq, is_fiq);
> > diff --git a/xen/arch/arm/include/asm/irq.h b/xen/arch/arm/include/asm/irq.h
> > index 09788dbfeb..c29f3d04a3 100644
> > --- a/xen/arch/arm/include/asm/irq.h
> > +++ b/xen/arch/arm/include/asm/irq.h
> > @@ -66,18 +66,7 @@ static inline bool is_lpi(unsigned int irq)
> >  
> >  static inline bool is_espi(unsigned int irq)
> >  {
> > -#ifdef CONFIG_GICV3_ESPI
> >      return irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID;
> > -#else
> > -    /*
> > -     * The function should not be called for eSPIs when CONFIG_GICV3_ESPI 
> > is
> > -     * disabled. Returning false allows the compiler to optimize the code
> > -     * when the config is disabled, while the assert ensures that 
> > out-of-range
> > -     * array resources are not accessed.
> > -     */
> > -    ASSERT(!(irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID));
> > -    return false;
> > -#endif
> >  }
> >  
> >  static inline unsigned int espi_intid_to_idx(unsigned int intid)
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index e5aca17dcb..e14123a30a 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -718,8 +718,9 @@ struct pending_irq *spi_to_pending(struct domain *d, 
> > unsigned int irq)
> >      unsigned int idx;
> >  
> >      ASSERT(irq >= NR_LOCAL_IRQS);
> > +    ASSERT(IS_ENABLED(CONFIG_GICV3_ESPI) || !is_espi(irq));
> >  
> > -    if ( is_espi(irq) )
> > +    if ( IS_ENABLED(CONFIG_GICV3_ESPI) && is_espi(irq) )
> Following the LPIs, irq_to_pending() returns NULL if they are not supported 
> and
> we somehow ended up here. We should do the same here without using IS_ENABLED
> and ASSERT. Note though that for that, some call sites need to be enabled not 
> to
> dereference NULL.

I will add an espi_to_pending() stub that returns NULL when eSPI
support is disabled.

I checked the callers. They already handle NULL, check the IRQ range
or register rank, or use allocated IRQs. I did not find a need for
extra NULL checks.

I tested the proposed change on QEMU and FVP with eSPI, ITS and
debug support on and off.

Best regards,
Mykola