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 <jul...@xen.org> writes:
>>
>>> Hi,
>>>
>>> On 21/08/2025 16: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.
>>>
>>> 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_stras...@epam.com
>
> 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
