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

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hi Volodymyr and Julien,

Thank you for your review comments and for raising an important 
discussion regarding memory usage and wrappers.

On 21.08.25 20:13, Julien Grall wrote:
> 
> 
> On 21/08/2025 17:59, Volodymyr Babchuk wrote:
>> Julien Grall <julien@xxxxxxx> writes:
>>
>>> Hi,
>>>
>>> On 21/08/2025 16:59, Volodymyr Babchuk wrote:
>>>> 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.
>>>> 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...
> 
> Well yes. But I was under the impression this the preferred approach. 
> See the discussion about disabling 32-bit support on 64-bit:
> 
> 20250723075835.3993182-1-grygorii_strashko@xxxxxxxx
> 
>   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.
> 
> The solution is to provide wrappers to reduce the number of #ifdef. I 
> haven't checked all the places.

I agree with you, it's a good point to use wrappers where possible. I 
will revise all patches in the series and try to use wrappers where 
possible to reduce #ifdefs.

>>
>>>
>>> [...]
>>>
>>>>>    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.
> 
> I guess "binary" was the wrong word. I was referring to the size of the 
> Xen in memory. On my setup Xen is 1448kb. Here you would increase ~9% of 
> resident size. This seems quite steep for a feature that is not often used.
> 
>>
>> 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.
> 
> This is up to Leonid. I don't think this is strictly necessary in order 
> to get the eSPI support. However, until this is solved CONFIG_GICV3_EPSI 
> *must not* be on by default as this is done in this patch.
> 

I will check how much time and effort are required to switch to dynamic 
allocation. If it does not take much time and does not require many 
changes, I will prepare an additional preparation patch in V3. 
Otherwise, I will disable the config by default in V3.

>> 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.
> 
> Did you intend to say "We can leave" rather than "we can't leave"?
> 
> Cheers,
> 

Best regards,
Leonid