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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

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.

> +
>          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.

> +{
> +    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().

> +    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.

Otherwsie, LGTM.

~Michal