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

[Julien Grall <julien@xxxxxxx>]

Hi Leonid,

On 03/09/2025 15:29, Leonid Komarianskyi wrote:
> ---
>   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);
Can we use is_espi()?

> +    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);
Regardless what Volodymyr mentioned about the assert!(), I am a bit 
unsure where we guarantee is_espi() will not be called with an irq <= 
ESPI_BASE_INTID. In fact, we could have the following code in Xen:
if (is_espi(irq))
{
}
else if (is_lpi(irq))
{
}
else
{
}

We could replace the check with "!(irq >= ESPI_BASE_INTID && irq <= 
ESPI_MAX_INTID)". But I would actually prefer if there is no check 
because I don't see the value.
> +    return false;
> +#endif
> +}
> +
Cheers,

--
Julien Grall