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

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hello Oleksandr and Volodymyr,

Thank you for your comments.

On 31.08.25 17:08, Oleksandr Tyshchenko wrote:
> 
> 
> On 29.08.25 22:45, Volodymyr Babchuk wrote:
>>
>> Hi Leonid,
> 
> Hello 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 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
> 
> 
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> 
> with the following addressed ...
> 
> 
>>>
>>> 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 | 24 +++++++++++++++
>>>   xen/arch/arm/irq.c             | 56 +++++++++++++++++++++++++++++++++-
>>>   3 files changed, 87 insertions(+), 1 deletion(-)
>>>
>>> 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..4443799648 100644
>>> --- a/xen/arch/arm/include/asm/irq.h
>>> +++ b/xen/arch/arm/include/asm/irq.h
>>> @@ -32,6 +32,13 @@ 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
>>> +
>>> +#define ESPI_INTID2IDX(intid) ((intid) - ESPI_BASE_INTID)
>>> +#define ESPI_IDX2INTID(idx)   ((idx) + ESPI_BASE_INTID)
>>> +
>>>   /* LPIs are always numbered starting at 8192, so 0 is a good 
>>> invalid case. */
>>>   #define INVALID_LPI     0
>>> @@ -39,7 +46,15 @@ struct arch_irq_desc {
>>>   #define INVALID_IRQ     1023
>>>   extern const unsigned int nr_irqs;
>>> +#ifdef CONFIG_GICV3_ESPI
>>> +/*
>>> + * This will also cover the eSPI range, as some critical devices
>>> + * for booting Xen (e.g., serial) may use this type of interrupts.
>>> + */
>>> +#define nr_static_irqs (ESPI_MAX_INTID + 1)
>>> +#else
>>>   #define nr_static_irqs NR_IRQS
>>> +#endif
>>>   struct irq_desc;
>>>   struct irqaction;
>>> @@ -55,6 +70,15 @@ static inline bool is_lpi(unsigned int irq)
>>>       return irq >= LPI_OFFSET;
>>>   }
>>> +static inline bool is_espi(unsigned int irq)
>>> +{
>>> +#ifdef CONFIG_GICV3_ESPI
>>
>> Taking into account that with CONFIG_GICV3_ESPI=n we should never have
>> "irq" in eSPI range, do you really need this #ifdef? I think that
>> ASSERT_UNREACHABLE in espi_to_desc() is sufficient guard.
>>
>> Also, IRQ line number belongs to eSPI range regardless of 
>> CONFIG_GICV3_ESPI,
>> value, so in my opinion is_espi() should always return correct value for
>> a given "irq".
> 
>   ... I agree with Volodymyr's suggestion for is_espi() to always return 
> correct value for a given "irq".
> 
> 

I will fix that in V6.

>>
>>> +    return (irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID);
>>
>> Also, you don't need parentheses here.

I will remove the redundant parentheses in V6 as well.

>>
>>> +#else
>>> +    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..61c915c3f9 100644
>>> --- a/xen/arch/arm/irq.c
>>> +++ b/xen/arch/arm/irq.c
>>> @@ -19,7 +19,11 @@
>>>   #include <asm/gic.h>
>>>   #include <asm/vgic.h>
>>> +#ifdef CONFIG_GICV3_ESPI
>>> +const unsigned int nr_irqs = ESPI_MAX_INTID + 1;
>>> +#else
>>>   const unsigned int nr_irqs = NR_IRQS;
>>> +#endif
>>>   static unsigned int local_irqs_type[NR_LOCAL_IRQS];
>>>   static DEFINE_SPINLOCK(local_irqs_type_lock);
>>> @@ -46,6 +50,53 @@ 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 if
>>> + * there is a need to enable GICV3_ESPI by default.
>>> + */
>>> +static irq_desc_t espi_desc[NR_ESPI_IRQS];
>>> +
>>> +static struct irq_desc *espi_to_desc(unsigned int irq)
>>> +{
>>> +    return &espi_desc[ESPI_INTID2IDX(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
>>> +/*
>>> + * This function is stub and will not be called if CONFIG_GICV3_ESPI=n,
>>> + * because in this case, is_espi will always return false.
> 
>   This comment should also be updated.
> 

Sure, I will update the comment accordingly.

>>> + */
>>> +static struct irq_desc *espi_to_desc(unsigned int irq)
>>> +{
>>> +    ASSERT_UNREACHABLE();
>>> +    return NULL;
>>> +}
>>> +
>>> +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 +104,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 +133,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)
>>
> 

Best regards,
Leonid