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

[Mykola Kvach <mykola_kvach@xxxxxxxx>]

Hi Michal,

Thank you for the review.

On Tue, Sep 22, 2026 at 01:19:57PM +0200, Orzel, Michal wrote:
> 
> 
> On 22-Sep-26 08:39, Mykola Kvach wrote:
> > is_espi() currently changes its result according to CONFIG_GICV3_ESPI.
> > Without eSPI support, it returns false, and its assertion fails if
> > an eSPI INTID is passed. Callers therefore use it both to identify
> > eSPIs and to exclude eSPI handling when support is disabled.
> > 
> > Make is_espi() report only whether an INTID is in the architectural
> > eSPI range. Check for eSPI support at the call sites.
> > 
> > Use BUG_ON() if the GIC reports an eSPI without compiled-in support,
> > matching the handling of unsupported LPIs. Return NULL from
> > spi_to_pending() for an eSPI when support is disabled, using an
> > espi_to_pending() stub.
> > 
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> > Changes in v4:
> > - Clarify the existing is_espi() behavior in the commit message.
> > - Drop the unrelated blank-line removal in vgic.c.
> > - Use BUG_ON() if the GIC reports an eSPI without eSPI support.
> > - Drop the redundant CONFIG_GICV3_ESPI check in IRQ dispatch.
> > - Return NULL for virtual eSPI lookup when eSPI support is disabled.
> > 
> > Changes in v3:
> > - New preparatory cleanup requested during review.
> > ---
> >  xen/arch/arm/gic.c             |  2 ++
> >  xen/arch/arm/include/asm/irq.h | 11 -----------
> >  xen/arch/arm/vgic.c            | 30 ++++++++++++++++--------------
> >  3 files changed, 18 insertions(+), 25 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 078049e741..3a7f972826 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -348,6 +348,8 @@ void gic_interrupt(struct cpu_user_regs *regs, int 
> > is_fiq)
> >          /* Reading IRQ will ACK it */
> >          irq = gic_hw_ops->read_irq();
> >  
> > +        BUG_ON(!IS_ENABLED(CONFIG_GICV3_ESPI) && is_espi(irq));
> Usually BUG() should come with a comment explaining why we cannot continue. 
> Here
> it's that without config enabled, there's no descriptor and no pending_irq
> storage for it. Please add a comment.

Ack.

> 
> > +
> >          if ( likely(irq >= GIC_SGI_STATIC_MAX && irq < 1020) || 
> > is_espi(irq) )
> >          {
> >              isb();
> > 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..e04678f134 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -62,6 +62,13 @@ static inline struct vgic_irq_rank 
> > *vgic_get_espi_rank(struct vcpu *v,
> >      return &v->domain->arch.vgic.ext_shared_irqs[EXT_RANK_NUM2IDX(rank)];
> >  }
> >  
> > +static struct pending_irq *espi_to_pending(struct domain *d, unsigned int 
> > irq)
> Constify d, please.
> Also, use static inline like the surrounding helpers.

Ack.

> 
> > +{
> > +    unsigned int idx = espi_intid_to_idx(irq) + d->arch.vgic.nr_spis;
> > +
> > +    return &d->arch.vgic.pending_irqs[idx];
> > +}
> > +
> >  #else
> >  static inline bool is_valid_espi_rank(struct domain *d, unsigned int rank)
> >  {
> > @@ -78,6 +85,11 @@ static inline struct vgic_irq_rank 
> > *vgic_get_espi_rank(struct vcpu *v,
> >      ASSERT_UNREACHABLE();
> >      return NULL;
> >  }
> > +
> > +static struct pending_irq *espi_to_pending(struct domain *d, unsigned int 
> > irq)
> > +{
> Add ASSERT_UNREACHABLE() here like for vgic_get_espi_rank().

Ack.

> 
> > +    return NULL;
> > +}
> >  #endif
> >  
> >  static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
> > @@ -696,8 +708,8 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum 
> > gic_sgi_mode irqmode,
> >  /*
> >   * Returns the pointer to the struct pending_irq belonging to the given
> >   * interrupt.
> > - * This can return NULL if called for an LPI which has been unmapped
> > - * meanwhile.
> > + * This can return NULL for an eSPI when support is disabled, or for an
> > + * LPI which has been unmapped meanwhile.
> This puts LPI and eSPIs on exactly the same level which is not correct. While
> unmapped LPI can happen, eSPI with support disabled must not. Leave the LPI
> comment as is and clarify eSPI in one sentence.

Ack.

Best regards,
Mykola