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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Mykola,

Mykola Kvach <mykola_kvach@xxxxxxxx> writes:

> 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. Treat virtual eSPI lookup
> without support as unreachable, with a NULL return from the
> espi_to_pending() stub.
>
> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>

> ---
> Changes in v5:
> - Explain why an eSPI cannot be handled without compiled-in support.
> - Make both espi_to_pending() helpers static inline and constify d.
> - Add ASSERT_UNREACHABLE() to the espi_to_pending() stub.
> - Preserve the unmapped LPI comment and clarify that an eSPI lookup
>   without support must not occur.
>
> 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             |  6 ++++++
>  xen/arch/arm/include/asm/irq.h | 11 -----------
>  xen/arch/arm/vgic.c            | 30 ++++++++++++++++++------------
>  3 files changed, 24 insertions(+), 23 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 078049e741..cdae6afb07 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -348,6 +348,12 @@ void gic_interrupt(struct cpu_user_regs *regs, int 
> is_fiq)
>          /* Reading IRQ will ACK it */
>          irq = gic_hw_ops->read_irq();
>  
> +        /*
> +         * Without CONFIG_GICV3_ESPI, there is no IRQ descriptor or
> +         * pending_irq storage for eSPIs, so we cannot handle them.
> +         */
> +        BUG_ON(!IS_ENABLED(CONFIG_GICV3_ESPI) && is_espi(irq));
> +
>          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..0ba13e18da 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -62,6 +62,14 @@ 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 inline struct pending_irq *espi_to_pending(const struct domain *d,
> +                                                  unsigned int irq)
> +{
> +    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 +86,13 @@ static inline struct vgic_irq_rank 
> *vgic_get_espi_rank(struct vcpu *v,
>      ASSERT_UNREACHABLE();
>      return NULL;
>  }
> +
> +static inline struct pending_irq *espi_to_pending(const struct domain *d,
> +                                                  unsigned int irq)
> +{
> +    ASSERT_UNREACHABLE();
> +    return NULL;
> +}
>  #endif
>  
>  static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
> @@ -698,6 +713,7 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum 
> gic_sgi_mode irqmode,
>   * interrupt.
>   * This can return NULL if called for an LPI which has been unmapped
>   * meanwhile.
> + * This must not be called for an eSPI when CONFIG_GICV3_ESPI is disabled.
>   */
>  struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
>  {
> @@ -715,22 +731,12 @@ struct pending_irq *irq_to_pending(struct vcpu *v, 
> unsigned int irq)
>  
>  struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq)
>  {
> -    unsigned int idx;
> -
>      ASSERT(irq >= NR_LOCAL_IRQS);
>  
>      if ( is_espi(irq) )
> -    {
> -        unsigned int nr_spis = d->arch.vgic.nr_spis;
> +        return espi_to_pending(d, irq);
>  
> -        idx = espi_intid_to_idx(irq) + nr_spis;
> -    }
> -    else
> -    {
> -        idx = irq - NR_LOCAL_IRQS;
> -    }
> -
> -    return &d->arch.vgic.pending_irqs[idx];
> +    return &d->arch.vgic.pending_irqs[irq - NR_LOCAL_IRQS];
>  }
>  
>  void vgic_clear_pending_irqs(struct vcpu *v)

-- 
WBR, Volodymyr