Hello Oleksandr and Volodymyr,
Thank you for your comments.
On 31.08.25 17:08, Oleksandr Tyshchenko wrote:
>
>
> On 29.08.25 22:45, Volodymyr Babchuk wrote:
>>
>> Hi Leonid,
>
> Hello Leonid
>
>>
>> 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.
>>>
>>> Signed-off-by: Leonid Komarianskyi <leonid_komarians...@epam.com>
>>>
>>> ---
>>> Changes in V5:
>>> - no functional changes introduced by this version compared with V4,
>>> only
>>> minor fixes and removal of ifdefs for macroses
>>> - added TODO comment, suggested by Oleksandr Tyshchenko
>>> - changed int to unsigned int for irqs
>>> - removed ifdefs for eSPI-specific defines and macros to reduce the
>>> number of ifdefs and code duplication in further changes
>>> - removed reviewed-by as moving defines from ifdefs requires additional
>>> confirmation from reviewers
>
>
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
>
> with the following addressed ...
>
>
>>>
>>> Changes in V4:
>>> - removed redundant line with 'default n' in Kconfig, as it is disabled
>>> by default, without explicit specification
>>> - added reviewed-by from Volodymyr Babchuk
>>>
>>> Changes in V3:
>>> - introduced a new define NR_ESPI_IRQS to avoid confusion, like in the
>>> case of using NR_IRQS for espi_desc array
>>> - implemented helper functions espi_to_desc and init_espi_data to make
>>> it possible to add stubs with the same name, and as a result, reduce
>>> the number of #ifdefs
>>> - disable CONFIG_GICV3_ESPI default value to n
>>>
>>> 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 | 8 +++++
>>> xen/arch/arm/include/asm/irq.h | 24 +++++++++++++++
>>> xen/arch/arm/irq.c | 56 +++++++++++++++++++++++++++++++++-
>>> 3 files changed, 87 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>> index 17df147b25..43b05533b1 100644
>>> --- a/xen/arch/arm/Kconfig
>>> +++ b/xen/arch/arm/Kconfig
>>> @@ -135,6 +135,14 @@ 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
>>> + 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..4443799648 100644
>>> --- a/xen/arch/arm/include/asm/irq.h
>>> +++ b/xen/arch/arm/include/asm/irq.h
>>> @@ -32,6 +32,13 @@ struct arch_irq_desc {
>>> #define SPI_MAX_INTID 1019
>>> #define LPI_OFFSET 8192
>>> +#define ESPI_BASE_INTID 4096
>>> +#define ESPI_MAX_INTID 5119
>>> +#define NR_ESPI_IRQS 1024
>>> +
>>> +#define ESPI_INTID2IDX(intid) ((intid) - ESPI_BASE_INTID)
>>> +#define ESPI_IDX2INTID(idx) ((idx) + ESPI_BASE_INTID)
>>> +
>>> /* LPIs are always numbered starting at 8192, so 0 is a good
>>> invalid case. */
>>> #define INVALID_LPI 0
>>> @@ -39,7 +46,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
>>> struct irq_desc;
>>> struct irqaction;
>>> @@ -55,6 +70,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
>>
>> Taking into account that with CONFIG_GICV3_ESPI=n we should never have
>> "irq" in eSPI range, do you really need this #ifdef? I think that
>> ASSERT_UNREACHABLE in espi_to_desc() is sufficient guard.
>>
>> Also, IRQ line number belongs to eSPI range regardless of
>> CONFIG_GICV3_ESPI,
>> value, so in my opinion is_espi() should always return correct value for
>> a given "irq".
>
> ... I agree with Volodymyr's suggestion for is_espi() to always return
> correct value for a given "irq".
>
>
I will fix that in V6.
>>
>>> + return (irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID);
>>
>> Also, you don't need parentheses here.
I will remove the redundant parentheses in V6 as well.
>>
>>> +#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 b8eccfc924..61c915c3f9 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,53 @@ void irq_end_none(struct irq_desc *irq)
>>> }
>>> static irq_desc_t irq_desc[NR_IRQS - NR_LOCAL_IRQS];
>>> +#ifdef CONFIG_GICV3_ESPI
>>> +/*
>>> + * TODO: Consider allocating an array dynamically if
>>> + * there is a need to enable GICV3_ESPI by default.
>>> + */
>>> +static irq_desc_t espi_desc[NR_ESPI_IRQS];
>>> +
>>> +static struct irq_desc *espi_to_desc(unsigned int irq)
>>> +{
>>> + return &espi_desc[ESPI_INTID2IDX(irq)];
>>> +}
>>> +
>>> +static int __init init_espi_data(void)
>>> +{
>>> + unsigned int irq;
>>> +
>>> + 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;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +#else
>>> +/*
>>> + * This function is stub and will not be called if CONFIG_GICV3_ESPI=n,
>>> + * because in this case, is_espi will always return false.
>
> This comment should also be updated.
>
Sure, I will update the comment accordingly.
>>> + */
>>> +static struct irq_desc *espi_to_desc(unsigned int irq)
>>> +{
>>> + ASSERT_UNREACHABLE();
>>> + return NULL;
>>> +}
>>> +
>>> +static int __init init_espi_data(void)
>>> +{
>>> + return 0;
>>> +}
>>> +#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 +104,9 @@ struct irq_desc *__irq_to_desc(unsigned int irq)
>>> if ( irq < NR_LOCAL_IRQS )
>>> return &this_cpu(local_irq_desc)[irq];
>>> + if ( is_espi(irq) )
>>> + return espi_to_desc(irq);
>>> +
>>> return &irq_desc[irq-NR_LOCAL_IRQS];
>>> }
>>> @@ -79,7 +133,7 @@ static int __init init_irq_data(void)
>>> desc->action = NULL;
>>> }
>>> - return 0;
>>> + return init_espi_data();
>>> }
>>> static int init_local_irq_data(unsigned int cpu)
>>
>
Best regards,
Leonid
