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
