Re: [PATCH v6 04/12] xen/arm/irq: add handling for IRQs in the eSPI range

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Leonid,

Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx> writes:

> Currently, Xen does not support eSPI interrupts, leading
> to a data abort when such interrupts are defined in the DTS.
>
> This patch introduces a separate array to initialize up to
> 1024 interrupt descriptors in the eSPI range and adds the
> necessary defines and helper function. These changes lay the
> groundwork for future implementation of full eSPI interrupt
> support. As this GICv3.1 feature is not required by all vendors,
> all changes are guarded by ifdefs, depending on the corresponding
> Kconfig option.
>
> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@xxxxxxxx>
>
> ---
> Changes in V6:
> - added an assert in is_espi() when CONFIG_GICV3_ESPI=n to ensure that
>   out-of-range array resources are not accessed, e.g., in __irq_to_desc()
> - removed unnecessary parentheses in is_espi()
> - converted helper macro to inline functions and added sanity checks
>   with ASSERTs to them
> - defined espi_to_desc for non-eSPI builds as a prototype
> - updates the comments
> - used the IS_ENABLED(CONFIG_GICV3_ESPI) macro to initialize nr_irqs
>
> Changes in V5:
> - no functional changes introduced by this version compared with V4, only
>   minor fixes and removal of ifdefs for macroses
> - added TODO comment, suggested by Oleksandr Tyshchenko
> - changed int to unsigned int for irqs
> - removed ifdefs for eSPI-specific defines and macros to reduce the
>   number of ifdefs and code duplication in further changes
> - removed reviewed-by as moving defines from ifdefs requires additional
>   confirmation from reviewers
>
> Changes in V4:
> - removed redundant line with 'default n' in Kconfig, as it is disabled
>   by default, without explicit specification
> - added reviewed-by from Volodymyr Babchuk
>
> Changes in V3:
> - introduced a new define NR_ESPI_IRQS to avoid confusion, like in the
>   case of using NR_IRQS for espi_desc array
> - implemented helper functions espi_to_desc and init_espi_data to make
>   it possible to add stubs with the same name, and as a result, reduce
>   the number of #ifdefs
> - disable CONFIG_GICV3_ESPI default value to n
>
> Changes in V2:
> - use (ESPI_MAX_INTID + 1) instead of (ESPI_BASE_INTID + NR_IRQS)
> - remove unnecessary comment for nr_irqs initialization
> ---
>  xen/arch/arm/Kconfig           |  8 +++++
>  xen/arch/arm/include/asm/irq.h | 37 ++++++++++++++++++++++++
>  xen/arch/arm/irq.c             | 53 ++++++++++++++++++++++++++++++++--
>  3 files changed, 96 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 17df147b25..43b05533b1 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -135,6 +135,14 @@ config GICV3
>         Driver for the ARM Generic Interrupt Controller v3.
>         If unsure, use the default setting.
>  
> +config GICV3_ESPI
> +     bool "Extended SPI range support"
> +     depends on GICV3 && !NEW_VGIC
> +     help
> +       Allow Xen and domains to use interrupt numbers from the extended SPI
> +       range, from 4096 to 5119. This feature is introduced in GICv3.1
> +       architecture.
> +
>  config HAS_ITS
>          bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
>          depends on GICV3 && !NEW_VGIC && !ARM_32
> diff --git a/xen/arch/arm/include/asm/irq.h b/xen/arch/arm/include/asm/irq.h
> index 5bc6475eb4..f4d0997651 100644
> --- a/xen/arch/arm/include/asm/irq.h
> +++ b/xen/arch/arm/include/asm/irq.h
> @@ -32,6 +32,10 @@ struct arch_irq_desc {
>  #define SPI_MAX_INTID   1019
>  #define LPI_OFFSET      8192
>  
> +#define ESPI_BASE_INTID 4096
> +#define ESPI_MAX_INTID  5119
> +#define NR_ESPI_IRQS    1024
> +
>  /* LPIs are always numbered starting at 8192, so 0 is a good invalid case. */
>  #define INVALID_LPI     0
>  
> @@ -39,7 +43,12 @@ struct arch_irq_desc {
>  #define INVALID_IRQ     1023
>  
>  extern const unsigned int nr_irqs;
> +#ifdef CONFIG_GICV3_ESPI
> +/* This will cover the eSPI range, to allow asignmant of eSPIs to domains. */
> +#define nr_static_irqs (ESPI_MAX_INTID + 1)
> +#else
>  #define nr_static_irqs NR_IRQS
> +#endif
>  
>  struct irq_desc;
>  struct irqaction;
> @@ -55,6 +64,34 @@ static inline bool is_lpi(unsigned int irq)
>      return irq >= LPI_OFFSET;
>  }
>  
> +static inline unsigned int espi_intid_to_idx(unsigned int intid)
> +{
> +    ASSERT(intid >= ESPI_BASE_INTID && intid <= ESPI_MAX_INTID);
> +    return intid - ESPI_BASE_INTID;
> +}
> +
> +static inline unsigned int espi_idx_to_intid(unsigned int idx)
> +{
> +    ASSERT(idx <= NR_ESPI_IRQS);
> +    return idx + ESPI_BASE_INTID;
> +}
> +
> +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, e.g., in __irq_to_desc().
> +     */
> +    ASSERT(irq >= ESPI_BASE_INTID);

This really puzzles me. Should it be other way around? I.e.

ASSERT(irq < ESPI_BASE_INTID) ? Or even ASSERT(irq <= 1022) ?

Actually, I tried to your series. XEN does not boots at all when
CONFIG_GICV3_ESPI=n. Looks like it panics even before it can bring up
the console, as I don't see any prints in QEMU. Non-debug build boots
fine, thought, but this is expected, as ASSERTs are disabled.


> +    return false;
> +#endif
> +}
> +
>  #define domain_pirq_to_irq(d, pirq) (pirq)
>  
>  bool is_assignable_irq(unsigned int irq);
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index b8eccfc924..c934d39bf6 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -19,7 +19,9 @@
>  #include <asm/gic.h>
>  #include <asm/vgic.h>
>  
> -const unsigned int nr_irqs = NR_IRQS;
> +const unsigned int nr_irqs = IS_ENABLED(CONFIG_GICV3_ESPI) ?
> +                                        (ESPI_MAX_INTID + 1) :
> +                                        NR_IRQS;
>  
>  static unsigned int local_irqs_type[NR_LOCAL_IRQS];
>  static DEFINE_SPINLOCK(local_irqs_type_lock);
> @@ -46,6 +48,50 @@ void irq_end_none(struct irq_desc *irq)
>  }
>  
>  static irq_desc_t irq_desc[NR_IRQS - NR_LOCAL_IRQS];
> +#ifdef CONFIG_GICV3_ESPI
> +/* TODO: Consider allocating an array dynamically */

