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@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
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
+ 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 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.
+ */
+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)
--
2.34.1Hi Leonid,
On 29/08/2025 17:06, 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@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
>
> 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)
NIT: I would consider adding sanity check (i.e. ASSERT()) to confirm
that both ``intid`` and ``idx`` are within the bounds.
> +
> /* 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.
> + */
Reading this again, I still don't quite understand why we are mentioning
Xen devices. Looking at the code, for Arm, we only seem to use
nr_static_irqs to configure nr_pirqs and XSM. Both are ony used by domains.
So I think this needs to be clarified.
> +#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
> + 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 b8eccfc924..61c915c3f9 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -19,7 +19,11 @@
> #include <asm/gic.h>
> #incl#ude <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
NIT: I think you can use:
const unsigned int nr_irqs = IS_ENABLED(CONFIG_GICV3_ESPI)?
(ESPI_MAX_INTID + 1) : NR_IRQS;
That said, I think we need to rethink about the use of nr_irqs and
nr_static_irqs because they don't entirely make sense for Arm as we
don't support PIRQs.
I would at least try to get rid of one of the variable (maybe nr_irqs)
if not both.
This could be done as a follow-up.
>
> 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.
> + */
I know this was suggested by Oleksandr, however most likely distro will
want to enable this feature so they can be booted on a wide range of
platform. So I think "if there is a need to enable GICV3_ESPI by
default" should be removed.
> +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.
> + */
Is this is not mean to be called, then can we only define the prototype
like we do for __bad_atomic_read()?
> +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)
Cheers,
--
Julien Grall
Hi Julien,
Thank you for your review.
On 01.09.25 19:11, Julien Grall wrote:
> Hi Leonid,
>
> On 29/08/2025 17:06, 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@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
>>
>> 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)
>
> NIT: I would consider adding sanity check (i.e. ASSERT()) to confirm
> that both ``intid`` and ``idx`` are within the bounds.
>
Okay, I will add sanity check with ASSERTs in V6 (similar to
GNTPIN_incr2oflow_mask):
#define ESPI_INTID2IDX(intid) \
({ \
ASSERT(((intid) >= ESPI_BASE_INTID) && \
((intid) <= ESPI_MAX_INTID)); \
((intid) - ESPI_BASE_INTID); \
})
#define ESPI_IDX2INTID(idx) \
({ \
ASSERT(((idx) >= 0) && \
((idx) <= NR_ESPI_IRQS)); \
((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.
>> + */
>
> Reading this again, I still don't quite understand why we are mentioning
> Xen devices. Looking at the code, for Arm, we only seem to use
> nr_static_irqs to configure nr_pirqs and XSM. Both are ony used by domains.
>
> So I think this needs to be clarified.
>
Sure, I will update the comment.
>> +#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
>> + 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 b8eccfc924..61c915c3f9 100644
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -19,7 +19,11 @@
>> #include <asm/gic.h>
>> #incl#ude <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
>
> NIT: I think you can use:
>
> const unsigned int nr_irqs = IS_ENABLED(CONFIG_GICV3_ESPI)?
> (ESPI_MAX_INTID + 1) : NR_IRQS;
>
I will update it to proposed above in V6, thanks.
> That said, I think we need to rethink about the use of nr_irqs and
> nr_static_irqs because they don't entirely make sense for Arm as we
> don't support PIRQs.
>
> I would at least try to get rid of one of the variable (maybe nr_irqs)
> if not both.
>
> This could be done as a follow-up.
>
>> 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.
>> + */
>
> I know this was suggested by Oleksandr, however most likely distro will
> want to enable this feature so they can be booted on a wide range of
> platform. So I think "if there is a need to enable GICV3_ESPI by
> default" should be removed.
>
I will remove the second part of comment in V6.
> > +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.
>> + */
>
> Is this is not mean to be called, then can we only define the prototype
> like we do for __bad_atomic_read()?
>
This would only be possible if is_espi() contains #ifdefs (when
CONFIG_GICV3_ESPI=n, it returns false unconditionally), but we need to
agree on this in the parallel thread discussion related to this patch.
If it is feasible, I will define it as a prototype in v6.
>> +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)
>
> Cheers,
>
Best regards,
Leonid
Hi,
On 02/09/2025 09:56, Leonid Komarianskyi wrote:
>>> 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)
>>
>> NIT: I would consider adding sanity check (i.e. ASSERT()) to confirm
>> that both ``intid`` and ``idx`` are within the bounds.
>>
>
> Okay, I will add sanity check with ASSERTs in V6 (similar to
> GNTPIN_incr2oflow_mask):
>
> #define ESPI_INTID2IDX(intid) \
> ({ \
> ASSERT(((intid) >= ESPI_BASE_INTID) && \
> ((intid) <= ESPI_MAX_INTID)); \
> ((intid) - ESPI_BASE_INTID); \
> })
If you are using a macro, then you will need to stash "intid" in a local
variable. Otherwise, it would be evaluated multiple time.
The alternative is to use a static inline helper which is usually preferred.
Cheers,
--
Julien Grall
Hi Leonid,
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.
>
> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@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
>
> 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".
> + return (irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID);
Also, you don't need parentheses here.
> +#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.
> + */
> +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)
--
WBR, Volodymyr
On 29.08.25 22:45, Volodymyr Babchuk wrote:
>
> Hi Leonid,
Hello Leonid
>
> 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.
>>
>> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@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_tyshchenko@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".
>
>> + return (irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID);
>
> Also, you don't need parentheses here.
>
>> +#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.
>> + */
>> +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)
>
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_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.
>>>
>>> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@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_tyshchenko@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
Hi,
On 01/09/2025 15:42, Leonid Komarianskyi wrote:
>>> 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.
I am not sure about this. If is_espi() is not returning false with
CONFIG_GICV3_EPSI, then the compiler would not be able to optimize code
like:
if (is_espi(...)) {
return espi_to_desc(irq);
}
return &irq_desc[irq-NR_LOCAL_IRQS];
irq_to_desc() is called fairly often, so I would like to keep the code
fairly optimized. An alternative would be to use #ifdef CONFIG_*. I
don't like it, but it could be a compromise if Oleksandr and Volodymyr
wants to push to remove #ifdef from CONFIG_IS_ESPI.
Cheers,
--
Julien Grall
Hello Julien, Volodymyr and Oleksandr,
On 01.09.25 19:16, Julien Grall wrote:
> Hi,
>
> On 01/09/2025 15:42, Leonid Komarianskyi wrote:
>>>> 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.
>
> I am not sure about this. If is_espi() is not returning false with
> CONFIG_GICV3_EPSI, then the compiler would not be able to optimize code
> like:
>
> if (is_espi(...)) {
> return espi_to_desc(irq);
> }
>
> return &irq_desc[irq-NR_LOCAL_IRQS];
>
> irq_to_desc() is called fairly often, so I would like to keep the code
> fairly optimized. An alternative would be to use #ifdef CONFIG_*. I
> don't like it, but it could be a compromise if Oleksandr and Volodymyr
> wants to push to remove #ifdef from CONFIG_IS_ESPI.
>
> Cheers,
>
I am just thinking about a possible compromise between writing logical
code where the function name reflects its actual functionality and
allowing for compiler optimization. Perhaps it would be better to keep
the #ifdef but rename the function to `is_valid_espi()` or something
similar.
In that case, I think there would be less confusion, as it seems
reasonable for a function with such a name to return false when
CONFIG_GICV3_ESPI=n, and also the compiler would be able to optimize the
code.
What do you think about this approach?
Best regards,
Leonid
© 2016 - 2025 Red Hat, Inc.