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

[Mykola Kvach <mykola_kvach@xxxxxxxx>]

Hi Volodymyr,

Thank you for the review.

On Tue, Aug 25, 2026 at 03:24:44AM +0300, Volodymyr Babchuk wrote:
> Hi Mykola,
> 
> Mykola Kvach <mykola_kvach@xxxxxxxx> writes:
> 
> > 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.
> 
> Probably you want to reword this part of the commit message. I think you
> wanted to say that "assertion fails when an eSPI INTID is passed to a
> build without eSPI support".

Sure, I'll make the assertion failure explicit in the wording.

> 
> > This makes a range predicate carry configuration policy and
> > causes callers to depend on its hidden side effects.
> 
> I'm not sure that I got this.

I meant that is_espi() does more than check the INTID range.
With CONFIG_GICV3_ESPI=n, it returns false and asserts if an
eSPI INTID is passed. The callers rely on these checks too.

I will explain this directly in the commit message.

> 
> >
> > 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));
> 
> I am not sure that it is a good idea to put ASSERT on value that we got
> from external source. What if Xen is build without CONFIG_GICV3_ESPI but
> hardware really reports an eSPI?

This assertion already exists inside is_espi(). It was added
by commit 98f7060b9ed5 ("xen/arm/irq: add handling for IRQs in
the eSPI range").

Before this patch, gic_interrupt() already called is_espi()
with the INTID read from hardware. If the hardware reported
an eSPI with CONFIG_GICV3_ESPI=n, that assertion would already
fail. I moved the check to the caller to keep the same behavior
while making is_espi() only check the INTID range.

Leonid explained the reason for the assertion in [1]. Without
eSPI support, an eSPI INTID could lead to an access outside the
regular irq_desc[] array.

The assertion was also present in v7 of the original series,
which you reviewed in [2]. We can discuss changing how this
case is handled, but my aim here was to keep the existing
behavior.

> 
> > +
> > +        if ( likely(irq >= GIC_SGI_STATIC_MAX && irq < 1020) ||
> > +             (IS_ENABLED(CONFIG_GICV3_ESPI) && is_espi(irq)) )
> >          {
> >              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) )
> >      {
> >          unsigned int nr_spis = d->arch.vgic.nr_spis;
> >  
> > @@ -949,4 +950,3 @@ void vgic_check_inflight_irqs_pending(struct vcpu *v, 
> > unsigned int rank, uint32_
> >   * indent-tabs-mode: nil
> >   * End:
> >   */
> > -
> 
> Please refrain from unneeded changes.

Ack.

Best regards,
Mykola

[1] https://lists.xenproject.org/archives/html/xen-devel/2025-09/msg00380.html
[2] https://lists.xenproject.org/archives/html/xen-devel/2025-09/msg00422.html