I'd considered using radix tree, honestly... But this is just topic for
discussion, no action should be taken here.

> +static irq_desc_t espi_desc[NR_ESPI_IRQS];
> +
> +static struct irq_desc *espi_to_desc(unsigned int irq)
> +{
> +    return &espi_desc[espi_intid_to_idx(irq)];
> +}
> +
> +static int __init init_espi_data(void)
> +{
> +    unsigned int irq;
> +
> +    for ( irq = ESPI_BASE_INTID; irq <= ESPI_MAX_INTID; irq++ )
> +    {
> +        struct irq_desc *desc = irq_to_desc(irq);
> +        int rc = init_one_irq_desc(desc);
> +
> +        if ( rc )
> +            return rc;
> +
> +        desc->irq = irq;
> +        desc->action  = NULL;
> +    }
> +
> +    return 0;
> +}
> +#else
> +/*
> + * Defined as a prototype as it should not be called if CONFIG_GICV3_ESPI=n.
> + * Without CONFIG_GICV3_ESPI, the additional 1024 IRQ descriptors will not
> + * be defined, and thus, they cannot be used. Unless INTIDs from the eSPI
> + * range are mistakenly defined in Xen DTS when the appropriate config is
> + * disabled, this function will not be reached because is_espi will return
> + * false for non-eSPI INTIDs.
> + */
> +struct irq_desc *espi_to_desc(unsigned int irq);
> +
> +static int __init init_espi_data(void)
> +{
> +    return 0;
> +}
> +#endif
> +
>  static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc);
>  
>  struct irq_desc *__irq_to_desc(unsigned int irq)
> @@ -53,6 +99,9 @@ struct irq_desc *__irq_to_desc(unsigned int irq)
>      if ( irq < NR_LOCAL_IRQS )
>          return &this_cpu(local_irq_desc)[irq];
>  
> +    if ( is_espi(irq) )
> +        return espi_to_desc(irq);
> +
>      return &irq_desc[irq-NR_LOCAL_IRQS];
>  }
>  
> @@ -79,7 +128,7 @@ static int __init init_irq_data(void)
>          desc->action  = NULL;
>      }
>  
> -    return 0;
> +    return init_espi_data();
>  }
>  
>  static int init_local_irq_data(unsigned int cpu)

-- 
WBR, Volodymyr