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 V6:
- added an assert in is_espi() when CONFIG_GICV3_ESPI=n to ensure that
out-of-range array resources are not accessed, e.g., in __irq_to_desc()
- removed unnecessary parentheses in is_espi()
- converted helper macro to inline functions and added sanity checks
with ASSERTs to them
- defined espi_to_desc for non-eSPI builds as a prototype
- updates the comments
- used the IS_ENABLED(CONFIG_GICV3_ESPI) macro to initialize nr_irqs
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 | 37 ++++++++++++++++++++++++
xen/arch/arm/irq.c | 53 ++++++++++++++++++++++++++++++++--
3 files changed, 96 insertions(+), 2 deletions(-)
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..f4d0997651 100644
--- a/xen/arch/arm/include/asm/irq.h
+++ b/xen/arch/arm/include/asm/irq.h
@@ -32,6 +32,10 @@ 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
+
/* LPIs are always numbered starting at 8192, so 0 is a good invalid case. */
#define INVALID_LPI 0
@@ -39,7 +43,12 @@ struct arch_irq_desc {
#define INVALID_IRQ 1023
extern const unsigned int nr_irqs;
+#ifdef CONFIG_GICV3_ESPI
+/* This will cover the eSPI range, to allow asignmant of eSPIs to domains. */
+#define nr_static_irqs (ESPI_MAX_INTID + 1)
+#else
#define nr_static_irqs NR_IRQS
+#endif
struct irq_desc;
struct irqaction;
@@ -55,6 +64,34 @@ static inline bool is_lpi(unsigned int irq)
return irq >= LPI_OFFSET;
}
+static inline unsigned int espi_intid_to_idx(unsigned int intid)
+{
+ ASSERT(intid >= ESPI_BASE_INTID && intid <= ESPI_MAX_INTID);
+ return intid - ESPI_BASE_INTID;
+}
+
+static inline unsigned int espi_idx_to_intid(unsigned int idx)
+{
+ ASSERT(idx <= NR_ESPI_IRQS);
+ return idx + ESPI_BASE_INTID;
+}
+
+static inline bool is_espi(unsigned int irq)
+{
+#ifdef CONFIG_GICV3_ESPI
+ return irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID;
+#else
+ /*
+ * The function should not be called for eSPIs when CONFIG_GICV3_ESPI is
+ * disabled. Returning false allows the compiler to optimize the code
+ * when the config is disabled, while the assert ensures that out-of-range
+ * array resources are not accessed, e.g., in __irq_to_desc().
+ */
+ ASSERT(irq >= ESPI_BASE_INTID);
+ 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..c934d39bf6 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -19,7 +19,9 @@
#include <asm/gic.h>
#include <asm/vgic.h>
-const unsigned int nr_irqs = NR_IRQS;
+const unsigned int nr_irqs = IS_ENABLED(CONFIG_GICV3_ESPI) ?
+ (ESPI_MAX_INTID + 1) :
+ NR_IRQS;
static unsigned int local_irqs_type[NR_LOCAL_IRQS];
static DEFINE_SPINLOCK(local_irqs_type_lock);
@@ -46,6 +48,50 @@ 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 */
+static irq_desc_t espi_desc[NR_ESPI_IRQS];
+
+static struct irq_desc *espi_to_desc(unsigned int irq)
+{
+ return &espi_desc[espi_intid_to_idx(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
+/*
+ * Defined as a prototype as it should not be called if CONFIG_GICV3_ESPI=n.
+ * Without CONFIG_GICV3_ESPI, the additional 1024 IRQ descriptors will not
+ * be defined, and thus, they cannot be used. Unless INTIDs from the eSPI
+ * range are mistakenly defined in Xen DTS when the appropriate config is
+ * disabled, this function will not be reached because is_espi will return
+ * false for non-eSPI INTIDs.
+ */
+struct irq_desc *espi_to_desc(unsigned int irq);
+
+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 +99,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 +128,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 03/09/2025 15:29, Leonid Komarianskyi wrote:
> ---
> xen/arch/arm/Kconfig | 8 +++++
> xen/arch/arm/include/asm/irq.h | 37 ++++++++++++++++++++++++
> xen/arch/arm/irq.c | 53 ++++++++++++++++++++++++++++++++--
> 3 files changed, 96 insertions(+), 2 deletions(-)
>
> 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..f4d0997651 100644
> --- a/xen/arch/arm/include/asm/irq.h
> +++ b/xen/arch/arm/include/asm/irq.h
> @@ -32,6 +32,10 @@ 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
> +
> /* LPIs are always numbered starting at 8192, so 0 is a good invalid case. */
> #define INVALID_LPI 0
>
> @@ -39,7 +43,12 @@ struct arch_irq_desc {
> #define INVALID_IRQ 1023
>
> extern const unsigned int nr_irqs;
> +#ifdef CONFIG_GICV3_ESPI
> +/* This will cover the eSPI range, to allow asignmant of eSPIs to domains. */
> +#define nr_static_irqs (ESPI_MAX_INTID + 1)
> +#else
> #define nr_static_irqs NR_IRQS
> +#endif
>
> struct irq_desc;
> struct irqaction;
> @@ -55,6 +64,34 @@ static inline bool is_lpi(unsigned int irq)
> return irq >= LPI_OFFSET;
> }
>
> +static inline unsigned int espi_intid_to_idx(unsigned int intid)
> +{
> + ASSERT(intid >= ESPI_BASE_INTID && intid <= ESPI_MAX_INTID);
Can we use is_espi()?
> + return intid - ESPI_BASE_INTID;
> +}
> +
> +static inline unsigned int espi_idx_to_intid(unsigned int idx)
> +{
> + ASSERT(idx <= NR_ESPI_IRQS);
> + return idx + ESPI_BASE_INTID;
> +}
> +
> +static inline bool is_espi(unsigned int irq)
> +{
> +#ifdef CONFIG_GICV3_ESPI
> + return irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID;
> +#else
> + /*
> + * The function should not be called for eSPIs when CONFIG_GICV3_ESPI is
> + * disabled. Returning false allows the compiler to optimize the code
> + * when the config is disabled, while the assert ensures that out-of-range
> + * array resources are not accessed, e.g., in __irq_to_desc().
> + */
> + ASSERT(irq >= ESPI_BASE_INTID);
Regardless what Volodymyr mentioned about the assert!(), I am a bit
unsure where we guarantee is_espi() will not be called with an irq <=
ESPI_BASE_INTID. In fact, we could have the following code in Xen:
if (is_espi(irq))
{
}
else if (is_lpi(irq))
{
}
else
{
}
We could replace the check with "!(irq >= ESPI_BASE_INTID && irq <=
ESPI_MAX_INTID)". But I would actually prefer if there is no check
because I don't see the value.
> + return false;
> +#endif
> +}
> +
Cheers,
--
Julien Grall
Hi Julien,
Thank you for your comments.
On 04.09.25 15:27, Julien Grall wrote:
> Hi Leonid,
>
> On 03/09/2025 15:29, Leonid Komarianskyi wrote:
>> ---
>> xen/arch/arm/Kconfig | 8 +++++
>> xen/arch/arm/include/asm/irq.h | 37 ++++++++++++++++++++++++
>> xen/arch/arm/irq.c | 53 ++++++++++++++++++++++++++++++++--
>> 3 files changed, 96 insertions(+), 2 deletions(-)
>>
>> 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..f4d0997651 100644
>> --- a/xen/arch/arm/include/asm/irq.h
>> +++ b/xen/arch/arm/include/asm/irq.h
>> @@ -32,6 +32,10 @@ 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
>> +
>> /* LPIs are always numbered starting at 8192, so 0 is a good invalid
>> case. */
>> #define INVALID_LPI 0
>> @@ -39,7 +43,12 @@ struct arch_irq_desc {
>> #define INVALID_IRQ 1023
>> extern const unsigned int nr_irqs;
>> +#ifdef CONFIG_GICV3_ESPI
>> +/* This will cover the eSPI range, to allow asignmant of eSPIs to
>> domains. */
>> +#define nr_static_irqs (ESPI_MAX_INTID + 1)
>> +#else
>> #define nr_static_irqs NR_IRQS
>> +#endif
>> struct irq_desc;
>> struct irqaction;
>> @@ -55,6 +64,34 @@ static inline bool is_lpi(unsigned int irq)
>> return irq >= LPI_OFFSET;
>> }
>> +static inline unsigned int espi_intid_to_idx(unsigned int intid)
>> +{
>> + ASSERT(intid >= ESPI_BASE_INTID && intid <= ESPI_MAX_INTID);
>
> Can we use is_espi()?
>
Yes, sure. I just need to change the function declaration order and then
I can use is_espi() here. I will do this in V7.
>> + return intid - ESPI_BASE_INTID;
>> +}
>> +
>> +static inline unsigned int espi_idx_to_intid(unsigned int idx)
>> +{
>> + ASSERT(idx <= NR_ESPI_IRQS);
>> + return idx + ESPI_BASE_INTID;
>> +}
>> +
>> +static inline bool is_espi(unsigned int irq)
>> +{
>> +#ifdef CONFIG_GICV3_ESPI
>> + return irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID;
>> +#else
>> + /*
>> + * The function should not be called for eSPIs when
>> CONFIG_GICV3_ESPI is
>> + * disabled. Returning false allows the compiler to optimize the
>> code
>> + * when the config is disabled, while the assert ensures that
>> out-of-range
>> + * array resources are not accessed, e.g., in __irq_to_desc().
>> + */
>> + ASSERT(irq >= ESPI_BASE_INTID);
>
> Regardless what Volodymyr mentioned about the assert!(), I am a bit
> unsure where we guarantee is_espi() will not be called with an irq <=
> ESPI_BASE_INTID. In fact, we could have the following code in Xen:
>
> if (is_espi(irq))
> {
> }
> else if (is_lpi(irq))
> {
> }
> else
> {
> }
>
> We could replace the check with "!(irq >= ESPI_BASE_INTID && irq <=
> ESPI_MAX_INTID)". But I would actually prefer if there is no check
> because I don't see the value.
>
The main reason to add ASSERT here is to trigger it if the config is
disabled but an eSPI INTID is defined in Xen DTS. In this case, instead
of triggering an ASSERT (as proposed), the following will occur in
__irq_to_desc:
// Assume we have irq = 4096
struct irq_desc *__irq_to_desc(unsigned int irq)
{
// This check will return false
if ( irq < NR_LOCAL_IRQS )
return &this_cpu(local_irq_desc)[irq];
/*
* This check will also return false because is_espi()
* will always return false when CONFIG_GICV3_ESPI=n.
*/
if ( is_espi(irq) )
return espi_to_desc(irq);
/*
* We will fall through to this point and attempt to access 4064,
* which does not exist
*/
return &irq_desc[irq-NR_LOCAL_IRQS];
}
So, I think it's better to use ASSERT to simplify error detection in
debug builds.
>> + return false;
>> +#endif
>> +}
>> +
>
> Cheers,
>
Best regards,
Leonid
Hi Leonid,
On 04/09/2025 14:09, Leonid Komarianskyi wrote:
> On 04.09.25 15:27, Julien Grall wrote:
>> Hi Leonid,
>>
>> On 03/09/2025 15:29, Leonid Komarianskyi wrote:
>>> ---
>>> xen/arch/arm/Kconfig | 8 +++++
>>> xen/arch/arm/include/asm/irq.h | 37 ++++++++++++++++++++++++
>>> xen/arch/arm/irq.c | 53 ++++++++++++++++++++++++++++++++--
>>> 3 files changed, 96 insertions(+), 2 deletions(-)
>>>
>>> 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..f4d0997651 100644
>>> --- a/xen/arch/arm/include/asm/irq.h
>>> +++ b/xen/arch/arm/include/asm/irq.h
>>> @@ -32,6 +32,10 @@ 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
>>> +
>>> /* LPIs are always numbered starting at 8192, so 0 is a good invalid
>>> case. */
>>> #define INVALID_LPI 0
>>> @@ -39,7 +43,12 @@ struct arch_irq_desc {
>>> #define INVALID_IRQ 1023
>>> extern const unsigned int nr_irqs;
>>> +#ifdef CONFIG_GICV3_ESPI
>>> +/* This will cover the eSPI range, to allow asignmant of eSPIs to
>>> domains. */
>>> +#define nr_static_irqs (ESPI_MAX_INTID + 1)
>>> +#else
>>> #define nr_static_irqs NR_IRQS
>>> +#endif
>>> struct irq_desc;
>>> struct irqaction;
>>> @@ -55,6 +64,34 @@ static inline bool is_lpi(unsigned int irq)
>>> return irq >= LPI_OFFSET;
>>> }
>>> +static inline unsigned int espi_intid_to_idx(unsigned int intid)
>>> +{
>>> + ASSERT(intid >= ESPI_BASE_INTID && intid <= ESPI_MAX_INTID);
>>
>> Can we use is_espi()?
>>
>
> Yes, sure. I just need to change the function declaration order and then
> I can use is_espi() here. I will do this in V7.
>
>>> + return intid - ESPI_BASE_INTID;
>>> +}
>>> +
>>> +static inline unsigned int espi_idx_to_intid(unsigned int idx)
>>> +{
>>> + ASSERT(idx <= NR_ESPI_IRQS);
>>> + return idx + ESPI_BASE_INTID;
>>> +}
>>> +
>>> +static inline bool is_espi(unsigned int irq)
>>> +{
>>> +#ifdef CONFIG_GICV3_ESPI
>>> + return irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID;
>>> +#else
>>> + /*
>>> + * The function should not be called for eSPIs when
>>> CONFIG_GICV3_ESPI is
>>> + * disabled. Returning false allows the compiler to optimize the
>>> code
>>> + * when the config is disabled, while the assert ensures that
>>> out-of-range
>>> + * array resources are not accessed, e.g., in __irq_to_desc().
>>> + */
>>> + ASSERT(irq >= ESPI_BASE_INTID);
>>
>> Regardless what Volodymyr mentioned about the assert!(), I am a bit
>> unsure where we guarantee is_espi() will not be called with an irq <=
>> ESPI_BASE_INTID. In fact, we could have the following code in Xen:
>>
>> if (is_espi(irq))
>> {
>> }
>> else if (is_lpi(irq))
>> {
>> }
>> else
>> {
>> }
>>
>> We could replace the check with "!(irq >= ESPI_BASE_INTID && irq <=
>> ESPI_MAX_INTID)". But I would actually prefer if there is no check
>> because I don't see the value.
>>
>
> The main reason to add ASSERT here is to trigger it if the config is
> disabled but an eSPI INTID is defined in Xen DTS.
I will not insist on remove the ASSERT(). However, it could correct and
we should avoid relying on ASSERT() to catch DTS bugs. Because...
> In this case, instead
> of triggering an ASSERT (as proposed), the following will occur in
> __irq_to_desc:
>
> // Assume we have irq = 4096
> struct irq_desc *__irq_to_desc(unsigned int irq)
> {
> // This check will return false
> if ( irq < NR_LOCAL_IRQS )
> return &this_cpu(local_irq_desc)[irq];
>
> /*
> * This check will also return false because is_espi()
> * will always return false when CONFIG_GICV3_ESPI=n.
> */
> if ( is_espi(irq) )
> return espi_to_desc(irq);
>
> /*
> * We will fall through to this point and attempt to access 4064,
> * which does not exist
> */
> return &irq_desc[irq-NR_LOCAL_IRQS];
> }
>
> So, I think it's better to use ASSERT to simplify error detection in
> debug builds.
... no everyone will use debug build. So if this is the purpose of the
ASSERT() then we need to have another runtime check during the parsing
of the DTS.
Cheers,
--
Julien Grall
Hi Julien,
Thank you for your comments.
On 04.09.25 17:06, Julien Grall wrote:
> Hi Leonid,
>
> On 04/09/2025 14:09, Leonid Komarianskyi wrote:
>> On 04.09.25 15:27, Julien Grall wrote:
>>> Hi Leonid,
>>>
>>> On 03/09/2025 15:29, Leonid Komarianskyi wrote:
>>>> ---
>>>> xen/arch/arm/Kconfig | 8 +++++
>>>> xen/arch/arm/include/asm/irq.h | 37 ++++++++++++++++++++++++
>>>> xen/arch/arm/irq.c | 53 +++++++++++++++++++++++++++++
>>>> +++--
>>>> 3 files changed, 96 insertions(+), 2 deletions(-)
>>>>
>>>> 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..f4d0997651 100644
>>>> --- a/xen/arch/arm/include/asm/irq.h
>>>> +++ b/xen/arch/arm/include/asm/irq.h
>>>> @@ -32,6 +32,10 @@ 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
>>>> +
>>>> /* LPIs are always numbered starting at 8192, so 0 is a good invalid
>>>> case. */
>>>> #define INVALID_LPI 0
>>>> @@ -39,7 +43,12 @@ struct arch_irq_desc {
>>>> #define INVALID_IRQ 1023
>>>> extern const unsigned int nr_irqs;
>>>> +#ifdef CONFIG_GICV3_ESPI
>>>> +/* This will cover the eSPI range, to allow asignmant of eSPIs to
>>>> domains. */
>>>> +#define nr_static_irqs (ESPI_MAX_INTID + 1)
>>>> +#else
>>>> #define nr_static_irqs NR_IRQS
>>>> +#endif
>>>> struct irq_desc;
>>>> struct irqaction;
>>>> @@ -55,6 +64,34 @@ static inline bool is_lpi(unsigned int irq)
>>>> return irq >= LPI_OFFSET;
>>>> }
>>>> +static inline unsigned int espi_intid_to_idx(unsigned int intid)
>>>> +{
>>>> + ASSERT(intid >= ESPI_BASE_INTID && intid <= ESPI_MAX_INTID);
>>>
>>> Can we use is_espi()?
>>>
>>
>> Yes, sure. I just need to change the function declaration order and then
>> I can use is_espi() here. I will do this in V7.
>>
>>>> + return intid - ESPI_BASE_INTID;
>>>> +}
>>>> +
>>>> +static inline unsigned int espi_idx_to_intid(unsigned int idx)
>>>> +{
>>>> + ASSERT(idx <= NR_ESPI_IRQS);
>>>> + return idx + ESPI_BASE_INTID;
>>>> +}
>>>> +
>>>> +static inline bool is_espi(unsigned int irq)
>>>> +{
>>>> +#ifdef CONFIG_GICV3_ESPI
>>>> + return irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID;
>>>> +#else
>>>> + /*
>>>> + * The function should not be called for eSPIs when
>>>> CONFIG_GICV3_ESPI is
>>>> + * disabled. Returning false allows the compiler to optimize the
>>>> code
>>>> + * when the config is disabled, while the assert ensures that
>>>> out-of-range
>>>> + * array resources are not accessed, e.g., in __irq_to_desc().
>>>> + */
>>>> + ASSERT(irq >= ESPI_BASE_INTID);
>>>
>>> Regardless what Volodymyr mentioned about the assert!(), I am a bit
>>> unsure where we guarantee is_espi() will not be called with an irq <=
>>> ESPI_BASE_INTID. In fact, we could have the following code in Xen:
>>>
>>> if (is_espi(irq))
>>> {
>>> }
>>> else if (is_lpi(irq))
>>> {
>>> }
>>> else
>>> {
>>> }
>>>
>>> We could replace the check with "!(irq >= ESPI_BASE_INTID && irq <=
>>> ESPI_MAX_INTID)". But I would actually prefer if there is no check
>>> because I don't see the value.
>>>
>>
>> The main reason to add ASSERT here is to trigger it if the config is
>> disabled but an eSPI INTID is defined in Xen DTS.
>
> I will not insist on remove the ASSERT(). However, it could correct and
> we should avoid relying on ASSERT() to catch DTS bugs. Because...
>
Yes, I agree with that, but I just checked something else - I tried
using mainline Xen (without eSPI patches) and defined an invalid IRQ
(0x110a00) for a device in the Xen DTS:
interrupts = <0x00 0x110a00 0x04>;
And Xen crashed with a data abort while starting Dom0:
(XEN) *** LOADING DOMAIN 0 ***
(XEN) Loading d0 kernel from boot module @ 000000007a000000
(XEN) Loading ramdisk from boot module @ 0000000055964000
(XEN) Grant table range: 0x00000078200000-0x00000078240000
(XEN) Allocating 1:1 mappings totalling 512MB for dom0:
(XEN) BANK[0] 0x00000068000000-0x00000078000000 (256MB)
(XEN) BANK[1] 0x000010d0000000-0x000010e0000000 (256MB)
(XEN) Data Abort Trap. Syndrome=0x6
(XEN) Walking Hypervisor VA 0xa0008b991a4 on CPU0 via TTBR
0x0000000078348000
(XEN) 0TH[0x014] = 0x78347f7f
(XEN) 1ST[0x000] = 0x78346f7f
(XEN) 2ND[0x045] = 0x0
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) ----[ Xen-4.21-unstable arm64 debug=y Not tainted ]----
(XEN) CPU: 0
(XEN) PC: 00000a00002285c8 _spin_lock+0x40/0xa4
(XEN) LR: 00000a00002285b0
(XEN) SP: 00000a0000326210
(XEN) CPSR: 00000000600002c9 MODE:64-bit EL2h (Hypervisor, handler)
(XEN) X0: 00000a0000330058 X1: 0000000000000001 X2: 0000000000000000
(XEN) X3: 0000000000000000 X4: 0000000000000000 X5: 00000a0000330130
(XEN) X6: 0000000000000000 X7: 0000800fbffdf9b0 X8: 7f7f7f7f7f7f7f7f
(XEN) X9: 0000000000000080 X10: 0101010101010101 X11: 0000000000000030
(XEN) X12: 0000000000000028 X13: ff00000000000000 X14: 0000000004000000
(XEN) X15: 0080000000000000 X16: 00000000000fffff X17: 0000000000000000
(XEN) X18: 00000000bbfefd20 X19: 00000a0008b991a4 X20: 0000000000010000
(XEN) X21: 00000a0008b991a8 X22: 0000000000000004 X23: 0000000000000000
(XEN) X24: 0000000000000000 X25: 0000800fbffcc980 X26: 0000000000000001
(XEN) X27: 0000000000173000 X28: 00000000481a8060 FP: 00000a0000326210
(XEN)
(XEN) VTCR_EL2: 00000000800d3590
(XEN) VTTBR_EL2: 0000000000000000
(XEN)
(XEN) SCTLR_EL2: 0000000030cd183d
(XEN) HCR_EL2: 0000000080000038
(XEN) TTBR0_EL2: 0000000078348000
(XEN)
(XEN) ESR_EL2: 0000000096000006
(XEN) HPFAR_EL2: 0000000000000000
(XEN) FAR_EL2: 00000a0008b991a4
(XEN)
....
(XEN) Xen call trace:
(XEN) [<00000a00002285c8>] _spin_lock+0x40/0xa4 (PC)
(XEN) [<00000a00002285b0>] _spin_lock+0x28/0xa4 (LR)
(XEN) [<00000a000022872c>] _spin_lock_irqsave+0x18/0x28
(XEN) [<00000a0000278e9c>] irq_set_spi_type+0x34/0x78
(XEN) [<00000a0000279034>] irq_set_type+0x154/0x16c
(XEN) [<00000a0000279074>] platform_get_irq+0x28/0x44
(XEN) [<00000a00002e188c>] domain_build.c#handle_node+0x100/0x7b0
(XEN) [<00000a00002e1dac>] domain_build.c#handle_node+0x620/0x7b0
(XEN) [<00000a00002e1dac>] domain_build.c#handle_node+0x620/0x7b0
(XEN) [<00000a00002e24e8>] construct_hwdom+0x3f4/0x4bc
(XEN) [<00000a00002e2650>] domain_build.c#construct_dom0+0xa0/0xb4
(XEN) [<00000a00002e273c>] create_dom0+0xd8/0x11c
(XEN) [<00000a00002e87e8>] start_xen+0x8bc/0x98c
(XEN) [<00000a00002001a4>] head.o#primary_switched+0x4/0x24
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) ****************************************
Currently, Xen does not verify the validity of interrupt numbers defined
in the DTS file. This should definitely be addressed elsewhere and not
just for the eSPI range, but at least the ASSERT for eSPIs will not make
things worse. Perhaps the issue with IRQ number validation should be
fixed in a separate patch series. I will try to look into this issue
after eSPI and dynamic allocation for irq_desc_t array.
>> In this case, instead
>> of triggering an ASSERT (as proposed), the following will occur in
>> __irq_to_desc:
>>
>> // Assume we have irq = 4096
>> struct irq_desc *__irq_to_desc(unsigned int irq)
>> {
>> // This check will return false
>> if ( irq < NR_LOCAL_IRQS )
>> return &this_cpu(local_irq_desc)[irq];
>>
>> /*
>> * This check will also return false because is_espi()
>> * will always return false when CONFIG_GICV3_ESPI=n.
>> */
>> if ( is_espi(irq) )
>> return espi_to_desc(irq);
>>
>> /*
>> * We will fall through to this point and attempt to access 4064,
>> * which does not exist
>> */
>> return &irq_desc[irq-NR_LOCAL_IRQS];
>> }
>>
>> So, I think it's better to use ASSERT to simplify error detection in
>> debug builds.
>
> ... no everyone will use debug build. So if this is the purpose of the
> ASSERT() then we need to have another runtime check during the parsing
> of the DTS.
>
> Cheers,
>
Best regards,
Leonid
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 V6:
> - added an assert in is_espi() when CONFIG_GICV3_ESPI=n to ensure that
> out-of-range array resources are not accessed, e.g., in __irq_to_desc()
> - removed unnecessary parentheses in is_espi()
> - converted helper macro to inline functions and added sanity checks
> with ASSERTs to them
> - defined espi_to_desc for non-eSPI builds as a prototype
> - updates the comments
> - used the IS_ENABLED(CONFIG_GICV3_ESPI) macro to initialize nr_irqs
>
> 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 | 37 ++++++++++++++++++++++++
> xen/arch/arm/irq.c | 53 ++++++++++++++++++++++++++++++++--
> 3 files changed, 96 insertions(+), 2 deletions(-)
>
> 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..f4d0997651 100644
> --- a/xen/arch/arm/include/asm/irq.h
> +++ b/xen/arch/arm/include/asm/irq.h
> @@ -32,6 +32,10 @@ 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
> +
> /* LPIs are always numbered starting at 8192, so 0 is a good invalid case. */
> #define INVALID_LPI 0
>
> @@ -39,7 +43,12 @@ struct arch_irq_desc {
> #define INVALID_IRQ 1023
>
> extern const unsigned int nr_irqs;
> +#ifdef CONFIG_GICV3_ESPI
> +/* This will cover the eSPI range, to allow asignmant of eSPIs to domains. */
> +#define nr_static_irqs (ESPI_MAX_INTID + 1)
> +#else
> #define nr_static_irqs NR_IRQS
> +#endif
>
> struct irq_desc;
> struct irqaction;
> @@ -55,6 +64,34 @@ static inline bool is_lpi(unsigned int irq)
> return irq >= LPI_OFFSET;
> }
>
> +static inline unsigned int espi_intid_to_idx(unsigned int intid)
> +{
> + ASSERT(intid >= ESPI_BASE_INTID && intid <= ESPI_MAX_INTID);
> + return intid - ESPI_BASE_INTID;
> +}
> +
> +static inline unsigned int espi_idx_to_intid(unsigned int idx)
> +{
> + ASSERT(idx <= NR_ESPI_IRQS);
> + return idx + ESPI_BASE_INTID;
> +}
> +
> +static inline bool is_espi(unsigned int irq)
> +{
> +#ifdef CONFIG_GICV3_ESPI
> + return irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID;
> +#else
> + /*
> + * The function should not be called for eSPIs when CONFIG_GICV3_ESPI is
> + * disabled. Returning false allows the compiler to optimize the code
> + * when the config is disabled, while the assert ensures that out-of-range
> + * array resources are not accessed, e.g., in __irq_to_desc().
> + */
> + ASSERT(irq >= ESPI_BASE_INTID);
This really puzzles me. Should it be other way around? I.e.
ASSERT(irq < ESPI_BASE_INTID) ? Or even ASSERT(irq <= 1022) ?
Actually, I tried to your series. XEN does not boots at all when
CONFIG_GICV3_ESPI=n. Looks like it panics even before it can bring up
the console, as I don't see any prints in QEMU. Non-debug build boots
fine, thought, but this is expected, as ASSERTs are disabled.
> + 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..c934d39bf6 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -19,7 +19,9 @@
> #include <asm/gic.h>
> #include <asm/vgic.h>
>
> -const unsigned int nr_irqs = NR_IRQS;
> +const unsigned int nr_irqs = IS_ENABLED(CONFIG_GICV3_ESPI) ?
> + (ESPI_MAX_INTID + 1) :
> + NR_IRQS;
>
> static unsigned int local_irqs_type[NR_LOCAL_IRQS];
> static DEFINE_SPINLOCK(local_irqs_type_lock);
> @@ -46,6 +48,50 @@ 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 */
I'd considered using radix tree, honestly... But this is just topic for
discussion, no action should be taken here.
> +static irq_desc_t espi_desc[NR_ESPI_IRQS];
> +
> +static struct irq_desc *espi_to_desc(unsigned int irq)
> +{
> + return &espi_desc[espi_intid_to_idx(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
> +/*
> + * Defined as a prototype as it should not be called if CONFIG_GICV3_ESPI=n.
> + * Without CONFIG_GICV3_ESPI, the additional 1024 IRQ descriptors will not
> + * be defined, and thus, they cannot be used. Unless INTIDs from the eSPI
> + * range are mistakenly defined in Xen DTS when the appropriate config is
> + * disabled, this function will not be reached because is_espi will return
> + * false for non-eSPI INTIDs.
> + */
> +struct irq_desc *espi_to_desc(unsigned int irq);
> +
> +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 +99,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 +128,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
Hi Volodymyr,
Thank you for your close review and for your time while reviewing so
many versions.
On 03.09.25 23:56, Volodymyr Babchuk wrote:
> 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 V6:
>> - added an assert in is_espi() when CONFIG_GICV3_ESPI=n to ensure that
>> out-of-range array resources are not accessed, e.g., in __irq_to_desc()
>> - removed unnecessary parentheses in is_espi()
>> - converted helper macro to inline functions and added sanity checks
>> with ASSERTs to them
>> - defined espi_to_desc for non-eSPI builds as a prototype
>> - updates the comments
>> - used the IS_ENABLED(CONFIG_GICV3_ESPI) macro to initialize nr_irqs
>>
>> 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 | 37 ++++++++++++++++++++++++
>> xen/arch/arm/irq.c | 53 ++++++++++++++++++++++++++++++++--
>> 3 files changed, 96 insertions(+), 2 deletions(-)
>>
>> 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..f4d0997651 100644
>> --- a/xen/arch/arm/include/asm/irq.h
>> +++ b/xen/arch/arm/include/asm/irq.h
>> @@ -32,6 +32,10 @@ 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
>> +
>> /* LPIs are always numbered starting at 8192, so 0 is a good invalid case. */
>> #define INVALID_LPI 0
>>
>> @@ -39,7 +43,12 @@ struct arch_irq_desc {
>> #define INVALID_IRQ 1023
>>
>> extern const unsigned int nr_irqs;
>> +#ifdef CONFIG_GICV3_ESPI
>> +/* This will cover the eSPI range, to allow asignmant of eSPIs to domains. */
>> +#define nr_static_irqs (ESPI_MAX_INTID + 1)
>> +#else
>> #define nr_static_irqs NR_IRQS
>> +#endif
>>
>> struct irq_desc;
>> struct irqaction;
>> @@ -55,6 +64,34 @@ static inline bool is_lpi(unsigned int irq)
>> return irq >= LPI_OFFSET;
>> }
>>
>> +static inline unsigned int espi_intid_to_idx(unsigned int intid)
>> +{
>> + ASSERT(intid >= ESPI_BASE_INTID && intid <= ESPI_MAX_INTID);
>> + return intid - ESPI_BASE_INTID;
>> +}
>> +
>> +static inline unsigned int espi_idx_to_intid(unsigned int idx)
>> +{
>> + ASSERT(idx <= NR_ESPI_IRQS);
>> + return idx + ESPI_BASE_INTID;
>> +}
>> +
>> +static inline bool is_espi(unsigned int irq)
>> +{
>> +#ifdef CONFIG_GICV3_ESPI
>> + return irq >= ESPI_BASE_INTID && irq <= ESPI_MAX_INTID;
>> +#else
>> + /*
>> + * The function should not be called for eSPIs when CONFIG_GICV3_ESPI is
>> + * disabled. Returning false allows the compiler to optimize the code
>> + * when the config is disabled, while the assert ensures that out-of-range
>> + * array resources are not accessed, e.g., in __irq_to_desc().
>> + */
>> + ASSERT(irq >= ESPI_BASE_INTID);
>
> This really puzzles me. Should it be other way around? I.e.
>
> ASSERT(irq < ESPI_BASE_INTID) ? Or even ASSERT(irq <= 1022) ?
>
> Actually, I tried to your series. XEN does not boots at all when
> CONFIG_GICV3_ESPI=n. Looks like it panics even before it can bring up
> the console, as I don't see any prints in QEMU. Non-debug build boots
> fine, thought, but this is expected, as ASSERTs are disabled.
>
>
Yes, it's my bad, I really apologize for that. It is a critical issue.
It should definitely be at least irq < ESPI_BASE_INTID...
>> + 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..c934d39bf6 100644
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -19,7 +19,9 @@
>> #include <asm/gic.h>
>> #include <asm/vgic.h>
>>
>> -const unsigned int nr_irqs = NR_IRQS;
>> +const unsigned int nr_irqs = IS_ENABLED(CONFIG_GICV3_ESPI) ?
>> + (ESPI_MAX_INTID + 1) :
>> + NR_IRQS;
>>
>> static unsigned int local_irqs_type[NR_LOCAL_IRQS];
>> static DEFINE_SPINLOCK(local_irqs_type_lock);
>> @@ -46,6 +48,50 @@ 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 */
>
> I'd considered using radix tree, honestly... But this is just topic for
> discussion, no action should be taken here.
>
>> +static irq_desc_t espi_desc[NR_ESPI_IRQS];
>> +
>> +static struct irq_desc *espi_to_desc(unsigned int irq)
>> +{
>> + return &espi_desc[espi_intid_to_idx(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
>> +/*
>> + * Defined as a prototype as it should not be called if CONFIG_GICV3_ESPI=n.
>> + * Without CONFIG_GICV3_ESPI, the additional 1024 IRQ descriptors will not
>> + * be defined, and thus, they cannot be used. Unless INTIDs from the eSPI
>> + * range are mistakenly defined in Xen DTS when the appropriate config is
>> + * disabled, this function will not be reached because is_espi will return
>> + * false for non-eSPI INTIDs.
>> + */
>> +struct irq_desc *espi_to_desc(unsigned int irq);
>> +
>> +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 +99,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 +128,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
© 2016 - 2025 Red Hat, Inc.