Hi Volodymyr,
Thank you for your review.
On 21.08.25 18:59, Volodymyr Babchuk wrote:
> Leonid Komarianskyi <leonid_komarians...@epam.com> 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.
>
> I don't think that it is a good idea to hide this feature under Kconfig
> option, as this will increase number of different build variants.
> I believe that runtime check for GICD_TYPER.ESPI should be sufficient,
> but maintainers can correct me there.
>
>>
>> Signed-off-by: Leonid Komarianskyi <leonid_komarians...@epam.com>
>>
>> ---
>> 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 | 9 +++++++++
>> xen/arch/arm/include/asm/irq.h | 25 +++++++++++++++++++++++++
>> xen/arch/arm/irq.c | 26 ++++++++++++++++++++++++++
>> 3 files changed, 60 insertions(+)
>>
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index 17df147b25..08073ece1f 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -135,6 +135,15 @@ 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
>> + default y
>> + 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..acebc3d42f 100644
>> --- a/xen/arch/arm/include/asm/irq.h
>> +++ b/xen/arch/arm/include/asm/irq.h
>> @@ -32,6 +32,14 @@ struct arch_irq_desc {
>> #define SPI_MAX_INTID 1019
>> #define LPI_OFFSET 8192
>>
>> +#ifdef CONFIG_GICV3_ESPI
>> +#define ESPI_BASE_INTID 4096
>> +#define ESPI_MAX_INTID 5119
>> +
>> +#define ESPI_INTID2IDX(intid) ((intid) - ESPI_BASE_INTID)
>> +#define ESPI_IDX2INTID(idx) ((idx) + ESPI_BASE_INTID)
>> +#endif
>> +
>> /* LPIs are always numbered starting at 8192, so 0 is a good invalid case.
>> */
>> #define INVALID_LPI 0
>>
>> @@ -39,7 +47,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
>
> Don't introduce defines that look like variables. I am sure that MISRA
> team will be unhappy about that. But what you can really do is to
> introduce variable nr_static_irqs, which value will depend on
> GICD_TYPER.ESPI and GICD_TYPER.ESPI_range
>
This is a generic define, which is present in all arch-specific irq
headers (x86, PPC, RISC-V, ARM). I think it is not a good idea to make
this a const variable only when CONFIG_GICV3_ESPI is enabled, while in
other places it is a define. Moreover, I clarified with one of our
teammates, who is engaged in MISRA fixes for Xen, and he confirmed that
there is no violation of the MISRA rule here. To summarize all of that,
would it be okay to leave this place unchanged?
>>
>> struct irq_desc;
>> struct irqaction;
>> @@ -55,6 +71,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
>> + return (irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID);
>> +#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 50e57aaea7..9bc72fbbc9 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,9 @@ void irq_end_none(struct irq_desc *irq)
>> }
>>
>> static irq_desc_t irq_desc[NR_IRQS - NR_LOCAL_IRQS];
>> +#ifdef CONFIG_GICV3_ESPI
>> +static irq_desc_t espi_desc[NR_IRQS];
>
> This is really confusing. Should it be something like espi_desc[NR_ESPI_IRQS]?
>
Sure, that would be better. So, I will add the NR_ESPI_IRQS define in
V3. Of course, if I am not able to make a dynamic allocation, which is
being discussed in the sub-thread for this patch. :)
>> +#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 +60,11 @@ struct irq_desc *__irq_to_desc(unsigned int irq)
>> if ( irq < NR_LOCAL_IRQS )
>> return &this_cpu(local_irq_desc)[irq];
>>
>> +#ifdef CONFIG_GICV3_ESPI
>> + if ( is_espi(irq) )
>> + return &espi_desc[ESPI_INTID2IDX(irq)];
>> +#endif
>> +
>> return &irq_desc[irq-NR_LOCAL_IRQS];
>> }
>>
>> @@ -79,6 +91,20 @@ static int __init init_irq_data(void)
>> desc->action = NULL;
>> }
>>
>> +#ifdef CONFIG_GICV3_ESPI
>> + 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;
>> + }
>> +#endif
>> +
>> return 0;
>> }
>
Best regards,
Leonid
