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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Hi Julien,

Julien Grall <julien@xen.org> writes:

> Hi,
>
> On 21/08/2025 16:59, Volodymyr Babchuk wrote:
>> Leonid Komarianskyi <Leonid_Komarianskyi@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.
>
> I haven't seen many board with ESPI available. So I think it would be
> better if this is under a Kconfig because not everyone may want to
> have the code.

Probably, we can expect more in the future... Anyways, after reviewing
all patches in the series, I can see that code will be littered with
#ifdef CONFIG_GICV3_ESPI, which, probably, not a good thing.

>
> [...]
>
>>>   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];
>
> By how much will this increase the Xen binary?

I am wondering this also. The struct is cache aligned, so it will take
at least 128 bytes. The whole array will take at least 128kb. Not
great, not terrible. As this should go to .bss, it should not increase
the binary itself.

Maybe it is better to allocate this dynamically? I do understand that we
want to get rid of as many dynamic allocs as possible, but maybe in this
case it will be okay. As a bonus point, we can't leave this pointer
present even if CONFIG_GICV3_ESPI=n, which will simplify some code in
latter patches.

[...]

-- 
WBR, Volodymyr