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

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

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

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

~Michal