Re: [PATCH 02/10] xen/arm/irq: add handling for IRQs in the eSPI range

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hi Julien,

Thank you for your close review and comments.

On 29.07.25 16:50, Julien Grall wrote:
> Hi Leonid,
> 
> On 24/07/2025 15:57, Leonid Komarianskyi wrote:
>> 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>
>> ---
>>   xen/arch/arm/Kconfig           |  9 +++++++++
>>   xen/arch/arm/include/asm/irq.h | 25 +++++++++++++++++++++++++
>>   xen/arch/arm/irq.c             | 30 ++++++++++++++++++++++++++++++
>>   3 files changed, 64 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..d621f17f10 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_BASE_INTID + NR_IRQS)
>> +#else
>>   #define nr_static_irqs NR_IRQS
>> +#endif
> 
> This seems a bit odd. NR_IRQS is supposed to indicate the last SPIs ID. 
> This sort of works because in both case we have 1024. But this is really 
> fragile. So I thin nr_irqs needs to be set to ESPI_MAX_INTID. ...

Yes, you're right, I will update it to (ESPI_MAX_INTID + 1). The +1 is 
needed to cover IRQ number 5119, as this define is used in flask/hooks.c 
and expects that IRQ number will be less than nr_static_irqs:

static int get_irq_sid(int irq, uint32_t *sid, struct avc_audit_data *ad)
{
     if ( irq >= nr_irqs || irq < 0 )
         return -EINVAL;
     if ( irq < nr_static_irqs ) {
         if (ad) {
             AVC_AUDIT_DATA_INIT(ad, IRQ);
             ad->irq = irq;
         }
         return security_irq_sid(irq, sid);
     }
....

or:
static int cf_check flask_map_domain_irq(
     struct domain *d, int irq, const void *data)
{
     uint32_t sid, dsid;
     int rc = -EPERM;
     struct avc_audit_data ad;
     uint32_t dperm = flask_iommu_resource_use_perm(d);

     if ( irq >= nr_static_irqs && data )
         rc = flask_map_domain_msi(d, irq, data, &sid, &ad);
....

> Same ...
>>   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 03fbb90c6c..3f68257fde 100644
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -19,7 +19,15 @@
>>   #include <asm/gic.h>
>>   #include <asm/vgic.h>
>> +#ifdef CONFIG_GICV3_ESPI
>> +/*
>> + * To operate with IRQs in the eSPI range (4096-5119),
>> + * we need to add the eSPI base interrupt ID.
>> + */
>  > +const unsigned int nr_irqs = ESPI_BASE_INTID + NR_IRQS;
> 
> 
> ... here.
> 
>> +#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 +54,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];
>> +#endif
> 
> Why do we have to introduce a new array? Can't we use irq_desc?...

I added the new array because reusing the existing array would leave us 
with two options:

1) Extending the existing array to ESPI_MAX_INTID: It is a bad idea,
because vGIC3.1 has reserved range for INTIDs. This is not obvious when 
working with regular SPIs, as their INTIDs range from 32 to 1019. But 
for eSPIs, INTIDs start at 4096, while the range from 1024 to 4095 is 
reserved. Extending the existing array to ESPI_MAX_INTID would 
unnecessarily add over 3000 irq_desc_t entries that would never be used.

2) Extend the existing array to exact 1024 more elements: This avoids 
unused entries but complicates accessing irq_desc. In this case we would 
require in __irq_to_desc to make additional calculation for index.
In the current version:

     return &espi_desc[ESPI_INTID2IDX(irq)];

Here, it the macro simply subtracts ESPI_BASE_INTID from the given IRQ 
INTID. But while reusing the same array would look like this:

     return &irq_desc[ESPI_INTID2IDX(irq) + NR_IRQS - NR_LOCAL_IRQS];

Since irq_to_desc (which means __irq_to_desc), is called from many 
places, I decided to introduce a second array to minimize runtime 
calculations when accessing irq_desc_t elements.

This is purely an optimization, though. If you think option 2 would be 
better, I will update the code in the v2.

Cheers,
Leonid