Re: [PATCH v3 1/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
> 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".

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

>
> 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?

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



-- 
WBR, Volodymyr