* [PATCH v6 01/12] xen/arm: gicv3: refactor obtaining GIC addresses for common operations
2025-09-03 14:29 [PATCH v6 00/12] Introduce eSPI support Leonid Komarianskyi
@ 2025-09-03 14:29 ` Leonid Komarianskyi
2025-09-03 14:29 ` [PATCH v6 02/12] xen/arm: gic: implement helper functions for INTID checks Leonid Komarianskyi
` (10 subsequent siblings)
11 siblings, 0 replies; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-03 14:29 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: olekstysh@gmail.com, Leonid Komarianskyi, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel, Volodymyr Babchuk,
Volodymyr Babchuk, Oleksandr Tyshchenko, Julien Grall
Currently, many common functions perform the same operations to calculate
GIC register addresses. This patch consolidates the similar code into
a separate helper function to improve maintainability and reduce duplication.
This refactoring also simplifies the implementation of eSPI support in future
changes.
Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
Changes in V6:
- no functional changes, just fixing minor nit: changed u32 to uint32_t
in get_addr_by_offset()
- added reviewed-by from Oleksandr Tyshchenko
Changes in V5:
- fixed a minor nit: changed %d to %u in the warning message because the
IRQ number cannot be less than zero
- added acked-by from Julien Grall
Changes in V4:
- no changes
Changes in V3:
- changed panic() in get_addr_by_offset() to printing warning and
ASSERT_UNREACHABLE()
- added verification of return pointer from get_addr_by_offset() in the
callers
- moved invocation of get_addr_by_offset() from spinlock guards, since
it is not necessarry
- added RB from Volodymyr Babchuk
Changes in V2:
- no changes
---
xen/arch/arm/gic-v3.c | 114 +++++++++++++++++++++++----------
xen/arch/arm/include/asm/irq.h | 1 +
2 files changed, 81 insertions(+), 34 deletions(-)
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index cd3e1acf79..a1e302fea2 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -445,17 +445,67 @@ static void gicv3_dump_state(const struct vcpu *v)
}
}
+static void __iomem *get_addr_by_offset(struct irq_desc *irqd, uint32_t offset)
+{
+ switch ( irqd->irq )
+ {
+ case 0 ... (NR_GIC_LOCAL_IRQS - 1):
+ switch ( offset )
+ {
+ case GICD_ISENABLER:
+ case GICD_ICENABLER:
+ case GICD_ISPENDR:
+ case GICD_ICPENDR:
+ case GICD_ISACTIVER:
+ case GICD_ICACTIVER:
+ return (GICD_RDIST_SGI_BASE + offset);
+ case GICD_ICFGR:
+ return (GICD_RDIST_SGI_BASE + GICR_ICFGR1);
+ case GICD_IPRIORITYR:
+ return (GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 + irqd->irq);
+ default:
+ break;
+ }
+ case NR_GIC_LOCAL_IRQS ... SPI_MAX_INTID:
+ switch ( offset )
+ {
+ case GICD_ISENABLER:
+ case GICD_ICENABLER:
+ case GICD_ISPENDR:
+ case GICD_ICPENDR:
+ case GICD_ISACTIVER:
+ case GICD_ICACTIVER:
+ return (GICD + offset + (irqd->irq / 32) * 4);
+ case GICD_ICFGR:
+ return (GICD + GICD_ICFGR + (irqd->irq / 16) * 4);
+ case GICD_IROUTER:
+ return (GICD + GICD_IROUTER + irqd->irq * 8);
+ case GICD_IPRIORITYR:
+ return (GICD + GICD_IPRIORITYR + irqd->irq);
+ default:
+ break;
+ }
+ default:
+ break;
+ }
+
+ /* Something went wrong, we shouldn't be able to reach here */
+ printk(XENLOG_WARNING "GICv3: WARNING: Invalid offset 0x%x for IRQ#%u",
+ offset, irqd->irq);
+ ASSERT_UNREACHABLE();
+
+ return NULL;
+}
+
static void gicv3_poke_irq(struct irq_desc *irqd, u32 offset, bool wait_for_rwp)
{
u32 mask = 1U << (irqd->irq % 32);
- void __iomem *base;
+ void __iomem *addr = get_addr_by_offset(irqd, offset);
- if ( irqd->irq < NR_GIC_LOCAL_IRQS )
- base = GICD_RDIST_SGI_BASE;
- else
- base = GICD;
+ if ( addr == NULL )
+ return;
- writel_relaxed(mask, base + offset + (irqd->irq / 32) * 4);
+ writel_relaxed(mask, addr);
if ( wait_for_rwp )
gicv3_wait_for_rwp(irqd->irq);
@@ -463,15 +513,12 @@ static void gicv3_poke_irq(struct irq_desc *irqd, u32 offset, bool wait_for_rwp)
static bool gicv3_peek_irq(struct irq_desc *irqd, u32 offset)
{
- void __iomem *base;
- unsigned int irq = irqd->irq;
+ void __iomem *addr = get_addr_by_offset(irqd, offset);
- if ( irq >= NR_GIC_LOCAL_IRQS)
- base = GICD + (irq / 32) * 4;
- else
- base = GICD_RDIST_SGI_BASE;
+ if ( addr == NULL )
+ return false;
- return !!(readl(base + offset) & (1U << (irq % 32)));
+ return !!(readl(addr) & (1U << (irqd->irq % 32)));
}
static void gicv3_unmask_irq(struct irq_desc *irqd)
@@ -558,30 +605,28 @@ static inline uint64_t gicv3_mpidr_to_affinity(int cpu)
static void gicv3_set_irq_type(struct irq_desc *desc, unsigned int type)
{
uint32_t cfg, actual, edgebit;
- void __iomem *base;
- unsigned int irq = desc->irq;
+ void __iomem *addr;
/* SGI's are always edge-triggered not need to call GICD_ICFGR0 */
- ASSERT(irq >= NR_GIC_SGI);
+ ASSERT(desc->irq >= NR_GIC_SGI);
- spin_lock(&gicv3.lock);
+ addr = get_addr_by_offset(desc, GICD_ICFGR);
+ if ( addr == NULL )
+ return;
- if ( irq >= NR_GIC_LOCAL_IRQS)
- base = GICD + GICD_ICFGR + (irq / 16) * 4;
- else
- base = GICD_RDIST_SGI_BASE + GICR_ICFGR1;
+ spin_lock(&gicv3.lock);
- cfg = readl_relaxed(base);
+ cfg = readl_relaxed(addr);
- edgebit = 2u << (2 * (irq % 16));
+ edgebit = 2u << (2 * (desc->irq % 16));
if ( type & IRQ_TYPE_LEVEL_MASK )
cfg &= ~edgebit;
else if ( type & IRQ_TYPE_EDGE_BOTH )
cfg |= edgebit;
- writel_relaxed(cfg, base);
+ writel_relaxed(cfg, addr);
- actual = readl_relaxed(base);
+ actual = readl_relaxed(addr);
if ( ( cfg & edgebit ) ^ ( actual & edgebit ) )
{
printk(XENLOG_WARNING "GICv3: WARNING: "
@@ -600,16 +645,13 @@ static void gicv3_set_irq_type(struct irq_desc *desc, unsigned int type)
static void gicv3_set_irq_priority(struct irq_desc *desc,
unsigned int priority)
{
- unsigned int irq = desc->irq;
+ void __iomem *addr = get_addr_by_offset(desc, GICD_IPRIORITYR);
- spin_lock(&gicv3.lock);
-
- /* Set priority */
- if ( irq < NR_GIC_LOCAL_IRQS )
- writeb_relaxed(priority, GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 + irq);
- else
- writeb_relaxed(priority, GICD + GICD_IPRIORITYR + irq);
+ if ( addr == NULL )
+ return;
+ spin_lock(&gicv3.lock);
+ writeb_relaxed(priority, addr);
spin_unlock(&gicv3.lock);
}
@@ -1273,6 +1315,10 @@ static void gicv3_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
{
unsigned int cpu;
uint64_t affinity;
+ void __iomem *addr = get_addr_by_offset(desc, GICD_IROUTER);
+
+ if ( addr == NULL )
+ return;
ASSERT(!cpumask_empty(mask));
@@ -1284,7 +1330,7 @@ static void gicv3_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
if ( desc->irq >= NR_GIC_LOCAL_IRQS )
- writeq_relaxed_non_atomic(affinity, (GICD + GICD_IROUTER + desc->irq * 8));
+ writeq_relaxed_non_atomic(affinity, addr);
spin_unlock(&gicv3.lock);
}
diff --git a/xen/arch/arm/include/asm/irq.h b/xen/arch/arm/include/asm/irq.h
index fce7e42a33..5bc6475eb4 100644
--- a/xen/arch/arm/include/asm/irq.h
+++ b/xen/arch/arm/include/asm/irq.h
@@ -29,6 +29,7 @@ struct arch_irq_desc {
*/
#define NR_IRQS 1024
+#define SPI_MAX_INTID 1019
#define LPI_OFFSET 8192
/* LPIs are always numbered starting at 8192, so 0 is a good invalid case. */
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v6 02/12] xen/arm: gic: implement helper functions for INTID checks
2025-09-03 14:29 [PATCH v6 00/12] Introduce eSPI support Leonid Komarianskyi
2025-09-03 14:29 ` [PATCH v6 01/12] xen/arm: gicv3: refactor obtaining GIC addresses for common operations Leonid Komarianskyi
@ 2025-09-03 14:29 ` Leonid Komarianskyi
2025-09-03 14:29 ` [PATCH v6 03/12] xen/arm: vgic: implement helper functions for virq checks Leonid Komarianskyi
` (9 subsequent siblings)
11 siblings, 0 replies; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-03 14:29 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: olekstysh@gmail.com, Leonid Komarianskyi, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel, Volodymyr Babchuk,
Volodymyr Babchuk, Julien Grall
Introduced two new helper functions: gic_is_valid_line and
gic_is_spi. The first function helps determine whether an IRQ
number is less than the number of lines supported by hardware. The
second function additionally checks if the IRQ number falls within the
SPI range. Also, updated the appropriate checks to use these new helper
functions.
The current checks for the real GIC are very similar to those for the
vGIC but serve a different purpose. For GIC-related code, the interrupt
numbers should be validated based on whether the hardware can operate
with such interrupts. On the other hand, for the vGIC, the indexes must
also be verified to ensure they are available for a specific domain. The
first reason for introducing these helper functions is to avoid
potential confusion with vGIC-related checks. The second reason is to
consolidate similar code into separate functions, which can be more
easily extended by additional conditions, e.g., when implementing
extended SPI interrupts.
The changes, which replace open-coded checks with the use of the new
helper functions, do not introduce any functional changes, as the helper
functions follow the current IRQ index verification logic.
Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
Changes in V6:
- no changes
Changes in V5:
- fixed a minor nit: moved the existing comment to the line above to fix
formatting that exceeded 80 characters
- added acked-by from Julien Grall
Changes in V4:
- removed redundant parentheses
- added reviewed-by from Volodymyr Babchuk
Changes in V3:
- renamed gic_is_valid_irq to gic_is_valid_line and gic_is_shared_irq to
gic_is_spi
- updated commit message
Changes in V2:
- introduced this patch
---
xen/arch/arm/gic.c | 3 ++-
xen/arch/arm/include/asm/gic.h | 9 +++++++++
xen/arch/arm/irq.c | 2 +-
3 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index e80fe0ca24..4bb11960ee 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -111,7 +111,8 @@ static void gic_set_irq_priority(struct irq_desc *desc, unsigned int priority)
void gic_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
{
ASSERT(priority <= 0xff); /* Only 8 bits of priority */
- ASSERT(desc->irq < gic_number_lines());/* Can't route interrupts that don't exist */
+ /* Can't route interrupts that don't exist */
+ ASSERT(gic_is_valid_line(desc->irq));
ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
ASSERT(spin_is_locked(&desc->lock));
diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
index 541f0eeb80..3fcee42675 100644
--- a/xen/arch/arm/include/asm/gic.h
+++ b/xen/arch/arm/include/asm/gic.h
@@ -306,6 +306,15 @@ extern void gic_dump_vgic_info(struct vcpu *v);
/* Number of interrupt lines */
extern unsigned int gic_number_lines(void);
+static inline bool gic_is_valid_line(unsigned int irq)
+{
+ return irq < gic_number_lines();
+}
+
+static inline bool gic_is_spi(unsigned int irq)
+{
+ return irq >= NR_LOCAL_IRQS && gic_is_valid_line(irq);
+}
/* IRQ translation function for the device tree */
int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 03fbb90c6c..7dd5a2a453 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -415,7 +415,7 @@ err:
bool is_assignable_irq(unsigned int irq)
{
/* For now, we can only route SPIs to the guest */
- return (irq >= NR_LOCAL_IRQS) && (irq < gic_number_lines());
+ return gic_is_spi(irq);
}
/*
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v6 03/12] xen/arm: vgic: implement helper functions for virq checks
2025-09-03 14:29 [PATCH v6 00/12] Introduce eSPI support Leonid Komarianskyi
2025-09-03 14:29 ` [PATCH v6 01/12] xen/arm: gicv3: refactor obtaining GIC addresses for common operations Leonid Komarianskyi
2025-09-03 14:29 ` [PATCH v6 02/12] xen/arm: gic: implement helper functions for INTID checks Leonid Komarianskyi
@ 2025-09-03 14:29 ` Leonid Komarianskyi
2025-09-03 14:29 ` [PATCH v6 04/12] xen/arm/irq: add handling for IRQs in the eSPI range Leonid Komarianskyi
` (8 subsequent siblings)
11 siblings, 0 replies; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-03 14:29 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: olekstysh@gmail.com, Leonid Komarianskyi, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel, Volodymyr Babchuk,
Oleksandr Tyshchenko, Volodymyr Babchuk, Julien Grall
Introduced two new helper functions for vGIC: vgic_is_valid_line and
vgic_is_spi. The functions are similar to the newly introduced
gic_is_valid_line and gic_is_spi, but they verify whether a vIRQ
is available for a specific domain, while GIC-specific functions
validate INTIDs for the real GIC hardware. For example, the GIC may
support all 992 SPI lines, but the domain may use only some part of them
(e.g., 640), depending on the highest IRQ number defined in the domain
configuration. Therefore, for vGIC-related code and checks, the
appropriate functions should be used. Also, updated the appropriate
checks to use these new helper functions.
The purpose of introducing new helper functions for vGIC is essentially
the same as for GIC: to avoid potential confusion with GIC-related
checks and to consolidate similar code into separate functions, which
can be more easily extended by additional conditions, e.g., when
implementing extended SPI interrupts.
Only the validation change in vgic_inject_irq may affect existing
functionality, as it currently checks whether the vIRQ is less than or
equal to vgic_num_irqs. Since IRQ indexes start from 0 (where 32 is the
first SPI), the check should behave consistently with similar logic in
other places and should check if the vIRQ number is less than
vgic_num_irqs. The remaining changes, which replace open-coded checks
with the use of these new helper functions, do not introduce any
functional changes, as the helper functions follow the current vIRQ
index verification logic.
Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
Changes in V6:
- no changes
Changes in V5:
- added reviewed-by from Oleksandr Tyshchenko and from Volodymyr Babchuk
- added acked-by from Julien Grall
Changes in V4:
- removed redundant parentheses
Changes in V3:
- renamed vgic_is_valid_irq to vgic_is_valid_line and vgic_is_shared_irq
to vgic_is_spi
- added vgic_is_valid_line implementation for new-vgic, because
vgic_is_valid_line is called from generic code. It is necessary to fix
the build for new-vgic.
- updated commit message
Changes in V2:
- introduced this patch
---
xen/arch/arm/gic.c | 3 +--
xen/arch/arm/include/asm/vgic.h | 7 +++++++
xen/arch/arm/irq.c | 4 ++--
xen/arch/arm/vgic.c | 10 ++++++++--
xen/arch/arm/vgic/vgic.c | 5 +++++
5 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 4bb11960ee..9469c9d08c 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -134,8 +134,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
ASSERT(spin_is_locked(&desc->lock));
/* Caller has already checked that the IRQ is an SPI */
- ASSERT(virq >= 32);
- ASSERT(virq < vgic_num_irqs(d));
+ ASSERT(vgic_is_spi(d, virq));
ASSERT(!is_lpi(virq));
ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
index 35c0c6a8b0..3e7cbbb196 100644
--- a/xen/arch/arm/include/asm/vgic.h
+++ b/xen/arch/arm/include/asm/vgic.h
@@ -335,6 +335,13 @@ extern void vgic_check_inflight_irqs_pending(struct vcpu *v,
/* Default number of vGIC SPIs. 32 are substracted to cover local IRQs. */
#define VGIC_DEF_NR_SPIS (min(gic_number_lines(), VGIC_MAX_IRQS) - 32)
+extern bool vgic_is_valid_line(struct domain *d, unsigned int virq);
+
+static inline bool vgic_is_spi(struct domain *d, unsigned int virq)
+{
+ return virq >= NR_LOCAL_IRQS && vgic_is_valid_line(d, virq);
+}
+
/*
* Allocate a guest VIRQ
* - spi == 0 => allocate a PPI. It will be the same on every vCPU
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 7dd5a2a453..b8eccfc924 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -442,7 +442,7 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
unsigned long flags;
int retval = 0;
- if ( virq >= vgic_num_irqs(d) )
+ if ( !vgic_is_valid_line(d, virq) )
{
printk(XENLOG_G_ERR
"the vIRQ number %u is too high for domain %u (max = %u)\n",
@@ -560,7 +560,7 @@ int release_guest_irq(struct domain *d, unsigned int virq)
int ret;
/* Only SPIs are supported */
- if ( virq < NR_LOCAL_IRQS || virq >= vgic_num_irqs(d) )
+ if ( !vgic_is_spi(d, virq) )
return -EINVAL;
desc = vgic_get_hw_irq_desc(d, NULL, virq);
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index c563ba93af..2bbf4d99aa 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -24,6 +24,12 @@
#include <asm/gic.h>
#include <asm/vgic.h>
+
+bool vgic_is_valid_line(struct domain *d, unsigned int virq)
+{
+ return virq < vgic_num_irqs(d);
+}
+
static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
unsigned int rank)
{
@@ -582,7 +588,7 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq,
if ( !v )
{
/* The IRQ needs to be an SPI if no vCPU is specified. */
- ASSERT(virq >= 32 && virq <= vgic_num_irqs(d));
+ ASSERT(vgic_is_spi(d, virq));
v = vgic_get_target_vcpu(d->vcpu[0], virq);
};
@@ -659,7 +665,7 @@ bool vgic_emulate(struct cpu_user_regs *regs, union hsr hsr)
bool vgic_reserve_virq(struct domain *d, unsigned int virq)
{
- if ( virq >= vgic_num_irqs(d) )
+ if ( !vgic_is_valid_line(d, virq) )
return false;
return !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index 6cabd0496d..b2c0e1873a 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -718,6 +718,11 @@ bool vgic_reserve_virq(struct domain *d, unsigned int virq)
return !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
}
+bool vgic_is_valid_line(struct domain *d, unsigned int virq)
+{
+ return virq < vgic_num_irqs(d);
+}
+
int vgic_allocate_virq(struct domain *d, bool spi)
{
int first, end;
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v6 04/12] xen/arm/irq: add handling for IRQs in the eSPI range
2025-09-03 14:29 [PATCH v6 00/12] Introduce eSPI support Leonid Komarianskyi
` (2 preceding siblings ...)
2025-09-03 14:29 ` [PATCH v6 03/12] xen/arm: vgic: implement helper functions for virq checks Leonid Komarianskyi
@ 2025-09-03 14:29 ` Leonid Komarianskyi
2025-09-03 20:56 ` Volodymyr Babchuk
2025-09-04 12:27 ` Julien Grall
2025-09-03 14:30 ` [PATCH v6 05/12] xen/arm: gicv3: implement handling of GICv3.1 eSPI Leonid Komarianskyi
` (7 subsequent siblings)
11 siblings, 2 replies; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-03 14:29 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: olekstysh@gmail.com, Leonid Komarianskyi, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel, Volodymyr Babchuk
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.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v6 04/12] xen/arm/irq: add handling for IRQs in the eSPI range
2025-09-03 14:29 ` [PATCH v6 04/12] xen/arm/irq: add handling for IRQs in the eSPI range Leonid Komarianskyi
@ 2025-09-03 20:56 ` Volodymyr Babchuk
2025-09-03 21:16 ` Leonid Komarianskyi
2025-09-04 12:27 ` Julien Grall
1 sibling, 1 reply; 34+ messages in thread
From: Volodymyr Babchuk @ 2025-09-03 20:56 UTC (permalink / raw)
To: Leonid Komarianskyi
Cc: xen-devel@lists.xenproject.org, olekstysh@gmail.com,
Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel
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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v6 04/12] xen/arm/irq: add handling for IRQs in the eSPI range
2025-09-03 20:56 ` Volodymyr Babchuk
@ 2025-09-03 21:16 ` Leonid Komarianskyi
0 siblings, 0 replies; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-03 21:16 UTC (permalink / raw)
To: Volodymyr Babchuk
Cc: xen-devel@lists.xenproject.org, olekstysh@gmail.com,
Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 04/12] xen/arm/irq: add handling for IRQs in the eSPI range
2025-09-03 14:29 ` [PATCH v6 04/12] xen/arm/irq: add handling for IRQs in the eSPI range Leonid Komarianskyi
2025-09-03 20:56 ` Volodymyr Babchuk
@ 2025-09-04 12:27 ` Julien Grall
2025-09-04 13:09 ` Leonid Komarianskyi
1 sibling, 1 reply; 34+ messages in thread
From: Julien Grall @ 2025-09-04 12:27 UTC (permalink / raw)
To: Leonid Komarianskyi, xen-devel@lists.xenproject.org
Cc: olekstysh@gmail.com, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk
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()?
> + 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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v6 04/12] xen/arm/irq: add handling for IRQs in the eSPI range
2025-09-04 12:27 ` Julien Grall
@ 2025-09-04 13:09 ` Leonid Komarianskyi
2025-09-04 14:06 ` Julien Grall
0 siblings, 1 reply; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-04 13:09 UTC (permalink / raw)
To: Julien Grall, xen-devel@lists.xenproject.org
Cc: olekstysh@gmail.com, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk
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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v6 04/12] xen/arm/irq: add handling for IRQs in the eSPI range
2025-09-04 13:09 ` Leonid Komarianskyi
@ 2025-09-04 14:06 ` Julien Grall
2025-09-04 17:34 ` Leonid Komarianskyi
0 siblings, 1 reply; 34+ messages in thread
From: Julien Grall @ 2025-09-04 14:06 UTC (permalink / raw)
To: Leonid Komarianskyi, xen-devel@lists.xenproject.org
Cc: olekstysh@gmail.com, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk
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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v6 04/12] xen/arm/irq: add handling for IRQs in the eSPI range
2025-09-04 14:06 ` Julien Grall
@ 2025-09-04 17:34 ` Leonid Komarianskyi
0 siblings, 0 replies; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-04 17:34 UTC (permalink / raw)
To: Julien Grall, xen-devel@lists.xenproject.org
Cc: olekstysh@gmail.com, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v6 05/12] xen/arm: gicv3: implement handling of GICv3.1 eSPI
2025-09-03 14:29 [PATCH v6 00/12] Introduce eSPI support Leonid Komarianskyi
` (3 preceding siblings ...)
2025-09-03 14:29 ` [PATCH v6 04/12] xen/arm/irq: add handling for IRQs in the eSPI range Leonid Komarianskyi
@ 2025-09-03 14:30 ` Leonid Komarianskyi
2025-09-04 14:37 ` Oleksandr Tyshchenko
2025-09-03 14:30 ` [PATCH v6 06/12] xen/arm/irq: allow eSPI processing in the gic_interrupt function Leonid Komarianskyi
` (6 subsequent siblings)
11 siblings, 1 reply; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-03 14:30 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: olekstysh@gmail.com, Leonid Komarianskyi, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel, Volodymyr Babchuk,
Oleksandr Tyshchenko
Introduced appropriate register definitions, helper macros,
and initialization of required GICv3.1 distributor registers
to support eSPI. This type of interrupt is handled in the
same way as regular SPI interrupts, with the following
differences:
1) eSPIs can have up to 1024 interrupts, starting from the
beginning of the range, whereas regular SPIs use INTIDs from
32 to 1019, totaling 988 interrupts;
2) eSPIs start at INTID 4096, necessitating additional interrupt
index conversion during register operations.
In case if appropriate config is disabled, or GIC HW doesn't
support eSPI, the existing functionality will remain the same.
Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
Changes in V6:
- removed unnecessary parentheses in gic_is_valid_espi()
- updated gic_is_valid_line(): it now verifies the condition irq <
gic_number_lines() first, as it is more likely that the irq number
will be from the non-eSPI range
- minor change: changed the macros ESPI_INTID2IDX and ESPI_IDX2INTID
into appropriate inline functions introduced in the previous patch
- added reviewed-by from Oleksandr Tyshchenko
Changes in V5:
- fixed minor nits, no functional changes: changed u32 to uint32_t and
added a comment noting that the configuration for eSPIs is the same as
for regular SPIs
- removed ifdefs for eSPI-specific offsets to reduce the number of
ifdefs and code duplication in further changes
- removed reviewed-by as moving offset from ifdefs requires additional
confirmation from reviewers
Changes in V4:
- added offsets for GICD_IGRPMODRnE and GICD_NSACRnE that are required
for vGIC emulation
- added a log banner with eSPI information, similar to the one for
regular SPI
- added newline after ifdef and before gic_is_valid_line
- added reviewed-by from Volodymyr Babchuk
Changes in V3:
- add __init attribute to gicv3_dist_espi_common_init
- change open-codded eSPI register initialization to the appropriate
gen-mask macro
- fixed formatting for lines with more than 80 symbols
- introduced gicv3_dist_espi_init_aff to be able to use stubs in case of
CONFIG_GICV3_ESPI disabled
- renamed parameter in the GICD_TYPER_ESPI_RANGE macro to espi_range
(name was taken from GIC specification) to avoid confusion
- changed type for i variable to unsigned int since it cannot be
negative
Changes in V2:
- move gic_number_espis function from
[PATCH 08/10] xen/arm: vgic: add resource management for extended SPIs
to use it in the newly introduced gic_is_valid_espi
- add gic_is_valid_espi which checks if IRQ number is in supported
by HW eSPI range
- update gic_is_valid_irq conditions to allow operations with eSPIs
---
xen/arch/arm/gic-v3.c | 83 ++++++++++++++++++++++++++
xen/arch/arm/include/asm/gic.h | 21 ++++++-
xen/arch/arm/include/asm/gic_v3_defs.h | 38 ++++++++++++
3 files changed, 141 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index a1e302fea2..a69263e461 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -485,6 +485,36 @@ static void __iomem *get_addr_by_offset(struct irq_desc *irqd, uint32_t offset)
default:
break;
}
+#ifdef CONFIG_GICV3_ESPI
+ case ESPI_BASE_INTID ... ESPI_MAX_INTID:
+ {
+ uint32_t irq_index = espi_intid_to_idx(irqd->irq);
+
+ switch ( offset )
+ {
+ case GICD_ISENABLER:
+ return (GICD + GICD_ISENABLERnE + (irq_index / 32) * 4);
+ case GICD_ICENABLER:
+ return (GICD + GICD_ICENABLERnE + (irq_index / 32) * 4);
+ case GICD_ISPENDR:
+ return (GICD + GICD_ISPENDRnE + (irq_index / 32) * 4);
+ case GICD_ICPENDR:
+ return (GICD + GICD_ICPENDRnE + (irq_index / 32) * 4);
+ case GICD_ISACTIVER:
+ return (GICD + GICD_ISACTIVERnE + (irq_index / 32) * 4);
+ case GICD_ICACTIVER:
+ return (GICD + GICD_ICACTIVERnE + (irq_index / 32) * 4);
+ case GICD_ICFGR:
+ return (GICD + GICD_ICFGRnE + (irq_index / 16) * 4);
+ case GICD_IROUTER:
+ return (GICD + GICD_IROUTERnE + irq_index * 8);
+ case GICD_IPRIORITYR:
+ return (GICD + GICD_IPRIORITYRnE + irq_index);
+ default:
+ break;
+ }
+ }
+#endif
default:
break;
}
@@ -655,6 +685,55 @@ static void gicv3_set_irq_priority(struct irq_desc *desc,
spin_unlock(&gicv3.lock);
}
+#ifdef CONFIG_GICV3_ESPI
+unsigned int gic_number_espis(void)
+{
+ return gic_hw_ops->info->nr_espi;
+}
+
+static void __init gicv3_dist_espi_common_init(uint32_t type)
+{
+ unsigned int espi_nr, i;
+
+ espi_nr = min(1024U, GICD_TYPER_ESPIS_NUM(type));
+ gicv3_info.nr_espi = espi_nr;
+ /* The GIC HW doesn't support eSPI, so we can leave from here */
+ if ( gicv3_info.nr_espi == 0 )
+ return;
+
+ printk("GICv3: %d eSPI lines\n", gicv3_info.nr_espi);
+
+ /* The configuration for eSPIs is similar to that for regular SPIs */
+ for ( i = 0; i < espi_nr; i += 16 )
+ writel_relaxed(0, GICD + GICD_ICFGRnE + (i / 16) * 4);
+
+ for ( i = 0; i < espi_nr; i += 4 )
+ writel_relaxed(GIC_PRI_IRQ_ALL,
+ GICD + GICD_IPRIORITYRnE + (i / 4) * 4);
+
+ for ( i = 0; i < espi_nr; i += 32 )
+ {
+ writel_relaxed(GENMASK(31, 0), GICD + GICD_ICENABLERnE + (i / 32) * 4);
+ writel_relaxed(GENMASK(31, 0), GICD + GICD_ICACTIVERnE + (i / 32) * 4);
+ }
+
+ for ( i = 0; i < espi_nr; i += 32 )
+ writel_relaxed(GENMASK(31, 0), GICD + GICD_IGROUPRnE + (i / 32) * 4);
+}
+
+static void __init gicv3_dist_espi_init_aff(uint64_t affinity)
+{
+ unsigned int i;
+
+ for ( i = 0; i < gicv3_info.nr_espi; i++ )
+ writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTERnE + i * 8);
+}
+#else
+static void __init gicv3_dist_espi_common_init(uint32_t type) { }
+
+static void __init gicv3_dist_espi_init_aff(uint64_t affinity) { }
+#endif
+
static void __init gicv3_dist_init(void)
{
uint32_t type;
@@ -700,6 +779,8 @@ static void __init gicv3_dist_init(void)
for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i += 32 )
writel_relaxed(GENMASK(31, 0), GICD + GICD_IGROUPR + (i / 32) * 4);
+ gicv3_dist_espi_common_init(type);
+
gicv3_dist_wait_for_rwp();
/* Turn on the distributor */
@@ -713,6 +794,8 @@ static void __init gicv3_dist_init(void)
for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i++ )
writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTER + i * 8);
+
+ gicv3_dist_espi_init_aff(affinity);
}
static int gicv3_enable_redist(void)
diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
index 3fcee42675..3947c8634d 100644
--- a/xen/arch/arm/include/asm/gic.h
+++ b/xen/arch/arm/include/asm/gic.h
@@ -306,9 +306,24 @@ extern void gic_dump_vgic_info(struct vcpu *v);
/* Number of interrupt lines */
extern unsigned int gic_number_lines(void);
+#ifdef CONFIG_GICV3_ESPI
+extern unsigned int gic_number_espis(void);
+
+static inline bool gic_is_valid_espi(unsigned int irq)
+{
+ return irq >= ESPI_BASE_INTID &&
+ irq < espi_idx_to_intid(gic_number_espis());
+}
+#else
+static inline bool gic_is_valid_espi(unsigned int irq)
+{
+ return false;
+}
+#endif
+
static inline bool gic_is_valid_line(unsigned int irq)
{
- return irq < gic_number_lines();
+ return irq < gic_number_lines() || gic_is_valid_espi(irq);
}
static inline bool gic_is_spi(unsigned int irq)
@@ -325,6 +340,10 @@ struct gic_info {
enum gic_version hw_version;
/* Number of GIC lines supported */
unsigned int nr_lines;
+#ifdef CONFIG_GICV3_ESPI
+ /* Number of GIC eSPI supported */
+ unsigned int nr_espi;
+#endif
/* Number of LR registers */
uint8_t nr_lrs;
/* Maintenance irq number */
diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h
index 2af093e774..3370b4cd52 100644
--- a/xen/arch/arm/include/asm/gic_v3_defs.h
+++ b/xen/arch/arm/include/asm/gic_v3_defs.h
@@ -37,6 +37,44 @@
#define GICD_IROUTER1019 (0x7FD8)
#define GICD_PIDR2 (0xFFE8)
+/* Additional registers for GICv3.1 */
+#define GICD_IGROUPRnE (0x1000)
+#define GICD_IGROUPRnEN (0x107C)
+#define GICD_ISENABLERnE (0x1200)
+#define GICD_ISENABLERnEN (0x127C)
+#define GICD_ICENABLERnE (0x1400)
+#define GICD_ICENABLERnEN (0x147C)
+#define GICD_ISPENDRnE (0x1600)
+#define GICD_ISPENDRnEN (0x167C)
+#define GICD_ICPENDRnE (0x1800)
+#define GICD_ICPENDRnEN (0x187C)
+#define GICD_ISACTIVERnE (0x1A00)
+#define GICD_ISACTIVERnEN (0x1A7C)
+#define GICD_ICACTIVERnE (0x1C00)
+#define GICD_ICACTIVERnEN (0x1C7C)
+#define GICD_IPRIORITYRnE (0x2000)
+#define GICD_IPRIORITYRnEN (0x23FC)
+#define GICD_ICFGRnE (0x3000)
+#define GICD_ICFGRnEN (0x30FC)
+#define GICD_IGRPMODRnE (0x3400)
+#define GICD_IGRPMODRnEN (0x347C)
+#define GICD_NSACRnE (0x3600)
+#define GICD_NSACRnEN (0x36FC)
+#define GICD_IROUTERnE (0x8000)
+#define GICD_IROUTERnEN (0x9FFC)
+
+#ifdef CONFIG_GICV3_ESPI
+#define GICD_TYPER_ESPI_SHIFT 8
+#define GICD_TYPER_ESPI_RANGE_SHIFT 27
+#define GICD_TYPER_ESPI_RANGE_MASK (0x1F)
+#define GICD_TYPER_ESPI (1U << GICD_TYPER_ESPI_SHIFT)
+#define GICD_TYPER_ESPI_RANGE(espi_range) ((((espi_range) & \
+ GICD_TYPER_ESPI_RANGE_MASK) + 1) * 32)
+#define GICD_TYPER_ESPIS_NUM(typer) \
+ (((typer) & GICD_TYPER_ESPI) ? \
+ GICD_TYPER_ESPI_RANGE((typer) >> GICD_TYPER_ESPI_RANGE_SHIFT) : 0)
+#endif
+
/* Common between GICD_PIDR2 and GICR_PIDR2 */
#define GIC_PIDR2_ARCH_MASK (0xf0)
#define GIC_PIDR2_ARCH_GICv3 (0x30)
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v6 05/12] xen/arm: gicv3: implement handling of GICv3.1 eSPI
2025-09-03 14:30 ` [PATCH v6 05/12] xen/arm: gicv3: implement handling of GICv3.1 eSPI Leonid Komarianskyi
@ 2025-09-04 14:37 ` Oleksandr Tyshchenko
2025-09-04 15:10 ` Leonid Komarianskyi
0 siblings, 1 reply; 34+ messages in thread
From: Oleksandr Tyshchenko @ 2025-09-04 14:37 UTC (permalink / raw)
To: Leonid Komarianskyi, xen-devel@lists.xenproject.org
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Oleksandr Tyshchenko
On 03.09.25 17:30, Leonid Komarianskyi wrote:
Hello Leonid
> Introduced appropriate register definitions, helper macros,
> and initialization of required GICv3.1 distributor registers
> to support eSPI. This type of interrupt is handled in the
> same way as regular SPI interrupts, with the following
> differences:
>
> 1) eSPIs can have up to 1024 interrupts, starting from the
> beginning of the range, whereas regular SPIs use INTIDs from
> 32 to 1019, totaling 988 interrupts;
> 2) eSPIs start at INTID 4096, necessitating additional interrupt
> index conversion during register operations.
>
> In case if appropriate config is disabled, or GIC HW doesn't
> support eSPI, the existing functionality will remain the same.
>
> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> ---
> Changes in V6:
> - removed unnecessary parentheses in gic_is_valid_espi()
> - updated gic_is_valid_line(): it now verifies the condition irq <
> gic_number_lines() first, as it is more likely that the irq number
> will be from the non-eSPI range
> - minor change: changed the macros ESPI_INTID2IDX and ESPI_IDX2INTID
> into appropriate inline functions introduced in the previous patch
> - added reviewed-by from Oleksandr Tyshchenko
>
> Changes in V5:
> - fixed minor nits, no functional changes: changed u32 to uint32_t and
> added a comment noting that the configuration for eSPIs is the same as
> for regular SPIs
> - removed ifdefs for eSPI-specific offsets to reduce the number of
> ifdefs and code duplication in further changes
> - removed reviewed-by as moving offset from ifdefs requires additional
> confirmation from reviewers
>
> Changes in V4:
> - added offsets for GICD_IGRPMODRnE and GICD_NSACRnE that are required
> for vGIC emulation
> - added a log banner with eSPI information, similar to the one for
> regular SPI
> - added newline after ifdef and before gic_is_valid_line
> - added reviewed-by from Volodymyr Babchuk
>
> Changes in V3:
> - add __init attribute to gicv3_dist_espi_common_init
> - change open-codded eSPI register initialization to the appropriate
> gen-mask macro
> - fixed formatting for lines with more than 80 symbols
> - introduced gicv3_dist_espi_init_aff to be able to use stubs in case of
> CONFIG_GICV3_ESPI disabled
> - renamed parameter in the GICD_TYPER_ESPI_RANGE macro to espi_range
> (name was taken from GIC specification) to avoid confusion
> - changed type for i variable to unsigned int since it cannot be
> negative
>
> Changes in V2:
> - move gic_number_espis function from
> [PATCH 08/10] xen/arm: vgic: add resource management for extended SPIs
> to use it in the newly introduced gic_is_valid_espi
> - add gic_is_valid_espi which checks if IRQ number is in supported
> by HW eSPI range
> - update gic_is_valid_irq conditions to allow operations with eSPIs
> ---
> xen/arch/arm/gic-v3.c | 83 ++++++++++++++++++++++++++
> xen/arch/arm/include/asm/gic.h | 21 ++++++-
> xen/arch/arm/include/asm/gic_v3_defs.h | 38 ++++++++++++
> 3 files changed, 141 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index a1e302fea2..a69263e461 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -485,6 +485,36 @@ static void __iomem *get_addr_by_offset(struct irq_desc *irqd, uint32_t offset)
> default:
> break;
> }
> +#ifdef CONFIG_GICV3_ESPI
> + case ESPI_BASE_INTID ... ESPI_MAX_INTID:
> + {
> + uint32_t irq_index = espi_intid_to_idx(irqd->irq);
> +
> + switch ( offset )
> + {
> + case GICD_ISENABLER:
> + return (GICD + GICD_ISENABLERnE + (irq_index / 32) * 4);
> + case GICD_ICENABLER:
> + return (GICD + GICD_ICENABLERnE + (irq_index / 32) * 4);
> + case GICD_ISPENDR:
> + return (GICD + GICD_ISPENDRnE + (irq_index / 32) * 4);
> + case GICD_ICPENDR:
> + return (GICD + GICD_ICPENDRnE + (irq_index / 32) * 4);
> + case GICD_ISACTIVER:
> + return (GICD + GICD_ISACTIVERnE + (irq_index / 32) * 4);
> + case GICD_ICACTIVER:
> + return (GICD + GICD_ICACTIVERnE + (irq_index / 32) * 4);
> + case GICD_ICFGR:
> + return (GICD + GICD_ICFGRnE + (irq_index / 16) * 4);
> + case GICD_IROUTER:
> + return (GICD + GICD_IROUTERnE + irq_index * 8);
> + case GICD_IPRIORITYR:
> + return (GICD + GICD_IPRIORITYRnE + irq_index);
> + default:
> + break;
> + }
> + }
> +#endif
> default:
> break;
> }
> @@ -655,6 +685,55 @@ static void gicv3_set_irq_priority(struct irq_desc *desc,
> spin_unlock(&gicv3.lock);
> }
>
> +#ifdef CONFIG_GICV3_ESPI
> +unsigned int gic_number_espis(void)
> +{
> + return gic_hw_ops->info->nr_espi;
> +}
> +
> +static void __init gicv3_dist_espi_common_init(uint32_t type)
> +{
> + unsigned int espi_nr, i;
> +
> + espi_nr = min(1024U, GICD_TYPER_ESPIS_NUM(type));
> + gicv3_info.nr_espi = espi_nr;
Sorry, I have just noticed one thing, and gicv3_cpu_init() probably
would be a more correct place to write about it, but since you don't
modify that function (it is not visible in the context), so writing here:
From "Arm IHI 0069H.b (ID041224)"
10.1.2 GICv3.1 extended INTID range support
Note
Arm recommends that Armv8-R AArch64 PEs report ICC_CTLR_EL1.ExtRange==1,
indicating that the GICv3.1 extended SPI and PPI ranges are supported.
Linux driver has an extra check for that:
WARN((gic_data.ppi_nr > 16 || GIC_ESPI_NR != 0) &&
!(gic_read_ctlr() & ICC_CTLR_EL1_ExtRange),
"Distributor has extended ranges, but CPU%d doesn't\n",
smp_processor_id());
added by the following commit:
irqchip/gic-v3: Warn about inconsistent implementations of extended ranges
https://github.com/torvalds/linux/commit/ad5a78d3da81836c88d1f2d53310484462660997
What is your opinion, is it worth having a similar check in Xen?
> + /* The GIC HW doesn't support eSPI, so we can leave from here */
> + if ( gicv3_info.nr_espi == 0 )
> + return;
> +
> + printk("GICv3: %d eSPI lines\n", gicv3_info.nr_espi);
> +
[snip]
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v6 05/12] xen/arm: gicv3: implement handling of GICv3.1 eSPI
2025-09-04 14:37 ` Oleksandr Tyshchenko
@ 2025-09-04 15:10 ` Leonid Komarianskyi
2025-09-04 15:34 ` Oleksandr Tyshchenko
0 siblings, 1 reply; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-04 15:10 UTC (permalink / raw)
To: Oleksandr Tyshchenko, xen-devel@lists.xenproject.org
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Oleksandr Tyshchenko
Hello Oleksandr,
Thank you for your comment.
On 04.09.25 17:37, Oleksandr Tyshchenko wrote:
>
>
> On 03.09.25 17:30, Leonid Komarianskyi wrote:
>
> Hello Leonid
>
>
>> Introduced appropriate register definitions, helper macros,
>> and initialization of required GICv3.1 distributor registers
>> to support eSPI. This type of interrupt is handled in the
>> same way as regular SPI interrupts, with the following
>> differences:
>>
>> 1) eSPIs can have up to 1024 interrupts, starting from the
>> beginning of the range, whereas regular SPIs use INTIDs from
>> 32 to 1019, totaling 988 interrupts;
>> 2) eSPIs start at INTID 4096, necessitating additional interrupt
>> index conversion during register operations.
>>
>> In case if appropriate config is disabled, or GIC HW doesn't
>> support eSPI, the existing functionality will remain the same.
>>
>> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
>> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> ---
>> Changes in V6:
>> - removed unnecessary parentheses in gic_is_valid_espi()
>> - updated gic_is_valid_line(): it now verifies the condition irq <
>> gic_number_lines() first, as it is more likely that the irq number
>> will be from the non-eSPI range
>> - minor change: changed the macros ESPI_INTID2IDX and ESPI_IDX2INTID
>> into appropriate inline functions introduced in the previous patch
>> - added reviewed-by from Oleksandr Tyshchenko
>>
>> Changes in V5:
>> - fixed minor nits, no functional changes: changed u32 to uint32_t and
>> added a comment noting that the configuration for eSPIs is the same as
>> for regular SPIs
>> - removed ifdefs for eSPI-specific offsets to reduce the number of
>> ifdefs and code duplication in further changes
>> - removed reviewed-by as moving offset from ifdefs requires additional
>> confirmation from reviewers
>>
>> Changes in V4:
>> - added offsets for GICD_IGRPMODRnE and GICD_NSACRnE that are required
>> for vGIC emulation
>> - added a log banner with eSPI information, similar to the one for
>> regular SPI
>> - added newline after ifdef and before gic_is_valid_line
>> - added reviewed-by from Volodymyr Babchuk
>>
>> Changes in V3:
>> - add __init attribute to gicv3_dist_espi_common_init
>> - change open-codded eSPI register initialization to the appropriate
>> gen-mask macro
>> - fixed formatting for lines with more than 80 symbols
>> - introduced gicv3_dist_espi_init_aff to be able to use stubs in case of
>> CONFIG_GICV3_ESPI disabled
>> - renamed parameter in the GICD_TYPER_ESPI_RANGE macro to espi_range
>> (name was taken from GIC specification) to avoid confusion
>> - changed type for i variable to unsigned int since it cannot be
>> negative
>>
>> Changes in V2:
>> - move gic_number_espis function from
>> [PATCH 08/10] xen/arm: vgic: add resource management for extended SPIs
>> to use it in the newly introduced gic_is_valid_espi
>> - add gic_is_valid_espi which checks if IRQ number is in supported
>> by HW eSPI range
>> - update gic_is_valid_irq conditions to allow operations with eSPIs
>> ---
>> xen/arch/arm/gic-v3.c | 83 ++++++++++++++++++++++++++
>> xen/arch/arm/include/asm/gic.h | 21 ++++++-
>> xen/arch/arm/include/asm/gic_v3_defs.h | 38 ++++++++++++
>> 3 files changed, 141 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index a1e302fea2..a69263e461 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -485,6 +485,36 @@ static void __iomem *get_addr_by_offset(struct
>> irq_desc *irqd, uint32_t offset)
>> default:
>> break;
>> }
>> +#ifdef CONFIG_GICV3_ESPI
>> + case ESPI_BASE_INTID ... ESPI_MAX_INTID:
>> + {
>> + uint32_t irq_index = espi_intid_to_idx(irqd->irq);
>> +
>> + switch ( offset )
>> + {
>> + case GICD_ISENABLER:
>> + return (GICD + GICD_ISENABLERnE + (irq_index / 32) * 4);
>> + case GICD_ICENABLER:
>> + return (GICD + GICD_ICENABLERnE + (irq_index / 32) * 4);
>> + case GICD_ISPENDR:
>> + return (GICD + GICD_ISPENDRnE + (irq_index / 32) * 4);
>> + case GICD_ICPENDR:
>> + return (GICD + GICD_ICPENDRnE + (irq_index / 32) * 4);
>> + case GICD_ISACTIVER:
>> + return (GICD + GICD_ISACTIVERnE + (irq_index / 32) * 4);
>> + case GICD_ICACTIVER:
>> + return (GICD + GICD_ICACTIVERnE + (irq_index / 32) * 4);
>> + case GICD_ICFGR:
>> + return (GICD + GICD_ICFGRnE + (irq_index / 16) * 4);
>> + case GICD_IROUTER:
>> + return (GICD + GICD_IROUTERnE + irq_index * 8);
>> + case GICD_IPRIORITYR:
>> + return (GICD + GICD_IPRIORITYRnE + irq_index);
>> + default:
>> + break;
>> + }
>> + }
>> +#endif
>> default:
>> break;
>> }
>> @@ -655,6 +685,55 @@ static void gicv3_set_irq_priority(struct
>> irq_desc *desc,
>> spin_unlock(&gicv3.lock);
>> }
>> +#ifdef CONFIG_GICV3_ESPI
>> +unsigned int gic_number_espis(void)
>> +{
>> + return gic_hw_ops->info->nr_espi;
>> +}
>> +
>> +static void __init gicv3_dist_espi_common_init(uint32_t type)
>> +{
>> + unsigned int espi_nr, i;
>> +
>> + espi_nr = min(1024U, GICD_TYPER_ESPIS_NUM(type));
>> + gicv3_info.nr_espi = espi_nr;
>
>
> Sorry, I have just noticed one thing, and gicv3_cpu_init() probably
> would be a more correct place to write about it, but since you don't
> modify that function (it is not visible in the context), so writing here:
>
> From "Arm IHI 0069H.b (ID041224)"
> 10.1.2 GICv3.1 extended INTID range support
>
> Note
> Arm recommends that Armv8-R AArch64 PEs report ICC_CTLR_EL1.ExtRange==1,
> indicating that the GICv3.1 extended SPI and PPI ranges are supported.
>
> Linux driver has an extra check for that:
>
> WARN((gic_data.ppi_nr > 16 || GIC_ESPI_NR != 0) &&
> !(gic_read_ctlr() & ICC_CTLR_EL1_ExtRange),
> "Distributor has extended ranges, but CPU%d doesn't\n",
> smp_processor_id());
>
> added by the following commit:
> irqchip/gic-v3: Warn about inconsistent implementations of extended ranges
> https://eur01.safelinks.protection.outlook.com/?
> url=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fcommit%2Fad5a78d3da81836c88d1f2d53310484462660997&data=05%7C02%7CLeonid_Komarianskyi%40epam.com%7C910bd935bb4f497df12b08ddebc08b2c%7Cb41b72d04e9f4c268a69f949f367c91d%7C1%7C0%7C638925934406096963%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=JjXAx9z0%2Frdf9ul%2Fv79d7q%2Fyb4Mn8twuiRlxYQHE1HA%3D&reserved=0
>
>
> What is your opinion, is it worth having a similar check in Xen?
>
>
I think we can omit adding such a warning for now, if you don't mind. I
will analyze it in more detail and send it as a follow-up patch later if
needed. Would that be okay for you?
>> + /* The GIC HW doesn't support eSPI, so we can leave from here */
>> + if ( gicv3_info.nr_espi == 0 )
>> + return;
>> +
>> + printk("GICv3: %d eSPI lines\n", gicv3_info.nr_espi);
>> +
>
>
> [snip]
>
Best regards,
Leonid
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v6 05/12] xen/arm: gicv3: implement handling of GICv3.1 eSPI
2025-09-04 15:10 ` Leonid Komarianskyi
@ 2025-09-04 15:34 ` Oleksandr Tyshchenko
0 siblings, 0 replies; 34+ messages in thread
From: Oleksandr Tyshchenko @ 2025-09-04 15:34 UTC (permalink / raw)
To: Leonid Komarianskyi, Oleksandr Tyshchenko,
xen-devel@lists.xenproject.org
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
On 04.09.25 18:10, Leonid Komarianskyi wrote:
> Hello Oleksandr,
Hello Leonid
>
> Thank you for your comment.
>
> On 04.09.25 17:37, Oleksandr Tyshchenko wrote:
>>
>>
>> On 03.09.25 17:30, Leonid Komarianskyi wrote:
>>
>> Hello Leonid
>>
>>
>>> Introduced appropriate register definitions, helper macros,
>>> and initialization of required GICv3.1 distributor registers
>>> to support eSPI. This type of interrupt is handled in the
>>> same way as regular SPI interrupts, with the following
>>> differences:
>>>
>>> 1) eSPIs can have up to 1024 interrupts, starting from the
>>> beginning of the range, whereas regular SPIs use INTIDs from
>>> 32 to 1019, totaling 988 interrupts;
>>> 2) eSPIs start at INTID 4096, necessitating additional interrupt
>>> index conversion during register operations.
>>>
>>> In case if appropriate config is disabled, or GIC HW doesn't
>>> support eSPI, the existing functionality will remain the same.
>>>
>>> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
>>> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> ---
>>> Changes in V6:
>>> - removed unnecessary parentheses in gic_is_valid_espi()
>>> - updated gic_is_valid_line(): it now verifies the condition irq <
>>> gic_number_lines() first, as it is more likely that the irq number
>>> will be from the non-eSPI range
>>> - minor change: changed the macros ESPI_INTID2IDX and ESPI_IDX2INTID
>>> into appropriate inline functions introduced in the previous patch
>>> - added reviewed-by from Oleksandr Tyshchenko
>>>
>>> Changes in V5:
>>> - fixed minor nits, no functional changes: changed u32 to uint32_t and
>>> added a comment noting that the configuration for eSPIs is the same as
>>> for regular SPIs
>>> - removed ifdefs for eSPI-specific offsets to reduce the number of
>>> ifdefs and code duplication in further changes
>>> - removed reviewed-by as moving offset from ifdefs requires additional
>>> confirmation from reviewers
>>>
>>> Changes in V4:
>>> - added offsets for GICD_IGRPMODRnE and GICD_NSACRnE that are required
>>> for vGIC emulation
>>> - added a log banner with eSPI information, similar to the one for
>>> regular SPI
>>> - added newline after ifdef and before gic_is_valid_line
>>> - added reviewed-by from Volodymyr Babchuk
>>>
>>> Changes in V3:
>>> - add __init attribute to gicv3_dist_espi_common_init
>>> - change open-codded eSPI register initialization to the appropriate
>>> gen-mask macro
>>> - fixed formatting for lines with more than 80 symbols
>>> - introduced gicv3_dist_espi_init_aff to be able to use stubs in case of
>>> CONFIG_GICV3_ESPI disabled
>>> - renamed parameter in the GICD_TYPER_ESPI_RANGE macro to espi_range
>>> (name was taken from GIC specification) to avoid confusion
>>> - changed type for i variable to unsigned int since it cannot be
>>> negative
>>>
>>> Changes in V2:
>>> - move gic_number_espis function from
>>> [PATCH 08/10] xen/arm: vgic: add resource management for extended SPIs
>>> to use it in the newly introduced gic_is_valid_espi
>>> - add gic_is_valid_espi which checks if IRQ number is in supported
>>> by HW eSPI range
>>> - update gic_is_valid_irq conditions to allow operations with eSPIs
>>> ---
>>> xen/arch/arm/gic-v3.c | 83 ++++++++++++++++++++++++++
>>> xen/arch/arm/include/asm/gic.h | 21 ++++++-
>>> xen/arch/arm/include/asm/gic_v3_defs.h | 38 ++++++++++++
>>> 3 files changed, 141 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>>> index a1e302fea2..a69263e461 100644
>>> --- a/xen/arch/arm/gic-v3.c
>>> +++ b/xen/arch/arm/gic-v3.c
>>> @@ -485,6 +485,36 @@ static void __iomem *get_addr_by_offset(struct
>>> irq_desc *irqd, uint32_t offset)
>>> default:
>>> break;
>>> }
>>> +#ifdef CONFIG_GICV3_ESPI
>>> + case ESPI_BASE_INTID ... ESPI_MAX_INTID:
>>> + {
>>> + uint32_t irq_index = espi_intid_to_idx(irqd->irq);
>>> +
>>> + switch ( offset )
>>> + {
>>> + case GICD_ISENABLER:
>>> + return (GICD + GICD_ISENABLERnE + (irq_index / 32) * 4);
>>> + case GICD_ICENABLER:
>>> + return (GICD + GICD_ICENABLERnE + (irq_index / 32) * 4);
>>> + case GICD_ISPENDR:
>>> + return (GICD + GICD_ISPENDRnE + (irq_index / 32) * 4);
>>> + case GICD_ICPENDR:
>>> + return (GICD + GICD_ICPENDRnE + (irq_index / 32) * 4);
>>> + case GICD_ISACTIVER:
>>> + return (GICD + GICD_ISACTIVERnE + (irq_index / 32) * 4);
>>> + case GICD_ICACTIVER:
>>> + return (GICD + GICD_ICACTIVERnE + (irq_index / 32) * 4);
>>> + case GICD_ICFGR:
>>> + return (GICD + GICD_ICFGRnE + (irq_index / 16) * 4);
>>> + case GICD_IROUTER:
>>> + return (GICD + GICD_IROUTERnE + irq_index * 8);
>>> + case GICD_IPRIORITYR:
>>> + return (GICD + GICD_IPRIORITYRnE + irq_index);
>>> + default:
>>> + break;
>>> + }
>>> + }
>>> +#endif
>>> default:
>>> break;
>>> }
>>> @@ -655,6 +685,55 @@ static void gicv3_set_irq_priority(struct
>>> irq_desc *desc,
>>> spin_unlock(&gicv3.lock);
>>> }
>>> +#ifdef CONFIG_GICV3_ESPI
>>> +unsigned int gic_number_espis(void)
>>> +{
>>> + return gic_hw_ops->info->nr_espi;
>>> +}
>>> +
>>> +static void __init gicv3_dist_espi_common_init(uint32_t type)
>>> +{
>>> + unsigned int espi_nr, i;
>>> +
>>> + espi_nr = min(1024U, GICD_TYPER_ESPIS_NUM(type));
>>> + gicv3_info.nr_espi = espi_nr;
>>
>>
>> Sorry, I have just noticed one thing, and gicv3_cpu_init() probably
>> would be a more correct place to write about it, but since you don't
>> modify that function (it is not visible in the context), so writing here:
>>
>> From "Arm IHI 0069H.b (ID041224)"
>> 10.1.2 GICv3.1 extended INTID range support
>>
>> Note
>> Arm recommends that Armv8-R AArch64 PEs report ICC_CTLR_EL1.ExtRange==1,
>> indicating that the GICv3.1 extended SPI and PPI ranges are supported.
>>
>> Linux driver has an extra check for that:
>>
>> WARN((gic_data.ppi_nr > 16 || GIC_ESPI_NR != 0) &&
>> !(gic_read_ctlr() & ICC_CTLR_EL1_ExtRange),
>> "Distributor has extended ranges, but CPU%d doesn't\n",
>> smp_processor_id());
>>
>> added by the following commit:
>> irqchip/gic-v3: Warn about inconsistent implementations of extended ranges
>> https://eur01.safelinks.protection.outlook.com/?
>> url=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fcommit%2Fad5a78d3da81836c88d1f2d53310484462660997&data=05%7C02%7CLeonid_Komarianskyi%40epam.com%7C910bd935bb4f497df12b08ddebc08b2c%7Cb41b72d04e9f4c268a69f949f367c91d%7C1%7C0%7C638925934406096963%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=JjXAx9z0%2Frdf9ul%2Fv79d7q%2Fyb4Mn8twuiRlxYQHE1HA%3D&reserved=0
>>
>>
>> What is your opinion, is it worth having a similar check in Xen?
>>
>>
>
> I think we can omit adding such a warning for now, if you don't mind. I
> will analyze it in more detail and send it as a follow-up patch later if
> needed. Would that be okay for you?
Sure, I do not mind, it was a question, but not a request for immediate
change.
>
>>> + /* The GIC HW doesn't support eSPI, so we can leave from here */
>>> + if ( gicv3_info.nr_espi == 0 )
>>> + return;
>>> +
>>> + printk("GICv3: %d eSPI lines\n", gicv3_info.nr_espi);
>>> +
>>
>>
>> [snip]
>>
>
> Best regards,
> Leonid
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v6 06/12] xen/arm/irq: allow eSPI processing in the gic_interrupt function
2025-09-03 14:29 [PATCH v6 00/12] Introduce eSPI support Leonid Komarianskyi
` (4 preceding siblings ...)
2025-09-03 14:30 ` [PATCH v6 05/12] xen/arm: gicv3: implement handling of GICv3.1 eSPI Leonid Komarianskyi
@ 2025-09-03 14:30 ` Leonid Komarianskyi
2025-09-03 14:30 ` [PATCH v6 07/12] xen/arm: gicv3: modify ICH_LR_PHYSICAL_MASK to allow eSPI processing Leonid Komarianskyi
` (5 subsequent siblings)
11 siblings, 0 replies; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-03 14:30 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: olekstysh@gmail.com, Leonid Komarianskyi, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel, Volodymyr Babchuk,
Volodymyr Babchuk, Julien Grall
The gic_interrupt function is the main handler for processing IRQs.
Currently, due to restrictive checks, it does not process interrupt
numbers greater than 1019. This patch updates the condition to allow
the handling of interrupts from the eSPI range.
Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
Changes in V6:
- added acked-by from Julien Grall
Changes in V5:
- no changes
Changes in V4:
- fixed commit message
- added reviewed-by from Volodymyr Babchuk
Changes in V3:
- no changes
Changes in V2:
- no changes
---
xen/arch/arm/gic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 9469c9d08c..260ee64cca 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -342,7 +342,7 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
/* Reading IRQ will ACK it */
irq = gic_hw_ops->read_irq();
- if ( likely(irq >= GIC_SGI_STATIC_MAX && irq < 1020) )
+ if ( likely(irq >= GIC_SGI_STATIC_MAX && irq < 1020) || is_espi(irq) )
{
isb();
do_IRQ(regs, irq, is_fiq);
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v6 07/12] xen/arm: gicv3: modify ICH_LR_PHYSICAL_MASK to allow eSPI processing
2025-09-03 14:29 [PATCH v6 00/12] Introduce eSPI support Leonid Komarianskyi
` (5 preceding siblings ...)
2025-09-03 14:30 ` [PATCH v6 06/12] xen/arm/irq: allow eSPI processing in the gic_interrupt function Leonid Komarianskyi
@ 2025-09-03 14:30 ` Leonid Komarianskyi
2025-09-03 20:58 ` Volodymyr Babchuk
2025-09-03 14:30 ` [PATCH v6 08/12] xen/arm: vgic: add resource management for extended SPIs Leonid Komarianskyi
` (4 subsequent siblings)
11 siblings, 1 reply; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-03 14:30 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: olekstysh@gmail.com, Leonid Komarianskyi, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel, Volodymyr Babchuk
To properly deactivate physical eSPI routed to a domain and allow them to
be retriggered after the initial trigger, the LR needs to be updated. The
current implementation ignores interrupts outside the range specified by
the mask 0x3FF, which only covers IRQ numbers up to 1023. To enable
processing of eSPI interrupts, this patch updates the mask to 0x1FFF.
Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
---
Changes in V6:
- updated mask to 0x1fff to avoid confusion
- updated commit message
- removed reviewed-by as new changes requires additional confirmation
from reviewers
Changes in V5:
- no changes
Changes in V4:
- added reviewed-by from Volodymyr Babchuk
Changes in V3:
- no changes
Changes in V2:
- remove unnecessary CONFIG_GICV3_ESPI ifdef guard
---
xen/arch/arm/include/asm/gic_v3_defs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h
index 3370b4cd52..c373b94d19 100644
--- a/xen/arch/arm/include/asm/gic_v3_defs.h
+++ b/xen/arch/arm/include/asm/gic_v3_defs.h
@@ -211,7 +211,7 @@
#define ICH_LR_VIRTUAL_SHIFT 0
#define ICH_LR_CPUID_MASK 0x7
#define ICH_LR_CPUID_SHIFT 10
-#define ICH_LR_PHYSICAL_MASK 0x3ff
+#define ICH_LR_PHYSICAL_MASK 0x1fff
#define ICH_LR_PHYSICAL_SHIFT 32
#define ICH_LR_STATE_MASK 0x3
#define ICH_LR_STATE_SHIFT 62
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v6 07/12] xen/arm: gicv3: modify ICH_LR_PHYSICAL_MASK to allow eSPI processing
2025-09-03 14:30 ` [PATCH v6 07/12] xen/arm: gicv3: modify ICH_LR_PHYSICAL_MASK to allow eSPI processing Leonid Komarianskyi
@ 2025-09-03 20:58 ` Volodymyr Babchuk
0 siblings, 0 replies; 34+ messages in thread
From: Volodymyr Babchuk @ 2025-09-03 20:58 UTC (permalink / raw)
To: Leonid Komarianskyi
Cc: xen-devel@lists.xenproject.org, olekstysh@gmail.com,
Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel
Hi,
Leonid Komarianskyi <Leonid_Komarianskyi@epam.com> writes:
> To properly deactivate physical eSPI routed to a domain and allow them to
> be retriggered after the initial trigger, the LR needs to be updated. The
> current implementation ignores interrupts outside the range specified by
> the mask 0x3FF, which only covers IRQ numbers up to 1023. To enable
> processing of eSPI interrupts, this patch updates the mask to 0x1FFF.
>
> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>
> ---
> Changes in V6:
> - updated mask to 0x1fff to avoid confusion
> - updated commit message
> - removed reviewed-by as new changes requires additional confirmation
> from reviewers
>
> Changes in V5:
> - no changes
>
> Changes in V4:
> - added reviewed-by from Volodymyr Babchuk
>
> Changes in V3:
> - no changes
>
> Changes in V2:
> - remove unnecessary CONFIG_GICV3_ESPI ifdef guard
> ---
> xen/arch/arm/include/asm/gic_v3_defs.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h
> index 3370b4cd52..c373b94d19 100644
> --- a/xen/arch/arm/include/asm/gic_v3_defs.h
> +++ b/xen/arch/arm/include/asm/gic_v3_defs.h
> @@ -211,7 +211,7 @@
> #define ICH_LR_VIRTUAL_SHIFT 0
> #define ICH_LR_CPUID_MASK 0x7
> #define ICH_LR_CPUID_SHIFT 10
> -#define ICH_LR_PHYSICAL_MASK 0x3ff
> +#define ICH_LR_PHYSICAL_MASK 0x1fff
> #define ICH_LR_PHYSICAL_SHIFT 32
> #define ICH_LR_STATE_MASK 0x3
> #define ICH_LR_STATE_SHIFT 62
--
WBR, Volodymyr
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v6 08/12] xen/arm: vgic: add resource management for extended SPIs
2025-09-03 14:29 [PATCH v6 00/12] Introduce eSPI support Leonid Komarianskyi
` (6 preceding siblings ...)
2025-09-03 14:30 ` [PATCH v6 07/12] xen/arm: gicv3: modify ICH_LR_PHYSICAL_MASK to allow eSPI processing Leonid Komarianskyi
@ 2025-09-03 14:30 ` Leonid Komarianskyi
2025-09-03 21:24 ` Volodymyr Babchuk
2025-09-04 18:12 ` Oleksandr Tyshchenko
2025-09-03 14:30 ` [PATCH v6 09/12] xen/arm: domain_build/dom0less-build: adjust domains config to support eSPIs Leonid Komarianskyi
` (3 subsequent siblings)
11 siblings, 2 replies; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-03 14:30 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: olekstysh@gmail.com, Leonid Komarianskyi, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel, Volodymyr Babchuk
This change introduces resource management in the VGIC to handle
extended SPIs introduced in GICv3.1. The pending_irqs and
allocated_irqs arrays are resized to support the required
number of eSPIs, based on what is supported by the hardware and
requested by the guest. A new field, ext_shared_irqs, is added
to the VGIC structure to store information about eSPIs, similar
to how shared_irqs is used for regular SPIs.
Since the eSPI range starts at INTID 4096 and INTIDs between 1025
and 4095 are reserved, helper macros are introduced to simplify the
transformation of indices and to enable easier access to eSPI-specific
resources. These changes prepare the VGIC for processing eSPIs as
required by future functionality.
The initialization and deinitialization paths for vgic have been updated
to allocate and free these resources appropriately. Additionally,
updated handling of INTIDs greater than 1024, passed from the toolstack
during domain creation, and verification logic ensures only valid SPI or
eSPI INTIDs are used.
The existing SPI behavior remains unaffected when guests do not request
eSPIs, GIC hardware does not support them, or the CONFIG_GICV3_ESPI
option is disabled.
Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
---
Changes for V6:
- introduced a new function for index to virq conversion, idx_to_virq.
This allows the removal of eSPI-specific functions and enables the
reuse of existing code for regular SPIs
- fixed the return value of vgic_allocate_virq in the case of eSPI
- updated the error message in route_irq_to_guest(). To simplify it and
avoid overcomplicating with INTID ranges, propose removing the
information about the max number of IRQs
- fixed eSPI rank initialization to avoid using EXT_RANK_IDX2NUM for
converting the eSPI rank to the extended range
- updated the recalculation logic to avoid the nr_spis > 1020 -
NR_LOCAL_IRQS check when nr_spis is reassigned in the case of eSPI
- fixed formatting issues for macros
- minor change: changed the macros ESPI_INTID2IDX and ESPI_IDX2INTID
into appropriate inline functions introduced in the previous patch
- minor change: changed int to unsigned int for nr_espis
Changes in V5:
- removed the has_espi field because it can be determined by checking
whether domain->arch.vgic.nr_espis is zero or not
- since vgic_ext_rank_offset is not used in this patch, it has been
moved to the appropriate patch in the patch series, which implements
vgic eSPI registers emulation and requires this function
- removed ifdefs for eSPI-specific macros to reduce the number of ifdefs
and code duplication in further changes
- fixed minor nit: used %pd for printing domain with its ID
Changes in V4:
- added has_espi field to simplify determining whether a domain is able
to operate with eSPI
- fixed formatting issues and misspellings
Changes in V3:
- fixed formatting for lines with more than 80 symbols
- introduced helper functions to be able to use stubs in case of
CONFIG_GICV3_ESPI disabled, and as a result, reduce the number of
#ifdefs
- fixed checks for nr_spis in domain_vgic_init
- updated comment about nr_spis adjustments with dom0less mention
- moved comment with additional explanations before checks
- used unsigned int for indexes since they cannot be negative
- removed unnecessary parentheses
- move vgic_ext_rank_offset to the below ifdef guard, to reduce the
number of ifdefs
Changes in V2:
- change is_espi_rank to is_valid_espi_rank to verify whether the array
element ext_shared_irqs exists. The previous version, is_espi_rank,
only checked if the rank index was less than the maximum possible eSPI
rank index, but this could potentially result in accessing a
non-existing array element. To address this, is_valid_espi_rank was
introduced, which ensures that the required eSPI rank exists
- move gic_number_espis to
xen/arm: gicv3: implement handling of GICv3.1 eSPI
- update vgic_is_valid_irq checks to allow operating with eSPIs
- remove redundant newline in vgic_allocate_virq
---
xen/arch/arm/include/asm/vgic.h | 12 +++
xen/arch/arm/irq.c | 3 +-
xen/arch/arm/vgic.c | 174 ++++++++++++++++++++++++++++++--
3 files changed, 176 insertions(+), 13 deletions(-)
diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
index 3e7cbbb196..1cf0a05832 100644
--- a/xen/arch/arm/include/asm/vgic.h
+++ b/xen/arch/arm/include/asm/vgic.h
@@ -146,6 +146,10 @@ struct vgic_dist {
int nr_spis; /* Number of SPIs */
unsigned long *allocated_irqs; /* bitmap of IRQs allocated */
struct vgic_irq_rank *shared_irqs;
+#ifdef CONFIG_GICV3_ESPI
+ struct vgic_irq_rank *ext_shared_irqs;
+ unsigned int nr_espis; /* Number of extended SPIs */
+#endif
/*
* SPIs are domain global, SGIs and PPIs are per-VCPU and stored in
* struct arch_vcpu.
@@ -243,6 +247,14 @@ struct vgic_ops {
/* Number of ranks of interrupt registers for a domain */
#define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_spis+31)/32)
+#ifdef CONFIG_GICV3_ESPI
+#define DOMAIN_NR_EXT_RANKS(d) (((d)->arch.vgic.nr_espis + 31) / 32)
+#endif
+#define EXT_RANK_MIN (ESPI_BASE_INTID / 32)
+#define EXT_RANK_MAX ((ESPI_MAX_INTID + 31) / 32)
+#define EXT_RANK_NUM2IDX(num) ((num) - EXT_RANK_MIN)
+#define EXT_RANK_IDX2NUM(idx) ((idx) + EXT_RANK_MIN)
+
#define vgic_lock(v) spin_lock_irq(&(v)->domain->arch.vgic.lock)
#define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index c934d39bf6..c2f1ceb91f 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -494,8 +494,7 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
if ( !vgic_is_valid_line(d, virq) )
{
printk(XENLOG_G_ERR
- "the vIRQ number %u is too high for domain %u (max = %u)\n",
- irq, d->domain_id, vgic_num_irqs(d));
+ "invalid vIRQ number %u for domain %pd\n", irq, d);
return -EINVAL;
}
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 2bbf4d99aa..b42aee8d0c 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -25,11 +25,61 @@
#include <asm/vgic.h>
+static inline unsigned int idx_to_virq(struct domain *d, unsigned int idx)
+{
+ if ( idx >= vgic_num_irqs(d) )
+ return espi_idx_to_intid(idx - vgic_num_irqs(d));
+
+ return idx;
+}
+
bool vgic_is_valid_line(struct domain *d, unsigned int virq)
{
+#ifdef CONFIG_GICV3_ESPI
+ if ( virq >= ESPI_BASE_INTID &&
+ virq < espi_idx_to_intid(d->arch.vgic.nr_espis) )
+ return true;
+#endif
+
return virq < vgic_num_irqs(d);
}
+#ifdef CONFIG_GICV3_ESPI
+/*
+ * Since eSPI indexes start from 4096 and numbers from 1024 to
+ * 4095 are forbidden, we need to check both lower and upper
+ * limits for ranks.
+ */
+static inline bool is_valid_espi_rank(struct domain *d, unsigned int rank)
+{
+ return rank >= EXT_RANK_MIN &&
+ EXT_RANK_NUM2IDX(rank) < DOMAIN_NR_EXT_RANKS(d);
+}
+
+static inline struct vgic_irq_rank *vgic_get_espi_rank(struct vcpu *v,
+ unsigned int rank)
+{
+ return &v->domain->arch.vgic.ext_shared_irqs[EXT_RANK_NUM2IDX(rank)];
+}
+
+#else
+static inline bool is_valid_espi_rank(struct domain *d, unsigned int rank)
+{
+ return false;
+}
+
+/*
+ * This function is stub and will not be called if CONFIG_GICV3_ESPI=n,
+ * because in this case, is_valid_espi_rank will always return false.
+ */
+static inline struct vgic_irq_rank *vgic_get_espi_rank(struct vcpu *v,
+ unsigned int rank)
+{
+ ASSERT_UNREACHABLE();
+ return NULL;
+}
+#endif
+
static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
unsigned int rank)
{
@@ -37,6 +87,8 @@ static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
return v->arch.vgic.private_irqs;
else if ( rank <= DOMAIN_NR_RANKS(v->domain) )
return &v->domain->arch.vgic.shared_irqs[rank - 1];
+ else if ( is_valid_espi_rank(v->domain, rank) )
+ return vgic_get_espi_rank(v, rank);
else
return NULL;
}
@@ -117,6 +169,54 @@ int domain_vgic_register(struct domain *d, unsigned int *mmio_count)
return 0;
}
+#ifdef CONFIG_GICV3_ESPI
+static unsigned int vgic_num_spi_lines(struct domain *d)
+{
+ return d->arch.vgic.nr_spis + d->arch.vgic.nr_espis;
+}
+
+static int init_vgic_espi(struct domain *d)
+{
+ unsigned int i, idx;
+
+ if ( d->arch.vgic.nr_espis == 0 )
+ return 0;
+
+ d->arch.vgic.ext_shared_irqs =
+ xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_EXT_RANKS(d));
+ if ( d->arch.vgic.ext_shared_irqs == NULL )
+ return -ENOMEM;
+
+ for ( i = d->arch.vgic.nr_spis, idx = 0;
+ i < vgic_num_spi_lines(d); i++, idx++ )
+ vgic_init_pending_irq(&d->arch.vgic.pending_irqs[i],
+ espi_idx_to_intid(idx));
+
+ for ( i = 0; i < DOMAIN_NR_EXT_RANKS(d); i++ )
+ vgic_rank_init(&d->arch.vgic.ext_shared_irqs[i],
+ EXT_RANK_IDX2NUM(i), 0);
+
+ return 0;
+}
+
+#else
+static int init_vgic_espi(struct domain *d)
+{
+ return 0;
+}
+
+static unsigned int vgic_num_spi_lines(struct domain *d)
+{
+ return d->arch.vgic.nr_spis;
+}
+
+#endif
+
+static unsigned int vgic_num_alloc_irqs(struct domain *d)
+{
+ return vgic_num_spi_lines(d) + NR_LOCAL_IRQS;
+}
+
int domain_vgic_init(struct domain *d, unsigned int nr_spis)
{
int i;
@@ -133,7 +233,39 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
/* Limit the number of virtual SPIs supported to (1020 - 32) = 988 */
if ( nr_spis > (1020 - NR_LOCAL_IRQS) )
+#ifndef CONFIG_GICV3_ESPI
return -EINVAL;
+#else
+ {
+ /*
+ * During domain creation, the dom0less DomUs code or toolstack
+ * specifies the maximum INTID, which is defined in the domain
+ * config subtracted by 32 to cover the local IRQs (please see
+ * the comment to VGIC_DEF_NR_SPIS). To compute the actual number
+ * of eSPI that will be usable for, add back 32.
+ */
+ nr_spis += 32;
+ if ( nr_spis > espi_idx_to_intid(NR_ESPI_IRQS) )
+ return -EINVAL;
+
+ if ( nr_spis >= ESPI_BASE_INTID )
+ {
+ unsigned int nr_espis = min(nr_spis - ESPI_BASE_INTID, 1024U);
+
+ /* Verify if GIC HW can handle provided INTID */
+ if ( nr_espis > gic_number_espis() )
+ return -EINVAL;
+
+ d->arch.vgic.nr_espis = nr_espis;
+ /* Set the maximum available number for regular SPIs */
+ nr_spis = VGIC_DEF_NR_SPIS;
+ }
+ else
+ {
+ return -EINVAL;
+ }
+ }
+#endif
d->arch.vgic.nr_spis = nr_spis;
@@ -145,7 +277,7 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
return -ENOMEM;
d->arch.vgic.pending_irqs =
- xzalloc_array(struct pending_irq, d->arch.vgic.nr_spis);
+ xzalloc_array(struct pending_irq, vgic_num_spi_lines(d));
if ( d->arch.vgic.pending_irqs == NULL )
return -ENOMEM;
@@ -156,12 +288,16 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
vgic_rank_init(&d->arch.vgic.shared_irqs[i], i + 1, 0);
+ ret = init_vgic_espi(d);
+ if ( ret )
+ return ret;
+
ret = d->arch.vgic.handler->domain_init(d);
if ( ret )
return ret;
d->arch.vgic.allocated_irqs =
- xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d)));
+ xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_alloc_irqs(d)));
if ( !d->arch.vgic.allocated_irqs )
return -ENOMEM;
@@ -182,9 +318,12 @@ void domain_vgic_free(struct domain *d)
int i;
int ret;
- for ( i = 0; i < (d->arch.vgic.nr_spis); i++ )
+ for ( i = 32; i < vgic_num_alloc_irqs(d); i++ )
{
- struct pending_irq *p = spi_to_pending(d, i + 32);
+ struct pending_irq *p;
+ unsigned int virq = idx_to_virq(d, i);
+
+ p = spi_to_pending(d, virq);
if ( p->desc )
{
@@ -198,6 +337,9 @@ void domain_vgic_free(struct domain *d)
if ( d->arch.vgic.handler )
d->arch.vgic.handler->domain_free(d);
xfree(d->arch.vgic.shared_irqs);
+#ifdef CONFIG_GICV3_ESPI
+ xfree(d->arch.vgic.ext_shared_irqs);
+#endif
xfree(d->arch.vgic.pending_irqs);
xfree(d->arch.vgic.allocated_irqs);
}
@@ -323,10 +465,12 @@ void arch_move_irqs(struct vcpu *v)
*/
ASSERT(!is_lpi(vgic_num_irqs(d) - 1));
- for ( i = 32; i < vgic_num_irqs(d); i++ )
+ for ( i = 32; i < vgic_num_alloc_irqs(d); i++ )
{
- v_target = vgic_get_target_vcpu(v, i);
- p = irq_to_pending(v_target, i);
+ unsigned int virq = idx_to_virq(d, i);
+
+ v_target = vgic_get_target_vcpu(v, virq);
+ p = irq_to_pending(v_target, virq);
if ( v_target == v && !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
irq_set_affinity(p->desc, cpu_mask);
@@ -539,7 +683,7 @@ struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
else if ( is_lpi(irq) )
n = v->domain->arch.vgic.handler->lpi_to_pending(v->domain, irq);
else
- n = &v->domain->arch.vgic.pending_irqs[irq - 32];
+ n = spi_to_pending(v->domain, irq);
return n;
}
@@ -547,7 +691,12 @@ struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq)
{
ASSERT(irq >= NR_LOCAL_IRQS);
- return &d->arch.vgic.pending_irqs[irq - 32];
+ if ( is_espi(irq) )
+ irq = espi_intid_to_idx(irq) + d->arch.vgic.nr_spis;
+ else
+ irq -= 32;
+
+ return &d->arch.vgic.pending_irqs[irq];
}
void vgic_clear_pending_irqs(struct vcpu *v)
@@ -668,6 +817,9 @@ bool vgic_reserve_virq(struct domain *d, unsigned int virq)
if ( !vgic_is_valid_line(d, virq) )
return false;
+ if ( is_espi(virq) )
+ virq = espi_intid_to_idx(virq) + vgic_num_irqs(d);
+
return !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
}
@@ -685,7 +837,7 @@ int vgic_allocate_virq(struct domain *d, bool spi)
else
{
first = 32;
- end = vgic_num_irqs(d);
+ end = vgic_num_alloc_irqs(d);
}
/*
@@ -700,7 +852,7 @@ int vgic_allocate_virq(struct domain *d, bool spi)
}
while ( test_and_set_bit(virq, d->arch.vgic.allocated_irqs) );
- return virq;
+ return idx_to_virq(d, virq);
}
void vgic_free_virq(struct domain *d, unsigned int virq)
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v6 08/12] xen/arm: vgic: add resource management for extended SPIs
2025-09-03 14:30 ` [PATCH v6 08/12] xen/arm: vgic: add resource management for extended SPIs Leonid Komarianskyi
@ 2025-09-03 21:24 ` Volodymyr Babchuk
2025-09-04 6:20 ` Leonid Komarianskyi
2025-09-04 18:12 ` Oleksandr Tyshchenko
1 sibling, 1 reply; 34+ messages in thread
From: Volodymyr Babchuk @ 2025-09-03 21:24 UTC (permalink / raw)
To: Leonid Komarianskyi
Cc: xen-devel@lists.xenproject.org, olekstysh@gmail.com,
Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel
Hi Leonid,
Leonid Komarianskyi <Leonid_Komarianskyi@epam.com> writes:
> This change introduces resource management in the VGIC to handle
> extended SPIs introduced in GICv3.1. The pending_irqs and
> allocated_irqs arrays are resized to support the required
> number of eSPIs, based on what is supported by the hardware and
> requested by the guest. A new field, ext_shared_irqs, is added
> to the VGIC structure to store information about eSPIs, similar
> to how shared_irqs is used for regular SPIs.
>
> Since the eSPI range starts at INTID 4096 and INTIDs between 1025
> and 4095 are reserved, helper macros are introduced to simplify the
> transformation of indices and to enable easier access to eSPI-specific
> resources. These changes prepare the VGIC for processing eSPIs as
> required by future functionality.
>
> The initialization and deinitialization paths for vgic have been updated
> to allocate and free these resources appropriately. Additionally,
> updated handling of INTIDs greater than 1024, passed from the toolstack
> during domain creation, and verification logic ensures only valid SPI or
> eSPI INTIDs are used.
>
> The existing SPI behavior remains unaffected when guests do not request
> eSPIs, GIC hardware does not support them, or the CONFIG_GICV3_ESPI
> option is disabled.
>
> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
>
> ---
> Changes for V6:
> - introduced a new function for index to virq conversion, idx_to_virq.
> This allows the removal of eSPI-specific functions and enables the
> reuse of existing code for regular SPIs
> - fixed the return value of vgic_allocate_virq in the case of eSPI
> - updated the error message in route_irq_to_guest(). To simplify it and
> avoid overcomplicating with INTID ranges, propose removing the
> information about the max number of IRQs
> - fixed eSPI rank initialization to avoid using EXT_RANK_IDX2NUM for
> converting the eSPI rank to the extended range
> - updated the recalculation logic to avoid the nr_spis > 1020 -
> NR_LOCAL_IRQS check when nr_spis is reassigned in the case of eSPI
> - fixed formatting issues for macros
> - minor change: changed the macros ESPI_INTID2IDX and ESPI_IDX2INTID
> into appropriate inline functions introduced in the previous patch
> - minor change: changed int to unsigned int for nr_espis
>
> Changes in V5:
> - removed the has_espi field because it can be determined by checking
> whether domain->arch.vgic.nr_espis is zero or not
> - since vgic_ext_rank_offset is not used in this patch, it has been
> moved to the appropriate patch in the patch series, which implements
> vgic eSPI registers emulation and requires this function
> - removed ifdefs for eSPI-specific macros to reduce the number of ifdefs
> and code duplication in further changes
> - fixed minor nit: used %pd for printing domain with its ID
>
> Changes in V4:
> - added has_espi field to simplify determining whether a domain is able
> to operate with eSPI
> - fixed formatting issues and misspellings
>
> Changes in V3:
> - fixed formatting for lines with more than 80 symbols
> - introduced helper functions to be able to use stubs in case of
> CONFIG_GICV3_ESPI disabled, and as a result, reduce the number of
> #ifdefs
> - fixed checks for nr_spis in domain_vgic_init
> - updated comment about nr_spis adjustments with dom0less mention
> - moved comment with additional explanations before checks
> - used unsigned int for indexes since they cannot be negative
> - removed unnecessary parentheses
> - move vgic_ext_rank_offset to the below ifdef guard, to reduce the
> number of ifdefs
>
> Changes in V2:
> - change is_espi_rank to is_valid_espi_rank to verify whether the array
> element ext_shared_irqs exists. The previous version, is_espi_rank,
> only checked if the rank index was less than the maximum possible eSPI
> rank index, but this could potentially result in accessing a
> non-existing array element. To address this, is_valid_espi_rank was
> introduced, which ensures that the required eSPI rank exists
> - move gic_number_espis to
> xen/arm: gicv3: implement handling of GICv3.1 eSPI
> - update vgic_is_valid_irq checks to allow operating with eSPIs
> - remove redundant newline in vgic_allocate_virq
> ---
> xen/arch/arm/include/asm/vgic.h | 12 +++
> xen/arch/arm/irq.c | 3 +-
> xen/arch/arm/vgic.c | 174 ++++++++++++++++++++++++++++++--
> 3 files changed, 176 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
> index 3e7cbbb196..1cf0a05832 100644
> --- a/xen/arch/arm/include/asm/vgic.h
> +++ b/xen/arch/arm/include/asm/vgic.h
> @@ -146,6 +146,10 @@ struct vgic_dist {
> int nr_spis; /* Number of SPIs */
> unsigned long *allocated_irqs; /* bitmap of IRQs allocated */
> struct vgic_irq_rank *shared_irqs;
> +#ifdef CONFIG_GICV3_ESPI
> + struct vgic_irq_rank *ext_shared_irqs;
> + unsigned int nr_espis; /* Number of extended SPIs */
> +#endif
> /*
> * SPIs are domain global, SGIs and PPIs are per-VCPU and stored in
> * struct arch_vcpu.
> @@ -243,6 +247,14 @@ struct vgic_ops {
> /* Number of ranks of interrupt registers for a domain */
> #define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_spis+31)/32)
>
> +#ifdef CONFIG_GICV3_ESPI
> +#define DOMAIN_NR_EXT_RANKS(d) (((d)->arch.vgic.nr_espis + 31) / 32)
> +#endif
> +#define EXT_RANK_MIN (ESPI_BASE_INTID / 32)
> +#define EXT_RANK_MAX ((ESPI_MAX_INTID + 31) / 32)
> +#define EXT_RANK_NUM2IDX(num) ((num) - EXT_RANK_MIN)
> +#define EXT_RANK_IDX2NUM(idx) ((idx) + EXT_RANK_MIN)
> +
> #define vgic_lock(v) spin_lock_irq(&(v)->domain->arch.vgic.lock)
> #define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)
>
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index c934d39bf6..c2f1ceb91f 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -494,8 +494,7 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
> if ( !vgic_is_valid_line(d, virq) )
> {
> printk(XENLOG_G_ERR
> - "the vIRQ number %u is too high for domain %u (max = %u)\n",
> - irq, d->domain_id, vgic_num_irqs(d));
> + "invalid vIRQ number %u for domain %pd\n", irq, d);
> return -EINVAL;
> }
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 2bbf4d99aa..b42aee8d0c 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -25,11 +25,61 @@
> #include <asm/vgic.h>
>
>
> +static inline unsigned int idx_to_virq(struct domain *d, unsigned int idx)
> +{
> + if ( idx >= vgic_num_irqs(d) )
> + return espi_idx_to_intid(idx - vgic_num_irqs(d));
> +
> + return idx;
> +}
> +
> bool vgic_is_valid_line(struct domain *d, unsigned int virq)
> {
> +#ifdef CONFIG_GICV3_ESPI
> + if ( virq >= ESPI_BASE_INTID &&
> + virq < espi_idx_to_intid(d->arch.vgic.nr_espis) )
> + return true;
> +#endif
> +
> return virq < vgic_num_irqs(d);
> }
>
> +#ifdef CONFIG_GICV3_ESPI
> +/*
> + * Since eSPI indexes start from 4096 and numbers from 1024 to
> + * 4095 are forbidden, we need to check both lower and upper
> + * limits for ranks.
> + */
> +static inline bool is_valid_espi_rank(struct domain *d, unsigned int rank)
> +{
> + return rank >= EXT_RANK_MIN &&
> + EXT_RANK_NUM2IDX(rank) < DOMAIN_NR_EXT_RANKS(d);
> +}
> +
> +static inline struct vgic_irq_rank *vgic_get_espi_rank(struct vcpu *v,
> + unsigned int rank)
> +{
> + return &v->domain->arch.vgic.ext_shared_irqs[EXT_RANK_NUM2IDX(rank)];
> +}
> +
> +#else
> +static inline bool is_valid_espi_rank(struct domain *d, unsigned int rank)
> +{
> + return false;
> +}
> +
> +/*
> + * This function is stub and will not be called if CONFIG_GICV3_ESPI=n,
> + * because in this case, is_valid_espi_rank will always return false.
> + */
> +static inline struct vgic_irq_rank *vgic_get_espi_rank(struct vcpu *v,
> + unsigned int rank)
> +{
> + ASSERT_UNREACHABLE();
> + return NULL;
> +}
> +#endif
> +
> static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
> unsigned int rank)
> {
> @@ -37,6 +87,8 @@ static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
> return v->arch.vgic.private_irqs;
> else if ( rank <= DOMAIN_NR_RANKS(v->domain) )
> return &v->domain->arch.vgic.shared_irqs[rank - 1];
> + else if ( is_valid_espi_rank(v->domain, rank) )
> + return vgic_get_espi_rank(v, rank);
> else
> return NULL;
> }
> @@ -117,6 +169,54 @@ int domain_vgic_register(struct domain *d, unsigned int *mmio_count)
> return 0;
> }
>
> +#ifdef CONFIG_GICV3_ESPI
> +static unsigned int vgic_num_spi_lines(struct domain *d)
> +{
> + return d->arch.vgic.nr_spis + d->arch.vgic.nr_espis;
> +}
> +
> +static int init_vgic_espi(struct domain *d)
> +{
> + unsigned int i, idx;
> +
> + if ( d->arch.vgic.nr_espis == 0 )
> + return 0;
> +
> + d->arch.vgic.ext_shared_irqs =
> + xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_EXT_RANKS(d));
> + if ( d->arch.vgic.ext_shared_irqs == NULL )
> + return -ENOMEM;
> +
> + for ( i = d->arch.vgic.nr_spis, idx = 0;
> + i < vgic_num_spi_lines(d); i++, idx++ )
> + vgic_init_pending_irq(&d->arch.vgic.pending_irqs[i],
> + espi_idx_to_intid(idx));
> +
> + for ( i = 0; i < DOMAIN_NR_EXT_RANKS(d); i++ )
> + vgic_rank_init(&d->arch.vgic.ext_shared_irqs[i],
> + EXT_RANK_IDX2NUM(i), 0);
> +
> + return 0;
> +}
> +
> +#else
> +static int init_vgic_espi(struct domain *d)
> +{
> + return 0;
> +}
> +
> +static unsigned int vgic_num_spi_lines(struct domain *d)
> +{
> + return d->arch.vgic.nr_spis;
> +}
> +
> +#endif
> +
> +static unsigned int vgic_num_alloc_irqs(struct domain *d)
> +{
> + return vgic_num_spi_lines(d) + NR_LOCAL_IRQS;
This is good that you are using NR_LOCAL_IRQS here.
> +}
> +
> int domain_vgic_init(struct domain *d, unsigned int nr_spis)
> {
> int i;
> @@ -133,7 +233,39 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
>
> /* Limit the number of virtual SPIs supported to (1020 - 32) = 988 */
> if ( nr_spis > (1020 - NR_LOCAL_IRQS) )
> +#ifndef CONFIG_GICV3_ESPI
> return -EINVAL;
> +#else
> + {
> + /*
> + * During domain creation, the dom0less DomUs code or toolstack
> + * specifies the maximum INTID, which is defined in the domain
> + * config subtracted by 32 to cover the local IRQs (please see
> + * the comment to VGIC_DEF_NR_SPIS). To compute the actual number
> + * of eSPI that will be usable for, add back 32.
> + */
> + nr_spis += 32;
I think that it is better to use NR_LOCAL_IRQS here.
(Also, I just noticed the same problem as with documentation, meaning
that nr_spis actually does not represent number of SPIs anymore, and
behaves more like max_spi, but let's set this issue aside for now)
> + if ( nr_spis > espi_idx_to_intid(NR_ESPI_IRQS) )
> + return -EINVAL;
> +
> + if ( nr_spis >= ESPI_BASE_INTID )
> + {
> + unsigned int nr_espis = min(nr_spis - ESPI_BASE_INTID, 1024U);
> +
> + /* Verify if GIC HW can handle provided INTID */
> + if ( nr_espis > gic_number_espis() )
> + return -EINVAL;
> +
> + d->arch.vgic.nr_espis = nr_espis;
> + /* Set the maximum available number for regular SPIs */
> + nr_spis = VGIC_DEF_NR_SPIS;
> + }
> + else
> + {
> + return -EINVAL;
> + }
> + }
> +#endif
>
> d->arch.vgic.nr_spis = nr_spis;
>
> @@ -145,7 +277,7 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
> return -ENOMEM;
>
> d->arch.vgic.pending_irqs =
> - xzalloc_array(struct pending_irq, d->arch.vgic.nr_spis);
> + xzalloc_array(struct pending_irq, vgic_num_spi_lines(d));
> if ( d->arch.vgic.pending_irqs == NULL )
> return -ENOMEM;
>
> @@ -156,12 +288,16 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
> for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
> vgic_rank_init(&d->arch.vgic.shared_irqs[i], i + 1, 0);
>
> + ret = init_vgic_espi(d);
> + if ( ret )
> + return ret;
> +
> ret = d->arch.vgic.handler->domain_init(d);
> if ( ret )
> return ret;
>
> d->arch.vgic.allocated_irqs =
> - xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d)));
> + xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_alloc_irqs(d)));
> if ( !d->arch.vgic.allocated_irqs )
> return -ENOMEM;
>
> @@ -182,9 +318,12 @@ void domain_vgic_free(struct domain *d)
> int i;
> int ret;
>
> - for ( i = 0; i < (d->arch.vgic.nr_spis); i++ )
> + for ( i = 32; i < vgic_num_alloc_irqs(d); i++ )
s/32/NR_LOCAL_IRQS
> {
> - struct pending_irq *p = spi_to_pending(d, i + 32);
> + struct pending_irq *p;
> + unsigned int virq = idx_to_virq(d, i);
> +
> + p = spi_to_pending(d, virq);
>
> if ( p->desc )
> {
> @@ -198,6 +337,9 @@ void domain_vgic_free(struct domain *d)
> if ( d->arch.vgic.handler )
> d->arch.vgic.handler->domain_free(d);
> xfree(d->arch.vgic.shared_irqs);
> +#ifdef CONFIG_GICV3_ESPI
> + xfree(d->arch.vgic.ext_shared_irqs);
> +#endif
> xfree(d->arch.vgic.pending_irqs);
> xfree(d->arch.vgic.allocated_irqs);
> }
> @@ -323,10 +465,12 @@ void arch_move_irqs(struct vcpu *v)
> */
> ASSERT(!is_lpi(vgic_num_irqs(d) - 1));
>
> - for ( i = 32; i < vgic_num_irqs(d); i++ )
> + for ( i = 32; i < vgic_num_alloc_irqs(d); i++ )
It is great idea to start using NR_LOCAL_IRQS here instead of hard-coded 32.
> {
> - v_target = vgic_get_target_vcpu(v, i);
> - p = irq_to_pending(v_target, i);
> + unsigned int virq = idx_to_virq(d, i);
> +
> + v_target = vgic_get_target_vcpu(v, virq);
> + p = irq_to_pending(v_target, virq);
>
> if ( v_target == v && !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
> irq_set_affinity(p->desc, cpu_mask);
> @@ -539,7 +683,7 @@ struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
> else if ( is_lpi(irq) )
> n = v->domain->arch.vgic.handler->lpi_to_pending(v->domain, irq);
> else
> - n = &v->domain->arch.vgic.pending_irqs[irq - 32];
> + n = spi_to_pending(v->domain, irq);
> return n;
> }
>
> @@ -547,7 +691,12 @@ struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq)
> {
> ASSERT(irq >= NR_LOCAL_IRQS);
>
> - return &d->arch.vgic.pending_irqs[irq - 32];
> + if ( is_espi(irq) )
> + irq = espi_intid_to_idx(irq) + d->arch.vgic.nr_spis;
> + else
> + irq -= 32;
s/32/NR_LOCAL_IRQS
Also, I want you to consider adding local variable "idx" instead of
reusing "irq" parameter.
> +
> + return &d->arch.vgic.pending_irqs[irq];
> }
>
> void vgic_clear_pending_irqs(struct vcpu *v)
> @@ -668,6 +817,9 @@ bool vgic_reserve_virq(struct domain *d, unsigned int virq)
> if ( !vgic_is_valid_line(d, virq) )
> return false;
>
> + if ( is_espi(virq) )
> + virq = espi_intid_to_idx(virq) + vgic_num_irqs(d);
Here as well. It is very confusing when you are updating virq with a
value that does not corresponds in IRQ number anymore.
> +
> return !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
> }
>
> @@ -685,7 +837,7 @@ int vgic_allocate_virq(struct domain *d, bool spi)
> else
> {
> first = 32;
> - end = vgic_num_irqs(d);
> + end = vgic_num_alloc_irqs(d);
> }
>
> /*
> @@ -700,7 +852,7 @@ int vgic_allocate_virq(struct domain *d, bool spi)
> }
> while ( test_and_set_bit(virq, d->arch.vgic.allocated_irqs) );
>
> - return virq;
> + return idx_to_virq(d, virq);
> }
>
> void vgic_free_virq(struct domain *d, unsigned int virq)
--
WBR, Volodymyr
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v6 08/12] xen/arm: vgic: add resource management for extended SPIs
2025-09-03 21:24 ` Volodymyr Babchuk
@ 2025-09-04 6:20 ` Leonid Komarianskyi
0 siblings, 0 replies; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-04 6:20 UTC (permalink / raw)
To: Volodymyr Babchuk
Cc: xen-devel@lists.xenproject.org, olekstysh@gmail.com,
Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel
Hi Volodymyr,
Thank you for your review.
On 04.09.25 00:24, Volodymyr Babchuk wrote:
> Hi Leonid,
>
> Leonid Komarianskyi <Leonid_Komarianskyi@epam.com> writes:
>
>> This change introduces resource management in the VGIC to handle
>> extended SPIs introduced in GICv3.1. The pending_irqs and
>> allocated_irqs arrays are resized to support the required
>> number of eSPIs, based on what is supported by the hardware and
>> requested by the guest. A new field, ext_shared_irqs, is added
>> to the VGIC structure to store information about eSPIs, similar
>> to how shared_irqs is used for regular SPIs.
>>
>> Since the eSPI range starts at INTID 4096 and INTIDs between 1025
>> and 4095 are reserved, helper macros are introduced to simplify the
>> transformation of indices and to enable easier access to eSPI-specific
>> resources. These changes prepare the VGIC for processing eSPIs as
>> required by future functionality.
>>
>> The initialization and deinitialization paths for vgic have been updated
>> to allocate and free these resources appropriately. Additionally,
>> updated handling of INTIDs greater than 1024, passed from the toolstack
>> during domain creation, and verification logic ensures only valid SPI or
>> eSPI INTIDs are used.
>>
>> The existing SPI behavior remains unaffected when guests do not request
>> eSPIs, GIC hardware does not support them, or the CONFIG_GICV3_ESPI
>> option is disabled.
>>
>> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
>>
>> ---
>> Changes for V6:
>> - introduced a new function for index to virq conversion, idx_to_virq.
>> This allows the removal of eSPI-specific functions and enables the
>> reuse of existing code for regular SPIs
>> - fixed the return value of vgic_allocate_virq in the case of eSPI
>> - updated the error message in route_irq_to_guest(). To simplify it and
>> avoid overcomplicating with INTID ranges, propose removing the
>> information about the max number of IRQs
>> - fixed eSPI rank initialization to avoid using EXT_RANK_IDX2NUM for
>> converting the eSPI rank to the extended range
>> - updated the recalculation logic to avoid the nr_spis > 1020 -
>> NR_LOCAL_IRQS check when nr_spis is reassigned in the case of eSPI
>> - fixed formatting issues for macros
>> - minor change: changed the macros ESPI_INTID2IDX and ESPI_IDX2INTID
>> into appropriate inline functions introduced in the previous patch
>> - minor change: changed int to unsigned int for nr_espis
>>
>> Changes in V5:
>> - removed the has_espi field because it can be determined by checking
>> whether domain->arch.vgic.nr_espis is zero or not
>> - since vgic_ext_rank_offset is not used in this patch, it has been
>> moved to the appropriate patch in the patch series, which implements
>> vgic eSPI registers emulation and requires this function
>> - removed ifdefs for eSPI-specific macros to reduce the number of ifdefs
>> and code duplication in further changes
>> - fixed minor nit: used %pd for printing domain with its ID
>>
>> Changes in V4:
>> - added has_espi field to simplify determining whether a domain is able
>> to operate with eSPI
>> - fixed formatting issues and misspellings
>>
>> Changes in V3:
>> - fixed formatting for lines with more than 80 symbols
>> - introduced helper functions to be able to use stubs in case of
>> CONFIG_GICV3_ESPI disabled, and as a result, reduce the number of
>> #ifdefs
>> - fixed checks for nr_spis in domain_vgic_init
>> - updated comment about nr_spis adjustments with dom0less mention
>> - moved comment with additional explanations before checks
>> - used unsigned int for indexes since they cannot be negative
>> - removed unnecessary parentheses
>> - move vgic_ext_rank_offset to the below ifdef guard, to reduce the
>> number of ifdefs
>>
>> Changes in V2:
>> - change is_espi_rank to is_valid_espi_rank to verify whether the array
>> element ext_shared_irqs exists. The previous version, is_espi_rank,
>> only checked if the rank index was less than the maximum possible eSPI
>> rank index, but this could potentially result in accessing a
>> non-existing array element. To address this, is_valid_espi_rank was
>> introduced, which ensures that the required eSPI rank exists
>> - move gic_number_espis to
>> xen/arm: gicv3: implement handling of GICv3.1 eSPI
>> - update vgic_is_valid_irq checks to allow operating with eSPIs
>> - remove redundant newline in vgic_allocate_virq
>> ---
>> xen/arch/arm/include/asm/vgic.h | 12 +++
>> xen/arch/arm/irq.c | 3 +-
>> xen/arch/arm/vgic.c | 174 ++++++++++++++++++++++++++++++--
>> 3 files changed, 176 insertions(+), 13 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
>> index 3e7cbbb196..1cf0a05832 100644
>> --- a/xen/arch/arm/include/asm/vgic.h
>> +++ b/xen/arch/arm/include/asm/vgic.h
>> @@ -146,6 +146,10 @@ struct vgic_dist {
>> int nr_spis; /* Number of SPIs */
>> unsigned long *allocated_irqs; /* bitmap of IRQs allocated */
>> struct vgic_irq_rank *shared_irqs;
>> +#ifdef CONFIG_GICV3_ESPI
>> + struct vgic_irq_rank *ext_shared_irqs;
>> + unsigned int nr_espis; /* Number of extended SPIs */
>> +#endif
>> /*
>> * SPIs are domain global, SGIs and PPIs are per-VCPU and stored in
>> * struct arch_vcpu.
>> @@ -243,6 +247,14 @@ struct vgic_ops {
>> /* Number of ranks of interrupt registers for a domain */
>> #define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_spis+31)/32)
>>
>> +#ifdef CONFIG_GICV3_ESPI
>> +#define DOMAIN_NR_EXT_RANKS(d) (((d)->arch.vgic.nr_espis + 31) / 32)
>> +#endif
>> +#define EXT_RANK_MIN (ESPI_BASE_INTID / 32)
>> +#define EXT_RANK_MAX ((ESPI_MAX_INTID + 31) / 32)
>> +#define EXT_RANK_NUM2IDX(num) ((num) - EXT_RANK_MIN)
>> +#define EXT_RANK_IDX2NUM(idx) ((idx) + EXT_RANK_MIN)
>> +
>> #define vgic_lock(v) spin_lock_irq(&(v)->domain->arch.vgic.lock)
>> #define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)
>>
>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>> index c934d39bf6..c2f1ceb91f 100644
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -494,8 +494,7 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
>> if ( !vgic_is_valid_line(d, virq) )
>> {
>> printk(XENLOG_G_ERR
>> - "the vIRQ number %u is too high for domain %u (max = %u)\n",
>> - irq, d->domain_id, vgic_num_irqs(d));
>> + "invalid vIRQ number %u for domain %pd\n", irq, d);
>> return -EINVAL;
>> }
>>
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 2bbf4d99aa..b42aee8d0c 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -25,11 +25,61 @@
>> #include <asm/vgic.h>
>>
>>
>> +static inline unsigned int idx_to_virq(struct domain *d, unsigned int idx)
>> +{
>> + if ( idx >= vgic_num_irqs(d) )
>> + return espi_idx_to_intid(idx - vgic_num_irqs(d));
>> +
>> + return idx;
>> +}
>> +
>> bool vgic_is_valid_line(struct domain *d, unsigned int virq)
>> {
>> +#ifdef CONFIG_GICV3_ESPI
>> + if ( virq >= ESPI_BASE_INTID &&
>> + virq < espi_idx_to_intid(d->arch.vgic.nr_espis) )
>> + return true;
>> +#endif
>> +
>> return virq < vgic_num_irqs(d);
>> }
>>
>> +#ifdef CONFIG_GICV3_ESPI
>> +/*
>> + * Since eSPI indexes start from 4096 and numbers from 1024 to
>> + * 4095 are forbidden, we need to check both lower and upper
>> + * limits for ranks.
>> + */
>> +static inline bool is_valid_espi_rank(struct domain *d, unsigned int rank)
>> +{
>> + return rank >= EXT_RANK_MIN &&
>> + EXT_RANK_NUM2IDX(rank) < DOMAIN_NR_EXT_RANKS(d);
>> +}
>> +
>> +static inline struct vgic_irq_rank *vgic_get_espi_rank(struct vcpu *v,
>> + unsigned int rank)
>> +{
>> + return &v->domain->arch.vgic.ext_shared_irqs[EXT_RANK_NUM2IDX(rank)];
>> +}
>> +
>> +#else
>> +static inline bool is_valid_espi_rank(struct domain *d, unsigned int rank)
>> +{
>> + return false;
>> +}
>> +
>> +/*
>> + * This function is stub and will not be called if CONFIG_GICV3_ESPI=n,
>> + * because in this case, is_valid_espi_rank will always return false.
>> + */
>> +static inline struct vgic_irq_rank *vgic_get_espi_rank(struct vcpu *v,
>> + unsigned int rank)
>> +{
>> + ASSERT_UNREACHABLE();
>> + return NULL;
>> +}
>> +#endif
>> +
>> static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
>> unsigned int rank)
>> {
>> @@ -37,6 +87,8 @@ static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
>> return v->arch.vgic.private_irqs;
>> else if ( rank <= DOMAIN_NR_RANKS(v->domain) )
>> return &v->domain->arch.vgic.shared_irqs[rank - 1];
>> + else if ( is_valid_espi_rank(v->domain, rank) )
>> + return vgic_get_espi_rank(v, rank);
>> else
>> return NULL;
>> }
>> @@ -117,6 +169,54 @@ int domain_vgic_register(struct domain *d, unsigned int *mmio_count)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_GICV3_ESPI
>> +static unsigned int vgic_num_spi_lines(struct domain *d)
>> +{
>> + return d->arch.vgic.nr_spis + d->arch.vgic.nr_espis;
>> +}
>> +
>> +static int init_vgic_espi(struct domain *d)
>> +{
>> + unsigned int i, idx;
>> +
>> + if ( d->arch.vgic.nr_espis == 0 )
>> + return 0;
>> +
>> + d->arch.vgic.ext_shared_irqs =
>> + xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_EXT_RANKS(d));
>> + if ( d->arch.vgic.ext_shared_irqs == NULL )
>> + return -ENOMEM;
>> +
>> + for ( i = d->arch.vgic.nr_spis, idx = 0;
>> + i < vgic_num_spi_lines(d); i++, idx++ )
>> + vgic_init_pending_irq(&d->arch.vgic.pending_irqs[i],
>> + espi_idx_to_intid(idx));
>> +
>> + for ( i = 0; i < DOMAIN_NR_EXT_RANKS(d); i++ )
>> + vgic_rank_init(&d->arch.vgic.ext_shared_irqs[i],
>> + EXT_RANK_IDX2NUM(i), 0);
>> +
>> + return 0;
>> +}
>> +
>> +#else
>> +static int init_vgic_espi(struct domain *d)
>> +{
>> + return 0;
>> +}
>> +
>> +static unsigned int vgic_num_spi_lines(struct domain *d)
>> +{
>> + return d->arch.vgic.nr_spis;
>> +}
>> +
>> +#endif
>> +
>> +static unsigned int vgic_num_alloc_irqs(struct domain *d)
>> +{
>> + return vgic_num_spi_lines(d) + NR_LOCAL_IRQS;
>
> This is good that you are using NR_LOCAL_IRQS here.
>
>> +}
>> +
>> int domain_vgic_init(struct domain *d, unsigned int nr_spis)
>> {
>> int i;
>> @@ -133,7 +233,39 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
>>
>> /* Limit the number of virtual SPIs supported to (1020 - 32) = 988 */
>> if ( nr_spis > (1020 - NR_LOCAL_IRQS) )
>> +#ifndef CONFIG_GICV3_ESPI
>> return -EINVAL;
>> +#else
>> + {
>> + /*
>> + * During domain creation, the dom0less DomUs code or toolstack
>> + * specifies the maximum INTID, which is defined in the domain
>> + * config subtracted by 32 to cover the local IRQs (please see
>> + * the comment to VGIC_DEF_NR_SPIS). To compute the actual number
>> + * of eSPI that will be usable for, add back 32.
>> + */
>> + nr_spis += 32;
>
> I think that it is better to use NR_LOCAL_IRQS here.
>
> (Also, I just noticed the same problem as with documentation, meaning
> that nr_spis actually does not represent number of SPIs anymore, and
> behaves more like max_spi, but let's set this issue aside for now)
>
>> + if ( nr_spis > espi_idx_to_intid(NR_ESPI_IRQS) )
>> + return -EINVAL;
>> +
>> + if ( nr_spis >= ESPI_BASE_INTID )
>> + {
>> + unsigned int nr_espis = min(nr_spis - ESPI_BASE_INTID, 1024U);
>> +
>> + /* Verify if GIC HW can handle provided INTID */
>> + if ( nr_espis > gic_number_espis() )
>> + return -EINVAL;
>> +
>> + d->arch.vgic.nr_espis = nr_espis;
>> + /* Set the maximum available number for regular SPIs */
>> + nr_spis = VGIC_DEF_NR_SPIS;
>> + }
>> + else
>> + {
>> + return -EINVAL;
>> + }
>> + }
>> +#endif
>>
>> d->arch.vgic.nr_spis = nr_spis;
>>
>> @@ -145,7 +277,7 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
>> return -ENOMEM;
>>
>> d->arch.vgic.pending_irqs =
>> - xzalloc_array(struct pending_irq, d->arch.vgic.nr_spis);
>> + xzalloc_array(struct pending_irq, vgic_num_spi_lines(d));
>> if ( d->arch.vgic.pending_irqs == NULL )
>> return -ENOMEM;
>>
>> @@ -156,12 +288,16 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
>> for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
>> vgic_rank_init(&d->arch.vgic.shared_irqs[i], i + 1, 0);
>>
>> + ret = init_vgic_espi(d);
>> + if ( ret )
>> + return ret;
>> +
>> ret = d->arch.vgic.handler->domain_init(d);
>> if ( ret )
>> return ret;
>>
>> d->arch.vgic.allocated_irqs =
>> - xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d)));
>> + xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_alloc_irqs(d)));
>> if ( !d->arch.vgic.allocated_irqs )
>> return -ENOMEM;
>>
>> @@ -182,9 +318,12 @@ void domain_vgic_free(struct domain *d)
>> int i;
>> int ret;
>>
>> - for ( i = 0; i < (d->arch.vgic.nr_spis); i++ )
>> + for ( i = 32; i < vgic_num_alloc_irqs(d); i++ )
>
> s/32/NR_LOCAL_IRQS
>
>> {
>> - struct pending_irq *p = spi_to_pending(d, i + 32);
>> + struct pending_irq *p;
>> + unsigned int virq = idx_to_virq(d, i);
>> +
>> + p = spi_to_pending(d, virq);
>>
>> if ( p->desc )
>> {
>> @@ -198,6 +337,9 @@ void domain_vgic_free(struct domain *d)
>> if ( d->arch.vgic.handler )
>> d->arch.vgic.handler->domain_free(d);
>> xfree(d->arch.vgic.shared_irqs);
>> +#ifdef CONFIG_GICV3_ESPI
>> + xfree(d->arch.vgic.ext_shared_irqs);
>> +#endif
>> xfree(d->arch.vgic.pending_irqs);
>> xfree(d->arch.vgic.allocated_irqs);
>> }
>> @@ -323,10 +465,12 @@ void arch_move_irqs(struct vcpu *v)
>> */
>> ASSERT(!is_lpi(vgic_num_irqs(d) - 1));
>>
>> - for ( i = 32; i < vgic_num_irqs(d); i++ )
>> + for ( i = 32; i < vgic_num_alloc_irqs(d); i++ )
>
> It is great idea to start using NR_LOCAL_IRQS here instead of hard-coded 32.
>
>> {
>> - v_target = vgic_get_target_vcpu(v, i);
>> - p = irq_to_pending(v_target, i);
>> + unsigned int virq = idx_to_virq(d, i);
>> +
>> + v_target = vgic_get_target_vcpu(v, virq);
>> + p = irq_to_pending(v_target, virq);
>>
>> if ( v_target == v && !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
>> irq_set_affinity(p->desc, cpu_mask);
>> @@ -539,7 +683,7 @@ struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
>> else if ( is_lpi(irq) )
>> n = v->domain->arch.vgic.handler->lpi_to_pending(v->domain, irq);
>> else
>> - n = &v->domain->arch.vgic.pending_irqs[irq - 32];
>> + n = spi_to_pending(v->domain, irq);
>> return n;
>> }
>>
>> @@ -547,7 +691,12 @@ struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq)
>> {
>> ASSERT(irq >= NR_LOCAL_IRQS);
>>
>> - return &d->arch.vgic.pending_irqs[irq - 32];
>> + if ( is_espi(irq) )
>> + irq = espi_intid_to_idx(irq) + d->arch.vgic.nr_spis;
>> + else
>> + irq -= 32;
>
> s/32/NR_LOCAL_IRQS
>
Okay, I will replace the hardcoded values with NR_LOCAL_IRQS here and in
similar places. :)
> Also, I want you to consider adding local variable "idx" instead of
> reusing "irq" parameter.
>
And also use idx in this case.
>> +
>> + return &d->arch.vgic.pending_irqs[irq];
>> }
>>
>> void vgic_clear_pending_irqs(struct vcpu *v)
>> @@ -668,6 +817,9 @@ bool vgic_reserve_virq(struct domain *d, unsigned int virq)
>> if ( !vgic_is_valid_line(d, virq) )
>> return false;
>>
>> + if ( is_espi(virq) )
>> + virq = espi_intid_to_idx(virq) + vgic_num_irqs(d);
>
> Here as well. It is very confusing when you are updating virq with a
> value that does not corresponds in IRQ number anymore.
>
>> +
>> return !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
>> }
>>
>> @@ -685,7 +837,7 @@ int vgic_allocate_virq(struct domain *d, bool spi)
>> else
>> {
>> first = 32;
>> - end = vgic_num_irqs(d);
>> + end = vgic_num_alloc_irqs(d);
>> }
>>
>> /*
>> @@ -700,7 +852,7 @@ int vgic_allocate_virq(struct domain *d, bool spi)
>> }
>> while ( test_and_set_bit(virq, d->arch.vgic.allocated_irqs) );
>>
>> - return virq;
>> + return idx_to_virq(d, virq);
>> }
>>
>> void vgic_free_virq(struct domain *d, unsigned int virq)
>
Best regards,
Leonid
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 08/12] xen/arm: vgic: add resource management for extended SPIs
2025-09-03 14:30 ` [PATCH v6 08/12] xen/arm: vgic: add resource management for extended SPIs Leonid Komarianskyi
2025-09-03 21:24 ` Volodymyr Babchuk
@ 2025-09-04 18:12 ` Oleksandr Tyshchenko
2025-09-04 18:23 ` Leonid Komarianskyi
1 sibling, 1 reply; 34+ messages in thread
From: Oleksandr Tyshchenko @ 2025-09-04 18:12 UTC (permalink / raw)
To: Leonid Komarianskyi, xen-devel@lists.xenproject.org
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
On 03.09.25 17:30, Leonid Komarianskyi wrote:
Hello Leonid
> This change introduces resource management in the VGIC to handle
> extended SPIs introduced in GICv3.1. The pending_irqs and
> allocated_irqs arrays are resized to support the required
> number of eSPIs, based on what is supported by the hardware and
> requested by the guest. A new field, ext_shared_irqs, is added
> to the VGIC structure to store information about eSPIs, similar
> to how shared_irqs is used for regular SPIs.
>
> Since the eSPI range starts at INTID 4096 and INTIDs between 1025
> and 4095 are reserved, helper macros are introduced to simplify the
> transformation of indices and to enable easier access to eSPI-specific
> resources. These changes prepare the VGIC for processing eSPIs as
> required by future functionality.
>
> The initialization and deinitialization paths for vgic have been updated
> to allocate and free these resources appropriately. Additionally,
> updated handling of INTIDs greater than 1024, passed from the toolstack
> during domain creation, and verification logic ensures only valid SPI or
> eSPI INTIDs are used.
>
> The existing SPI behavior remains unaffected when guests do not request
> eSPIs, GIC hardware does not support them, or the CONFIG_GICV3_ESPI
> option is disabled.
>
> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
>
> ---
> Changes for V6:
> - introduced a new function for index to virq conversion, idx_to_virq.
> This allows the removal of eSPI-specific functions and enables the
> reuse of existing code for regular SPIs
> - fixed the return value of vgic_allocate_virq in the case of eSPI
> - updated the error message in route_irq_to_guest(). To simplify it and
> avoid overcomplicating with INTID ranges, propose removing the
> information about the max number of IRQs
> - fixed eSPI rank initialization to avoid using EXT_RANK_IDX2NUM for
> converting the eSPI rank to the extended range
> - updated the recalculation logic to avoid the nr_spis > 1020 -
> NR_LOCAL_IRQS check when nr_spis is reassigned in the case of eSPI
> - fixed formatting issues for macros
> - minor change: changed the macros ESPI_INTID2IDX and ESPI_IDX2INTID
> into appropriate inline functions introduced in the previous patch
> - minor change: changed int to unsigned int for nr_espis
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
preferably with NIT below
>
> Changes in V5:
> - removed the has_espi field because it can be determined by checking
> whether domain->arch.vgic.nr_espis is zero or not
> - since vgic_ext_rank_offset is not used in this patch, it has been
> moved to the appropriate patch in the patch series, which implements
> vgic eSPI registers emulation and requires this function
> - removed ifdefs for eSPI-specific macros to reduce the number of ifdefs
> and code duplication in further changes
> - fixed minor nit: used %pd for printing domain with its ID
>
> Changes in V4:
> - added has_espi field to simplify determining whether a domain is able
> to operate with eSPI
> - fixed formatting issues and misspellings
>
> Changes in V3:
> - fixed formatting for lines with more than 80 symbols
> - introduced helper functions to be able to use stubs in case of
> CONFIG_GICV3_ESPI disabled, and as a result, reduce the number of
> #ifdefs
> - fixed checks for nr_spis in domain_vgic_init
> - updated comment about nr_spis adjustments with dom0less mention
> - moved comment with additional explanations before checks
> - used unsigned int for indexes since they cannot be negative
> - removed unnecessary parentheses
> - move vgic_ext_rank_offset to the below ifdef guard, to reduce the
> number of ifdefs
>
> Changes in V2:
> - change is_espi_rank to is_valid_espi_rank to verify whether the array
> element ext_shared_irqs exists. The previous version, is_espi_rank,
> only checked if the rank index was less than the maximum possible eSPI
> rank index, but this could potentially result in accessing a
> non-existing array element. To address this, is_valid_espi_rank was
> introduced, which ensures that the required eSPI rank exists
> - move gic_number_espis to
> xen/arm: gicv3: implement handling of GICv3.1 eSPI
> - update vgic_is_valid_irq checks to allow operating with eSPIs
> - remove redundant newline in vgic_allocate_virq
> ---
> xen/arch/arm/include/asm/vgic.h | 12 +++
> xen/arch/arm/irq.c | 3 +-
> xen/arch/arm/vgic.c | 174 ++++++++++++++++++++++++++++++--
> 3 files changed, 176 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
> index 3e7cbbb196..1cf0a05832 100644
> --- a/xen/arch/arm/include/asm/vgic.h
> +++ b/xen/arch/arm/include/asm/vgic.h
> @@ -146,6 +146,10 @@ struct vgic_dist {
> int nr_spis; /* Number of SPIs */
> unsigned long *allocated_irqs; /* bitmap of IRQs allocated */
> struct vgic_irq_rank *shared_irqs;
> +#ifdef CONFIG_GICV3_ESPI
> + struct vgic_irq_rank *ext_shared_irqs;
> + unsigned int nr_espis; /* Number of extended SPIs */
> +#endif
> /*
> * SPIs are domain global, SGIs and PPIs are per-VCPU and stored in
> * struct arch_vcpu.
> @@ -243,6 +247,14 @@ struct vgic_ops {
> /* Number of ranks of interrupt registers for a domain */
> #define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_spis+31)/32)
>
> +#ifdef CONFIG_GICV3_ESPI
> +#define DOMAIN_NR_EXT_RANKS(d) (((d)->arch.vgic.nr_espis + 31) / 32)
> +#endif
> +#define EXT_RANK_MIN (ESPI_BASE_INTID / 32)
> +#define EXT_RANK_MAX ((ESPI_MAX_INTID + 31) / 32)
> +#define EXT_RANK_NUM2IDX(num) ((num) - EXT_RANK_MIN)
> +#define EXT_RANK_IDX2NUM(idx) ((idx) + EXT_RANK_MIN)
> +
> #define vgic_lock(v) spin_lock_irq(&(v)->domain->arch.vgic.lock)
> #define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)
>
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index c934d39bf6..c2f1ceb91f 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -494,8 +494,7 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
> if ( !vgic_is_valid_line(d, virq) )
> {
> printk(XENLOG_G_ERR
> - "the vIRQ number %u is too high for domain %u (max = %u)\n",
> - irq, d->domain_id, vgic_num_irqs(d));
> + "invalid vIRQ number %u for domain %pd\n", irq, d);
> return -EINVAL;
> }
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 2bbf4d99aa..b42aee8d0c 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -25,11 +25,61 @@
> #include <asm/vgic.h>
>
>
> +static inline unsigned int idx_to_virq(struct domain *d, unsigned int idx)
> +{
> + if ( idx >= vgic_num_irqs(d) )
> + return espi_idx_to_intid(idx - vgic_num_irqs(d));
> +
> + return idx;
> +}
> +
> bool vgic_is_valid_line(struct domain *d, unsigned int virq)
> {
> +#ifdef CONFIG_GICV3_ESPI
> + if ( virq >= ESPI_BASE_INTID &&
> + virq < espi_idx_to_intid(d->arch.vgic.nr_espis) )
> + return true;
> +#endif
> +
> return virq < vgic_num_irqs(d);
> }
>
> +#ifdef CONFIG_GICV3_ESPI
> +/*
> + * Since eSPI indexes start from 4096 and numbers from 1024 to
> + * 4095 are forbidden, we need to check both lower and upper
> + * limits for ranks.
> + */
> +static inline bool is_valid_espi_rank(struct domain *d, unsigned int rank)
> +{
> + return rank >= EXT_RANK_MIN &&
> + EXT_RANK_NUM2IDX(rank) < DOMAIN_NR_EXT_RANKS(d);
> +}
> +
> +static inline struct vgic_irq_rank *vgic_get_espi_rank(struct vcpu *v,
> + unsigned int rank)
> +{
> + return &v->domain->arch.vgic.ext_shared_irqs[EXT_RANK_NUM2IDX(rank)];
> +}
> +
> +#else
> +static inline bool is_valid_espi_rank(struct domain *d, unsigned int rank)
> +{
> + return false;
> +}
> +
> +/*
> + * This function is stub and will not be called if CONFIG_GICV3_ESPI=n,
> + * because in this case, is_valid_espi_rank will always return false.
> + */
> +static inline struct vgic_irq_rank *vgic_get_espi_rank(struct vcpu *v,
> + unsigned int rank)
> +{
> + ASSERT_UNREACHABLE();
> + return NULL;
> +}
> +#endif
> +
> static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
> unsigned int rank)
> {
> @@ -37,6 +87,8 @@ static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
> return v->arch.vgic.private_irqs;
> else if ( rank <= DOMAIN_NR_RANKS(v->domain) )
> return &v->domain->arch.vgic.shared_irqs[rank - 1];
> + else if ( is_valid_espi_rank(v->domain, rank) )
> + return vgic_get_espi_rank(v, rank);
> else
> return NULL;
> }
> @@ -117,6 +169,54 @@ int domain_vgic_register(struct domain *d, unsigned int *mmio_count)
> return 0;
> }
>
> +#ifdef CONFIG_GICV3_ESPI
> +static unsigned int vgic_num_spi_lines(struct domain *d)
> +{
> + return d->arch.vgic.nr_spis + d->arch.vgic.nr_espis;
> +}
> +
> +static int init_vgic_espi(struct domain *d)
> +{
> + unsigned int i, idx;
> +
> + if ( d->arch.vgic.nr_espis == 0 )
> + return 0;
> +
> + d->arch.vgic.ext_shared_irqs =
> + xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_EXT_RANKS(d));
> + if ( d->arch.vgic.ext_shared_irqs == NULL )
> + return -ENOMEM;
> +
> + for ( i = d->arch.vgic.nr_spis, idx = 0;
> + i < vgic_num_spi_lines(d); i++, idx++ )
> + vgic_init_pending_irq(&d->arch.vgic.pending_irqs[i],
> + espi_idx_to_intid(idx));
> +
> + for ( i = 0; i < DOMAIN_NR_EXT_RANKS(d); i++ )
> + vgic_rank_init(&d->arch.vgic.ext_shared_irqs[i],
> + EXT_RANK_IDX2NUM(i), 0);
> +
> + return 0;
> +}
> +
> +#else
> +static int init_vgic_espi(struct domain *d)
> +{
> + return 0;
> +}
> +
> +static unsigned int vgic_num_spi_lines(struct domain *d)
> +{
> + return d->arch.vgic.nr_spis;
> +}
> +
> +#endif
> +
> +static unsigned int vgic_num_alloc_irqs(struct domain *d)
> +{
> + return vgic_num_spi_lines(d) + NR_LOCAL_IRQS;
> +}
> +
> int domain_vgic_init(struct domain *d, unsigned int nr_spis)
> {
> int i;
> @@ -133,7 +233,39 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
>
> /* Limit the number of virtual SPIs supported to (1020 - 32) = 988 */
> if ( nr_spis > (1020 - NR_LOCAL_IRQS) )
> +#ifndef CONFIG_GICV3_ESPI
> return -EINVAL;
> +#else
> + {
> + /*
> + * During domain creation, the dom0less DomUs code or toolstack
> + * specifies the maximum INTID, which is defined in the domain
> + * config subtracted by 32 to cover the local IRQs (please see
> + * the comment to VGIC_DEF_NR_SPIS). To compute the actual number
> + * of eSPI that will be usable for, add back 32.
> + */
> + nr_spis += 32;
> + if ( nr_spis > espi_idx_to_intid(NR_ESPI_IRQS) )
> + return -EINVAL;
> +
> + if ( nr_spis >= ESPI_BASE_INTID )
> + {
> + unsigned int nr_espis = min(nr_spis - ESPI_BASE_INTID, 1024U);
> +
> + /* Verify if GIC HW can handle provided INTID */
> + if ( nr_espis > gic_number_espis() )
> + return -EINVAL;
> +
> + d->arch.vgic.nr_espis = nr_espis;
> + /* Set the maximum available number for regular SPIs */
> + nr_spis = VGIC_DEF_NR_SPIS;
> + }
> + else
> + {
> + return -EINVAL;
> + }
> + }
> +#endif
>
> d->arch.vgic.nr_spis = nr_spis;
>
> @@ -145,7 +277,7 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
> return -ENOMEM;
>
> d->arch.vgic.pending_irqs =
> - xzalloc_array(struct pending_irq, d->arch.vgic.nr_spis);
> + xzalloc_array(struct pending_irq, vgic_num_spi_lines(d));
So, you do not keep two separate pending_irqs arrays, just like
the shared_irqs and ext_shared_irqs arrays. You allocate
pending_irqs array as a single flat array to hold both regular SPIs and
eSPIs with looks like the following mapping ---> regular SPIs
[0..nr_spis-1] and eSPIs [nr_spis..nr_spis+nr_espis-1].
NIT: I would add a comment above the pending_irqs field in struct
vgic_dist saying that an array is also supposed to hold extended SPIs if
present, or something like that.
Otherwise, by just looking into struct vgic_dist in the header, it is
not clear where other (than ext_shared_irqs, nr_espis) eSPI bits are.
> if ( d->arch.vgic.pending_irqs == NULL )
> return -ENOMEM;
>
> @@ -156,12 +288,16 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
> for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
> vgic_rank_init(&d->arch.vgic.shared_irqs[i], i + 1, 0);
>
> + ret = init_vgic_espi(d);
> + if ( ret )
> + return ret;
> +
> ret = d->arch.vgic.handler->domain_init(d);
> if ( ret )
> return ret;
>
> d->arch.vgic.allocated_irqs =
> - xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d)));
> + xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_alloc_irqs(d)));
NIT: The same goes for the allocated_irqs field.
> if ( !d->arch.vgic.allocated_irqs )
> return -ENOMEM;
>
[snip]
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v6 08/12] xen/arm: vgic: add resource management for extended SPIs
2025-09-04 18:12 ` Oleksandr Tyshchenko
@ 2025-09-04 18:23 ` Leonid Komarianskyi
0 siblings, 0 replies; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-04 18:23 UTC (permalink / raw)
To: Oleksandr Tyshchenko, xen-devel@lists.xenproject.org
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Hello Oleksandr,
Thank you for your review comments and RB.
On 04.09.25 21:12, Oleksandr Tyshchenko wrote:
>
>
> On 03.09.25 17:30, Leonid Komarianskyi wrote:
>
> Hello Leonid
>
>> This change introduces resource management in the VGIC to handle
>> extended SPIs introduced in GICv3.1. The pending_irqs and
>> allocated_irqs arrays are resized to support the required
>> number of eSPIs, based on what is supported by the hardware and
>> requested by the guest. A new field, ext_shared_irqs, is added
>> to the VGIC structure to store information about eSPIs, similar
>> to how shared_irqs is used for regular SPIs.
>>
>> Since the eSPI range starts at INTID 4096 and INTIDs between 1025
>> and 4095 are reserved, helper macros are introduced to simplify the
>> transformation of indices and to enable easier access to eSPI-specific
>> resources. These changes prepare the VGIC for processing eSPIs as
>> required by future functionality.
>>
>> The initialization and deinitialization paths for vgic have been updated
>> to allocate and free these resources appropriately. Additionally,
>> updated handling of INTIDs greater than 1024, passed from the toolstack
>> during domain creation, and verification logic ensures only valid SPI or
>> eSPI INTIDs are used.
>>
>> The existing SPI behavior remains unaffected when guests do not request
>> eSPIs, GIC hardware does not support them, or the CONFIG_GICV3_ESPI
>> option is disabled.
>>
>> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
>>
>> ---
>> Changes for V6:
>> - introduced a new function for index to virq conversion, idx_to_virq.
>> This allows the removal of eSPI-specific functions and enables the
>> reuse of existing code for regular SPIs
>> - fixed the return value of vgic_allocate_virq in the case of eSPI
>> - updated the error message in route_irq_to_guest(). To simplify it and
>> avoid overcomplicating with INTID ranges, propose removing the
>> information about the max number of IRQs
>> - fixed eSPI rank initialization to avoid using EXT_RANK_IDX2NUM for
>> converting the eSPI rank to the extended range
>> - updated the recalculation logic to avoid the nr_spis > 1020 -
>> NR_LOCAL_IRQS check when nr_spis is reassigned in the case of eSPI
>> - fixed formatting issues for macros
>> - minor change: changed the macros ESPI_INTID2IDX and ESPI_IDX2INTID
>> into appropriate inline functions introduced in the previous patch
>> - minor change: changed int to unsigned int for nr_espis
>
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> preferably with NIT below
>
>
>>
>> Changes in V5:
>> - removed the has_espi field because it can be determined by checking
>> whether domain->arch.vgic.nr_espis is zero or not
>> - since vgic_ext_rank_offset is not used in this patch, it has been
>> moved to the appropriate patch in the patch series, which implements
>> vgic eSPI registers emulation and requires this function
>> - removed ifdefs for eSPI-specific macros to reduce the number of ifdefs
>> and code duplication in further changes
>> - fixed minor nit: used %pd for printing domain with its ID
>>
>> Changes in V4:
>> - added has_espi field to simplify determining whether a domain is able
>> to operate with eSPI
>> - fixed formatting issues and misspellings
>>
>> Changes in V3:
>> - fixed formatting for lines with more than 80 symbols
>> - introduced helper functions to be able to use stubs in case of
>> CONFIG_GICV3_ESPI disabled, and as a result, reduce the number of
>> #ifdefs
>> - fixed checks for nr_spis in domain_vgic_init
>> - updated comment about nr_spis adjustments with dom0less mention
>> - moved comment with additional explanations before checks
>> - used unsigned int for indexes since they cannot be negative
>> - removed unnecessary parentheses
>> - move vgic_ext_rank_offset to the below ifdef guard, to reduce the
>> number of ifdefs
>>
>> Changes in V2:
>> - change is_espi_rank to is_valid_espi_rank to verify whether the array
>> element ext_shared_irqs exists. The previous version, is_espi_rank,
>> only checked if the rank index was less than the maximum possible eSPI
>> rank index, but this could potentially result in accessing a
>> non-existing array element. To address this, is_valid_espi_rank was
>> introduced, which ensures that the required eSPI rank exists
>> - move gic_number_espis to
>> xen/arm: gicv3: implement handling of GICv3.1 eSPI
>> - update vgic_is_valid_irq checks to allow operating with eSPIs
>> - remove redundant newline in vgic_allocate_virq
>> ---
>> xen/arch/arm/include/asm/vgic.h | 12 +++
>> xen/arch/arm/irq.c | 3 +-
>> xen/arch/arm/vgic.c | 174 ++++++++++++++++++++++++++++++--
>> 3 files changed, 176 insertions(+), 13 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/
>> asm/vgic.h
>> index 3e7cbbb196..1cf0a05832 100644
>> --- a/xen/arch/arm/include/asm/vgic.h
>> +++ b/xen/arch/arm/include/asm/vgic.h
>> @@ -146,6 +146,10 @@ struct vgic_dist {
>> int nr_spis; /* Number of SPIs */
>> unsigned long *allocated_irqs; /* bitmap of IRQs allocated */
>> struct vgic_irq_rank *shared_irqs;
>> +#ifdef CONFIG_GICV3_ESPI
>> + struct vgic_irq_rank *ext_shared_irqs;
>> + unsigned int nr_espis; /* Number of extended SPIs */
>> +#endif
>> /*
>> * SPIs are domain global, SGIs and PPIs are per-VCPU and stored in
>> * struct arch_vcpu.
>> @@ -243,6 +247,14 @@ struct vgic_ops {
>> /* Number of ranks of interrupt registers for a domain */
>> #define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_spis+31)/32)
>> +#ifdef CONFIG_GICV3_ESPI
>> +#define DOMAIN_NR_EXT_RANKS(d) (((d)->arch.vgic.nr_espis + 31) / 32)
>> +#endif
>> +#define EXT_RANK_MIN (ESPI_BASE_INTID / 32)
>> +#define EXT_RANK_MAX ((ESPI_MAX_INTID + 31) / 32)
>> +#define EXT_RANK_NUM2IDX(num) ((num) - EXT_RANK_MIN)
>> +#define EXT_RANK_IDX2NUM(idx) ((idx) + EXT_RANK_MIN)
>> +
>> #define vgic_lock(v) spin_lock_irq(&(v)->domain->arch.vgic.lock)
>> #define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)
>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>> index c934d39bf6..c2f1ceb91f 100644
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -494,8 +494,7 @@ int route_irq_to_guest(struct domain *d, unsigned
>> int virq,
>> if ( !vgic_is_valid_line(d, virq) )
>> {
>> printk(XENLOG_G_ERR
>> - "the vIRQ number %u is too high for domain %u (max =
>> %u)\n",
>> - irq, d->domain_id, vgic_num_irqs(d));
>> + "invalid vIRQ number %u for domain %pd\n", irq, d);
>> return -EINVAL;
>> }
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 2bbf4d99aa..b42aee8d0c 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -25,11 +25,61 @@
>> #include <asm/vgic.h>
>> +static inline unsigned int idx_to_virq(struct domain *d, unsigned int
>> idx)
>> +{
>> + if ( idx >= vgic_num_irqs(d) )
>> + return espi_idx_to_intid(idx - vgic_num_irqs(d));
>> +
>> + return idx;
>> +}
>> +
>> bool vgic_is_valid_line(struct domain *d, unsigned int virq)
>> {
>> +#ifdef CONFIG_GICV3_ESPI
>> + if ( virq >= ESPI_BASE_INTID &&
>> + virq < espi_idx_to_intid(d->arch.vgic.nr_espis) )
>> + return true;
>> +#endif
>> +
>> return virq < vgic_num_irqs(d);
>> }
>> +#ifdef CONFIG_GICV3_ESPI
>> +/*
>> + * Since eSPI indexes start from 4096 and numbers from 1024 to
>> + * 4095 are forbidden, we need to check both lower and upper
>> + * limits for ranks.
>> + */
>> +static inline bool is_valid_espi_rank(struct domain *d, unsigned int
>> rank)
>> +{
>> + return rank >= EXT_RANK_MIN &&
>> + EXT_RANK_NUM2IDX(rank) < DOMAIN_NR_EXT_RANKS(d);
>> +}
>> +
>> +static inline struct vgic_irq_rank *vgic_get_espi_rank(struct vcpu *v,
>> + unsigned int
>> rank)
>> +{
>> + return &v->domain-
>> >arch.vgic.ext_shared_irqs[EXT_RANK_NUM2IDX(rank)];
>> +}
>> +
>> +#else
>> +static inline bool is_valid_espi_rank(struct domain *d, unsigned int
>> rank)
>> +{
>> + return false;
>> +}
>> +
>> +/*
>> + * This function is stub and will not be called if CONFIG_GICV3_ESPI=n,
>> + * because in this case, is_valid_espi_rank will always return false.
>> + */
>> +static inline struct vgic_irq_rank *vgic_get_espi_rank(struct vcpu *v,
>> + unsigned int
>> rank)
>> +{
>> + ASSERT_UNREACHABLE();
>> + return NULL;
>> +}
>> +#endif
>> +
>> static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
>> unsigned int rank)
>> {
>> @@ -37,6 +87,8 @@ static inline struct vgic_irq_rank
>> *vgic_get_rank(struct vcpu *v,
>> return v->arch.vgic.private_irqs;
>> else if ( rank <= DOMAIN_NR_RANKS(v->domain) )
>> return &v->domain->arch.vgic.shared_irqs[rank - 1];
>> + else if ( is_valid_espi_rank(v->domain, rank) )
>> + return vgic_get_espi_rank(v, rank);
>> else
>> return NULL;
>> }
>> @@ -117,6 +169,54 @@ int domain_vgic_register(struct domain *d,
>> unsigned int *mmio_count)
>> return 0;
>> }
>> +#ifdef CONFIG_GICV3_ESPI
>> +static unsigned int vgic_num_spi_lines(struct domain *d)
>> +{
>> + return d->arch.vgic.nr_spis + d->arch.vgic.nr_espis;
>> +}
>> +
>> +static int init_vgic_espi(struct domain *d)
>> +{
>> + unsigned int i, idx;
>> +
>> + if ( d->arch.vgic.nr_espis == 0 )
>> + return 0;
>> +
>> + d->arch.vgic.ext_shared_irqs =
>> + xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_EXT_RANKS(d));
>> + if ( d->arch.vgic.ext_shared_irqs == NULL )
>> + return -ENOMEM;
>> +
>> + for ( i = d->arch.vgic.nr_spis, idx = 0;
>> + i < vgic_num_spi_lines(d); i++, idx++ )
>> + vgic_init_pending_irq(&d->arch.vgic.pending_irqs[i],
>> + espi_idx_to_intid(idx));
>> +
>> + for ( i = 0; i < DOMAIN_NR_EXT_RANKS(d); i++ )
>> + vgic_rank_init(&d->arch.vgic.ext_shared_irqs[i],
>> + EXT_RANK_IDX2NUM(i), 0);
>> +
>> + return 0;
>> +}
>> +
>> +#else
>> +static int init_vgic_espi(struct domain *d)
>> +{
>> + return 0;
>> +}
>> +
>> +static unsigned int vgic_num_spi_lines(struct domain *d)
>> +{
>> + return d->arch.vgic.nr_spis;
>> +}
>> +
>> +#endif
>> +
>> +static unsigned int vgic_num_alloc_irqs(struct domain *d)
>> +{
>> + return vgic_num_spi_lines(d) + NR_LOCAL_IRQS;
>> +}
>> +
>> int domain_vgic_init(struct domain *d, unsigned int nr_spis)
>> {
>> int i;
>> @@ -133,7 +233,39 @@ int domain_vgic_init(struct domain *d, unsigned
>> int nr_spis)
>> /* Limit the number of virtual SPIs supported to (1020 - 32) =
>> 988 */
>> if ( nr_spis > (1020 - NR_LOCAL_IRQS) )
>> +#ifndef CONFIG_GICV3_ESPI
>> return -EINVAL;
>> +#else
>> + {
>> + /*
>> + * During domain creation, the dom0less DomUs code or toolstack
>> + * specifies the maximum INTID, which is defined in the domain
>> + * config subtracted by 32 to cover the local IRQs (please see
>> + * the comment to VGIC_DEF_NR_SPIS). To compute the actual
>> number
>> + * of eSPI that will be usable for, add back 32.
>> + */
>> + nr_spis += 32;
>> + if ( nr_spis > espi_idx_to_intid(NR_ESPI_IRQS) )
>> + return -EINVAL;
>> +
>> + if ( nr_spis >= ESPI_BASE_INTID )
>> + {
>> + unsigned int nr_espis = min(nr_spis - ESPI_BASE_INTID,
>> 1024U);
>> +
>> + /* Verify if GIC HW can handle provided INTID */
>> + if ( nr_espis > gic_number_espis() )
>> + return -EINVAL;
>> +
>> + d->arch.vgic.nr_espis = nr_espis;
>> + /* Set the maximum available number for regular SPIs */
>> + nr_spis = VGIC_DEF_NR_SPIS;
>> + }
>> + else
>> + {
>> + return -EINVAL;
>> + }
>> + }
>> +#endif
>> d->arch.vgic.nr_spis = nr_spis;
>> @@ -145,7 +277,7 @@ int domain_vgic_init(struct domain *d, unsigned
>> int nr_spis)
>> return -ENOMEM;
>> d->arch.vgic.pending_irqs =
>> - xzalloc_array(struct pending_irq, d->arch.vgic.nr_spis);
>> + xzalloc_array(struct pending_irq, vgic_num_spi_lines(d));
>
>
> So, you do not keep two separate pending_irqs arrays, just like
> the shared_irqs and ext_shared_irqs arrays. You allocate
> pending_irqs array as a single flat array to hold both regular SPIs and
> eSPIs with looks like the following mapping ---> regular SPIs
> [0..nr_spis-1] and eSPIs [nr_spis..nr_spis+nr_espis-1].
>
> NIT: I would add a comment above the pending_irqs field in struct
> vgic_dist saying that an array is also supposed to hold extended SPIs if
> present, or something like that.
> Otherwise, by just looking into struct vgic_dist in the header, it is
> not clear where other (than ext_shared_irqs, nr_espis) eSPI bits are.
>
>
Yes, it will definitely be useful. I will add the comments - thank you.
>> if ( d->arch.vgic.pending_irqs == NULL )
>> return -ENOMEM;
>> @@ -156,12 +288,16 @@ int domain_vgic_init(struct domain *d, unsigned
>> int nr_spis)
>> for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
>> vgic_rank_init(&d->arch.vgic.shared_irqs[i], i + 1, 0);
>> + ret = init_vgic_espi(d);
>> + if ( ret )
>> + return ret;
>> +
>> ret = d->arch.vgic.handler->domain_init(d);
>> if ( ret )
>> return ret;
>> d->arch.vgic.allocated_irqs =
>> - xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d)));
>> + xzalloc_array(unsigned long,
>> BITS_TO_LONGS(vgic_num_alloc_irqs(d)));
>
> NIT: The same goes for the allocated_irqs field.
>
>> if ( !d->arch.vgic.allocated_irqs )
>> return -ENOMEM;
>
>
> [snip]
Best regards,
Leonid
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v6 09/12] xen/arm: domain_build/dom0less-build: adjust domains config to support eSPIs
2025-09-03 14:29 [PATCH v6 00/12] Introduce eSPI support Leonid Komarianskyi
` (7 preceding siblings ...)
2025-09-03 14:30 ` [PATCH v6 08/12] xen/arm: vgic: add resource management for extended SPIs Leonid Komarianskyi
@ 2025-09-03 14:30 ` Leonid Komarianskyi
2025-09-03 14:30 ` [PATCH v6 10/12] xen/arm: vgic-v3: add emulation of GICv3.1 eSPI registers Leonid Komarianskyi
` (2 subsequent siblings)
11 siblings, 0 replies; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-03 14:30 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: olekstysh@gmail.com, Leonid Komarianskyi, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel, Volodymyr Babchuk,
Volodymyr Babchuk, Oleksandr Tyshchenko
The Dom0 and DomUs logic for the dom0less configuration in create_dom0()
and arch_create_domUs() should account for extended SPIs when supported
by the hardware and enabled with CONFIG_GICV3_ESPI. These changes ensure
proper calculation of the maximum number of SPIs and eSPIs available to
Dom0 and DomUs in dom0less setups.
When eSPIs are supported by the hardware and CONFIG_GICV3_ESPI is enabled, the
maximum number of eSPI interrupts is calculated using the ESPI_BASE_INTID
offset (4096) and is limited to 1024, with 32 IRQs subtracted. To ensure
compatibility with non-Dom0 domains, this adjustment is applied by the
toolstack during domain creation, while for Dom0 or DomUs in Dom0, it is
handled directly during VGIC initialization. If eSPIs are not supported, the
calculation defaults to using the standard SPI range, with a maximum value of
992 interrupt lines, as it works currently.
Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
Changes in V6:
- minor: updated the commit message
Changes in V5:
- fixed minor nits, no functional changes: updated the comment to make
it clearer and corrected a misspelling
- added reviewed-by from Volodymyr Babchuk and from Oleksandr Tyshchenko
Changes in V4:
- consolidated the eSPI and SPI logic into a new inline function,
vgic_def_nr_spis. Without eSPI support (either due to config being
disabled or hardware not supporting it), it will return the regular SPI
range, as it works currently. There are no functional changes compared
with the previous patch version
- removed VGIC_DEF_MAX_SPI macro, to reduce the number of ifdefs
Changes in V3:
- renamed macro VGIC_DEF_NR_ESPIS to more appropriate VGIC_DEF_MAX_SPI
- added eSPI initialization for dom0less setups
- fixed comment with mentions about dom0less builds
- fixed formatting for lines with more than 80 symbols
- updated commit message
Changes in V2:
- no changes
---
xen/arch/arm/dom0less-build.c | 2 +-
xen/arch/arm/domain_build.c | 2 +-
xen/arch/arm/include/asm/vgic.h | 19 +++++++++++++++++++
3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 69b9ea22ce..02d5559102 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -285,7 +285,7 @@ void __init arch_create_domUs(struct dt_device_node *node,
{
int vpl011_virq = GUEST_VPL011_SPI;
- d_cfg->arch.nr_spis = VGIC_DEF_NR_SPIS;
+ d_cfg->arch.nr_spis = vgic_def_nr_spis();
/*
* The VPL011 virq is GUEST_VPL011_SPI, unless direct-map is
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d91a71acfd..39eea0be00 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2054,7 +2054,7 @@ void __init create_dom0(void)
/* The vGIC for DOM0 is exactly emulating the hardware GIC */
dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
- dom0_cfg.arch.nr_spis = VGIC_DEF_NR_SPIS;
+ dom0_cfg.arch.nr_spis = vgic_def_nr_spis();
dom0_cfg.arch.tee_type = tee_get_type();
dom0_cfg.max_vcpus = dom0_max_vcpus();
diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
index 1cf0a05832..c52bbcb115 100644
--- a/xen/arch/arm/include/asm/vgic.h
+++ b/xen/arch/arm/include/asm/vgic.h
@@ -347,6 +347,25 @@ extern void vgic_check_inflight_irqs_pending(struct vcpu *v,
/* Default number of vGIC SPIs. 32 are substracted to cover local IRQs. */
#define VGIC_DEF_NR_SPIS (min(gic_number_lines(), VGIC_MAX_IRQS) - 32)
+static inline unsigned int vgic_def_nr_spis(void)
+{
+#ifdef CONFIG_GICV3_ESPI
+ /*
+ * Check if the hardware supports extended SPIs (even if the appropriate
+ * config is set). If not, the common SPI range will be used. Otherwise
+ * return the maximum eSPI INTID, supported by HW GIC, subtracted by 32.
+ * For Dom0 and started at boot time DomUs we will add back this value
+ * during VGIC initialization. This ensures consistent handling for Dom0
+ * and other domains. For the regular SPI range interrupts in this case,
+ * the maximum value of VGIC_DEF_NR_SPIS will be used.
+ */
+ if ( gic_number_espis() > 0 )
+ return ESPI_BASE_INTID + min(gic_number_espis(), 1024U) - 32;
+#endif
+
+ return VGIC_DEF_NR_SPIS;
+}
+
extern bool vgic_is_valid_line(struct domain *d, unsigned int virq);
static inline bool vgic_is_spi(struct domain *d, unsigned int virq)
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v6 10/12] xen/arm: vgic-v3: add emulation of GICv3.1 eSPI registers
2025-09-03 14:29 [PATCH v6 00/12] Introduce eSPI support Leonid Komarianskyi
` (8 preceding siblings ...)
2025-09-03 14:30 ` [PATCH v6 09/12] xen/arm: domain_build/dom0less-build: adjust domains config to support eSPIs Leonid Komarianskyi
@ 2025-09-03 14:30 ` Leonid Komarianskyi
2025-09-03 19:27 ` Oleksandr Tyshchenko
2025-09-03 14:30 ` [PATCH v6 11/12] doc/man: update description for nr_spis with eSPI Leonid Komarianskyi
2025-09-03 14:30 ` [PATCH v6 12/12] CHANGELOG.md: add mention of GICv3.1 eSPI support Leonid Komarianskyi
11 siblings, 1 reply; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-03 14:30 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: olekstysh@gmail.com, Leonid Komarianskyi, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel, Volodymyr Babchuk
Implemented support for GICv3.1 extended SPI registers for vGICv3,
allowing the emulation of eSPI-specific behavior for guest domains.
The implementation includes read and write emulation for eSPI-related
registers (e.g., GICD_ISENABLERnE, GICD_IROUTERnE, and others),
following a similar approach to the handling of regular SPIs.
The eSPI registers, previously located in reserved address ranges,
are now adjusted to support MMIO read and write operations correctly
when CONFIG_GICV3_ESPI is enabled.
The availability of eSPIs and the number of emulated extended SPIs
for guest domains is reported by setting the appropriate bits in the
GICD_TYPER register, based on the number of eSPIs requested by the
domain and supported by the hardware. In cases where the configuration
option is disabled, the hardware does not support eSPIs, or the domain
does not request such interrupts, the functionality remains unchanged.
Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
---
Changes in V6:
- introduced helper functions: vgic_get_rank and vgic_get_reg_offset to
avoid boilerplate code and simplify changes
- fixed index initialization in the previous patch ([08/12] xen/arm:
vgic: add resource management for extended SPIs) and removed index
conversion for vgic_enable_irqs(), vgic_disable_irqs(),
vgic_set_irqs_pending(), and vgic_check_inflight_irqs_pending()
- grouped SPI and eSPI registers
- updated the comment for vgic_store_irouter to reflect parameter
changes
- minor change: changed the macros ESPI_INTID2IDX and ESPI_IDX2INTID
into appropriate inline functions introduced in the previous patch
Changes in V5:
- since eSPI-specific defines and macros are now available even when the
appropriate config is disabled, this allows us to remove many
#ifdef guards and reuse the existing code for regular SPIs for eSPIs as
well, as eSPIs are processed similarly. This improves code readability
and consolidates the register ranges for SPIs and eSPIs in a single
place, simplifying future changes or fixes for SPIs and their
counterparts from the extended range
- moved vgic_ext_rank_offset() from
[08/12] xen/arm: vgic: add resource management for extended SPIs
as the function was unused before this patch
- added stub implementation of vgic_ext_rank_offset() when CONFIG_GICV3_ESPI=n
- removed unnecessary defines for reserved ranges, which were introduced in
V4 to reduce the number of #ifdefs. The issue is now resolved by
allowing the use of SPI-specific offsets and reworking the logic
Changes in V4:
- added missing RAZ and write ignore eSPI-specific registers ranges:
GICD_NSACRnE and GICD_IGRPMODRnE
- changed previously reserved range to cover GICD_NSACRnE and
GICD_IGRPMODRnE
- introduced GICD_RESERVED_RANGE<n>_START/END defines to remove
hardcoded values and reduce the number of ifdefs
- fixed reserved ranges with eSPI option enabled: added missing range
0x0F30-0x0F7C
- updated the logic for domains that do not support eSPI, but Xen is
compiled with the eSPI option. Now, prior to other MMIO checks, we
verify whether eSPI is available for the domain or not. If not, it
behaves as it does currently - RAZ and WI
- fixed print for GICD_ICACTIVERnE
- fixed new lines formatting for switch-case
Changes in V3:
- changed vgic_store_irouter parameters - instead of offset virq is
used, to remove the additional bool espi parameter and simplify
checks. Also, adjusted parameters for regular SPI. Since the offset
parameter was used only for calculating virq number and then reused for
finding rank offset, it will not affect functionality.
- fixed formatting for goto lables - added newlines after condition
- fixed logs for GICD_ISACTIVERnE and GICD_ICACTIVERnE handlers
- removed #ifdefs in 2 places where they were adjacent and could be merged
Changes in V2:
- add missing rank index conversion for pending and inflight irqs
---
xen/arch/arm/include/asm/vgic.h | 4 +
xen/arch/arm/vgic-v3.c | 198 +++++++++++++++++++++++++-------
xen/arch/arm/vgic.c | 22 ++++
3 files changed, 180 insertions(+), 44 deletions(-)
diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
index c52bbcb115..dec08a75a4 100644
--- a/xen/arch/arm/include/asm/vgic.h
+++ b/xen/arch/arm/include/asm/vgic.h
@@ -314,6 +314,10 @@ extern struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v,
unsigned int b,
unsigned int n,
unsigned int s);
+extern struct vgic_irq_rank *vgic_ext_rank_offset(struct vcpu *v,
+ unsigned int b,
+ unsigned int n,
+ unsigned int s);
extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq);
extern void vgic_disable_irqs(struct vcpu *v, uint32_t r, unsigned int n);
extern void vgic_enable_irqs(struct vcpu *v, uint32_t r, unsigned int n);
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 4369c55177..27af247d30 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -107,17 +107,12 @@ static uint64_t vgic_fetch_irouter(struct vgic_irq_rank *rank,
/*
* Store an IROUTER register in a convenient way and migrate the vIRQ
* if necessary. This function only deals with IROUTER32 and onwards.
- *
- * Note the offset will be aligned to the appropriate boundary.
*/
static void vgic_store_irouter(struct domain *d, struct vgic_irq_rank *rank,
- unsigned int offset, uint64_t irouter)
+ unsigned int virq, uint64_t irouter)
{
struct vcpu *new_vcpu, *old_vcpu;
- unsigned int virq;
-
- /* There is 1 vIRQ per IROUTER */
- virq = offset / NR_BYTES_PER_IROUTER;
+ unsigned int offset;
/*
* The IROUTER0-31, used for SGIs/PPIs, are reserved and should
@@ -673,6 +668,34 @@ write_reserved:
return 1;
}
+/*
+ * Since all eSPI counterparts of SPI registers belong to lower offsets,
+ * we can check whether the register offset is less than espi_base and,
+ * if so, return the rank for regular SPI. Otherwise, return the rank
+ * for eSPI.
+ */
+static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
+ unsigned int b,
+ uint32_t reg,
+ unsigned int s,
+ uint32_t spi_base,
+ uint32_t espi_base)
+{
+ if ( reg < espi_base )
+ return vgic_rank_offset(v, b, reg - spi_base, s);
+ else
+ return vgic_ext_rank_offset(v, b, reg - espi_base, s);
+}
+
+static inline uint32_t vgic_get_reg_offset(uint32_t reg, uint32_t spi_base,
+ uint32_t espi_base)
+{
+ if ( reg < espi_base )
+ return reg - spi_base;
+ else
+ return reg - espi_base;
+}
+
static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
mmio_info_t *info, uint32_t reg,
register_t *r)
@@ -685,13 +708,17 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
{
case VRANGE32(GICD_IGROUPR, GICD_IGROUPRN):
case VRANGE32(GICD_IGRPMODR, GICD_IGRPMODRN):
+ case VRANGE32(GICD_IGROUPRnE, GICD_IGROUPRnEN):
+ case VRANGE32(GICD_IGRPMODRnE, GICD_IGRPMODRnEN):
/* We do not implement security extensions for guests, read zero */
if ( dabt.size != DABT_WORD ) goto bad_width;
goto read_as_zero;
case VRANGE32(GICD_ISENABLER, GICD_ISENABLERN):
+ case VRANGE32(GICD_ISENABLERnE, GICD_ISENABLERnEN):
if ( dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 1, reg - GICD_ISENABLER, DABT_WORD);
+ rank = vgic_get_rank(v, 1, reg, DABT_WORD, GICD_ISENABLER,
+ GICD_ISENABLERnE);
if ( rank == NULL ) goto read_as_zero;
vgic_lock_rank(v, rank, flags);
*r = vreg_reg32_extract(rank->ienable, info);
@@ -699,8 +726,10 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
return 1;
case VRANGE32(GICD_ICENABLER, GICD_ICENABLERN):
+ case VRANGE32(GICD_ICENABLERnE, GICD_ICENABLERnEN):
if ( dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 1, reg - GICD_ICENABLER, DABT_WORD);
+ rank = vgic_get_rank(v, 1, reg, DABT_WORD, GICD_ICENABLER,
+ GICD_ICENABLERnE);
if ( rank == NULL ) goto read_as_zero;
vgic_lock_rank(v, rank, flags);
*r = vreg_reg32_extract(rank->ienable, info);
@@ -710,22 +739,29 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
/* Read the pending status of an IRQ via GICD/GICR is not supported */
case VRANGE32(GICD_ISPENDR, GICD_ISPENDRN):
case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
+ case VRANGE32(GICD_ISPENDRnE, GICD_ISPENDRnEN):
+ case VRANGE32(GICD_ICPENDRnE, GICD_ICPENDRnEN):
goto read_as_zero;
/* Read the active status of an IRQ via GICD/GICR is not supported */
case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
+ case VRANGE32(GICD_ISACTIVERnE, GICD_ISACTIVERnEN):
+ case VRANGE32(GICD_ICACTIVERnE, GICD_ICACTIVERnEN):
goto read_as_zero;
case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
+ case VRANGE32(GICD_IPRIORITYRnE, GICD_IPRIORITYRnEN):
{
- uint32_t ipriorityr;
+ uint32_t ipriorityr, offset;
uint8_t rank_index;
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
+ rank = vgic_get_rank(v, 8, reg, DABT_WORD, GICD_IPRIORITYR,
+ GICD_IPRIORITYRnE);
if ( rank == NULL ) goto read_as_zero;
- rank_index = REG_RANK_INDEX(8, reg - GICD_IPRIORITYR, DABT_WORD);
+ offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR, GICD_IPRIORITYRnE);
+ rank_index = REG_RANK_INDEX(8, offset, DABT_WORD);
vgic_lock_rank(v, rank, flags);
ipriorityr = ACCESS_ONCE(rank->ipriorityr[rank_index]);
@@ -737,14 +773,16 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
}
case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
+ case VRANGE32(GICD_ICFGRnE, GICD_ICFGRnEN):
{
- uint32_t icfgr;
+ uint32_t icfgr, offset;
if ( dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
+ rank = vgic_get_rank(v, 2, reg, DABT_WORD, GICD_ICFGR, GICD_ICFGRnE);
if ( rank == NULL ) goto read_as_zero;
+ offset = vgic_get_reg_offset(reg, GICD_ICFGR, GICD_ICFGRnE);
vgic_lock_rank(v, rank, flags);
- icfgr = rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR, DABT_WORD)];
+ icfgr = rank->icfg[REG_RANK_INDEX(2, offset, DABT_WORD)];
vgic_unlock_rank(v, rank, flags);
*r = vreg_reg32_extract(icfgr, info);
@@ -782,12 +820,16 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
{
case VRANGE32(GICD_IGROUPR, GICD_IGROUPRN):
case VRANGE32(GICD_IGRPMODR, GICD_IGRPMODRN):
+ case VRANGE32(GICD_IGROUPRnE, GICD_IGROUPRnEN):
+ case VRANGE32(GICD_IGRPMODRnE, GICD_IGRPMODRnEN):
/* We do not implement security extensions for guests, write ignore */
goto write_ignore_32;
case VRANGE32(GICD_ISENABLER, GICD_ISENABLERN):
+ case VRANGE32(GICD_ISENABLERnE, GICD_ISENABLERnEN):
if ( dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 1, reg - GICD_ISENABLER, DABT_WORD);
+ rank = vgic_get_rank(v, 1, reg, DABT_WORD, GICD_ISENABLER,
+ GICD_ISENABLERnE);
if ( rank == NULL ) goto write_ignore;
vgic_lock_rank(v, rank, flags);
tr = rank->ienable;
@@ -797,8 +839,10 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
return 1;
case VRANGE32(GICD_ICENABLER, GICD_ICENABLERN):
+ case VRANGE32(GICD_ICENABLERnE, GICD_ICENABLERnEN):
if ( dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 1, reg - GICD_ICENABLER, DABT_WORD);
+ rank = vgic_get_rank(v, 1, reg, DABT_WORD, GICD_ICENABLER,
+ GICD_ICENABLERnE);
if ( rank == NULL ) goto write_ignore;
vgic_lock_rank(v, rank, flags);
tr = rank->ienable;
@@ -808,8 +852,10 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
return 1;
case VRANGE32(GICD_ISPENDR, GICD_ISPENDRN):
+ case VRANGE32(GICD_ISPENDRnE, GICD_ISPENDRnEN):
if ( dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 1, reg - GICD_ISPENDR, DABT_WORD);
+ rank = vgic_get_rank(v, 1, reg, DABT_WORD, GICD_ISPENDR,
+ GICD_ISPENDRnE);
if ( rank == NULL ) goto write_ignore;
vgic_set_irqs_pending(v, r, rank->index);
@@ -817,8 +863,10 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
return 1;
case VRANGE32(GICD_ICPENDR, GICD_ICPENDRN):
+ case VRANGE32(GICD_ICPENDRnE, GICD_ICPENDRnEN):
if ( dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
+ rank = vgic_get_rank(v, 1, reg, DABT_WORD, GICD_ICPENDR,
+ GICD_ICPENDRnE);
if ( rank == NULL ) goto write_ignore;
vgic_check_inflight_irqs_pending(v, rank->index, r);
@@ -826,28 +874,42 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
goto write_ignore;
case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):
+ case VRANGE32(GICD_ISACTIVERnE, GICD_ISACTIVERnEN):
if ( dabt.size != DABT_WORD ) goto bad_width;
- printk(XENLOG_G_ERR
- "%pv: %s: unhandled word write %#"PRIregister" to ISACTIVER%d\n",
- v, name, r, reg - GICD_ISACTIVER);
+ if ( reg < GICD_ISACTIVERnE )
+ printk(XENLOG_G_ERR
+ "%pv: %s: unhandled word write %#"PRIregister" to ISACTIVER%d\n",
+ v, name, r, reg - GICD_ISACTIVER);
+ else
+ printk(XENLOG_G_ERR
+ "%pv: %s: unhandled word write %#"PRIregister" to ISACTIVER%dE\n",
+ v, name, r, reg - GICD_ISACTIVERnE);
return 0;
case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
- printk(XENLOG_G_ERR
- "%pv: %s: unhandled word write %#"PRIregister" to ICACTIVER%d\n",
- v, name, r, reg - GICD_ICACTIVER);
+ case VRANGE32(GICD_ICACTIVERnE, GICD_ICACTIVERnEN):
+ if ( reg < GICD_ICACTIVERnE )
+ printk(XENLOG_G_ERR
+ "%pv: %s: unhandled word write %#"PRIregister" to ICACTIVER%d\n",
+ v, name, r, reg - GICD_ICACTIVER);
+ else
+ printk(XENLOG_G_ERR
+ "%pv: %s: unhandled word write %#"PRIregister" to ICACTIVER%dE\n",
+ v, name, r, reg - GICD_ICACTIVERnE);
goto write_ignore_32;
case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
+ case VRANGE32(GICD_IPRIORITYRnE, GICD_IPRIORITYRnEN):
{
- uint32_t *ipriorityr, priority;
+ uint32_t *ipriorityr, priority, offset;
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
+ rank = vgic_get_rank(v, 8, reg, DABT_WORD, GICD_IPRIORITYR,
+ GICD_IPRIORITYRnE);
if ( rank == NULL ) goto write_ignore;
+ offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR, GICD_IPRIORITYRnE);
vgic_lock_rank(v, rank, flags);
- ipriorityr = &rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
- DABT_WORD)];
+ ipriorityr = &rank->ipriorityr[REG_RANK_INDEX(8, offset, DABT_WORD)];
priority = ACCESS_ONCE(*ipriorityr);
vreg_reg32_update(&priority, r, info);
ACCESS_ONCE(*ipriorityr) = priority;
@@ -859,17 +921,22 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
goto write_ignore_32;
case VRANGE32(GICD_ICFGR + 4, GICD_ICFGRN): /* PPI + SPIs */
+ case VRANGE32(GICD_ICFGRnE, GICD_ICFGRnEN):
+ {
+ uint32_t offset;
+
/* ICFGR1 for PPI's, which is implementation defined
if ICFGR1 is programmable or not. We chose to program */
if ( dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
+ rank = vgic_get_rank(v, 2, reg, DABT_WORD, GICD_ICFGR, GICD_ICFGRnE);
if ( rank == NULL ) goto write_ignore;
+ offset = vgic_get_reg_offset(reg, GICD_ICFGR, GICD_ICFGRnE);
vgic_lock_rank(v, rank, flags);
- vreg_reg32_update(&rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR,
- DABT_WORD)],
+ vreg_reg32_update(&rank->icfg[REG_RANK_INDEX(2, offset, DABT_WORD)],
r, info);
vgic_unlock_rank(v, rank, flags);
return 1;
+ }
default:
printk(XENLOG_G_ERR
@@ -1129,6 +1196,16 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
typer |= GICD_TYPE_LPIS;
typer |= (v->domain->arch.vgic.intid_bits - 1) << GICD_TYPE_ID_BITS_SHIFT;
+#ifdef CONFIG_GICV3_ESPI
+ if ( v->domain->arch.vgic.nr_espis > 0 )
+ {
+ /* Set eSPI support bit for the domain */
+ typer |= GICD_TYPER_ESPI;
+ /* Set ESPI range bits */
+ typer |= (DIV_ROUND_UP(v->domain->arch.vgic.nr_espis, 32) - 1)
+ << GICD_TYPER_ESPI_RANGE_SHIFT;
+ }
+#endif
*r = vreg_reg32_extract(typer, info);
@@ -1194,6 +1271,16 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
case VRANGE32(GICD_IGRPMODR, GICD_IGRPMODRN):
+ case VRANGE32(GICD_IGROUPRnE, GICD_IGROUPRnEN):
+ case VRANGE32(GICD_ISENABLERnE, GICD_ISENABLERnEN):
+ case VRANGE32(GICD_ICENABLERnE, GICD_ICENABLERnEN):
+ case VRANGE32(GICD_ISPENDRnE, GICD_ISPENDRnEN):
+ case VRANGE32(GICD_ICPENDRnE, GICD_ICPENDRnEN):
+ case VRANGE32(GICD_ISACTIVERnE, GICD_ISACTIVERnEN):
+ case VRANGE32(GICD_ICACTIVERnE, GICD_ICACTIVERnEN):
+ case VRANGE32(GICD_IPRIORITYRnE, GICD_IPRIORITYRnEN):
+ case VRANGE32(GICD_ICFGRnE, GICD_ICFGRnEN):
+ case VRANGE32(GICD_IGRPMODRnE, GICD_IGRPMODRnEN):
/*
* Above all register are common with GICR and GICD
* Manage in common
@@ -1201,6 +1288,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
return __vgic_v3_distr_common_mmio_read("vGICD", v, info, gicd_reg, r);
case VRANGE32(GICD_NSACR, GICD_NSACRN):
+ case VRANGE32(GICD_NSACRnE, GICD_NSACRnEN):
/* We do not implement security extensions for guests, read zero */
goto read_as_zero_32;
@@ -1216,27 +1304,30 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
/* Replaced with GICR_ISPENDR0. So ignore write */
goto read_as_zero_32;
- case VRANGE32(0x0F30, 0x60FC):
+ case VRANGE32(0x0F30, 0x0FFC):
goto read_reserved;
case VRANGE64(GICD_IROUTER32, GICD_IROUTER1019):
+ case VRANGE64(GICD_IROUTERnE, GICD_IROUTERnEN):
{
uint64_t irouter;
+ uint32_t offset;
if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
- rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER,
- DABT_DOUBLE_WORD);
+ rank = vgic_get_rank(v, 64, gicd_reg, DABT_DOUBLE_WORD, GICD_IROUTER,
+ GICD_IROUTERnE);
if ( rank == NULL ) goto read_as_zero;
+ offset = vgic_get_reg_offset(gicd_reg, GICD_IROUTER, GICD_IROUTERnE);
vgic_lock_rank(v, rank, flags);
- irouter = vgic_fetch_irouter(rank, gicd_reg - GICD_IROUTER);
+ irouter = vgic_fetch_irouter(rank, offset);
vgic_unlock_rank(v, rank, flags);
*r = vreg_reg64_extract(irouter, info);
return 1;
}
-
- case VRANGE32(0x7FE0, 0xBFFC):
+ case VRANGE32(0x3700, 0x60FC):
+ case VRANGE32(0xA004, 0xBFFC):
goto read_reserved;
case VRANGE32(0xC000, 0xFFCC):
@@ -1382,12 +1473,23 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
case VRANGE32(GICD_IGRPMODR, GICD_IGRPMODRN):
+ case VRANGE32(GICD_IGROUPRnE, GICD_IGROUPRnEN):
+ case VRANGE32(GICD_ISENABLERnE, GICD_ISENABLERnEN):
+ case VRANGE32(GICD_ICENABLERnE, GICD_ICENABLERnEN):
+ case VRANGE32(GICD_ISPENDRnE, GICD_ISPENDRnEN):
+ case VRANGE32(GICD_ICPENDRnE, GICD_ICPENDRnEN):
+ case VRANGE32(GICD_ISACTIVERnE, GICD_ISACTIVERnEN):
+ case VRANGE32(GICD_ICACTIVERnE, GICD_ICACTIVERnEN):
+ case VRANGE32(GICD_IPRIORITYRnE, GICD_IPRIORITYRnEN):
+ case VRANGE32(GICD_ICFGRnE, GICD_ICFGRnEN):
+ case VRANGE32(GICD_IGRPMODRnE, GICD_IGRPMODRnEN):
/* Above registers are common with GICR and GICD
* Manage in common */
return __vgic_v3_distr_common_mmio_write("vGICD", v, info,
gicd_reg, r);
case VRANGE32(GICD_NSACR, GICD_NSACRN):
+ case VRANGE32(GICD_NSACRnE, GICD_NSACRnEN):
/* We do not implement security extensions for guests, write ignore */
goto write_ignore_32;
@@ -1405,26 +1507,34 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
if ( dabt.size != DABT_WORD ) goto bad_width;
return 0;
- case VRANGE32(0x0F30, 0x60FC):
+ case VRANGE32(0x0F30, 0x0FFC):
goto write_reserved;
case VRANGE64(GICD_IROUTER32, GICD_IROUTER1019):
+ case VRANGE64(GICD_IROUTERnE, GICD_IROUTERnEN):
{
uint64_t irouter;
+ unsigned int offset, virq;
if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
- rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER,
- DABT_DOUBLE_WORD);
+ rank = vgic_get_rank(v, 64, gicd_reg, DABT_DOUBLE_WORD, GICD_IROUTER,
+ GICD_IROUTERnE);
if ( rank == NULL ) goto write_ignore;
+ offset = vgic_get_reg_offset(gicd_reg, GICD_IROUTER, GICD_IROUTERnE);
vgic_lock_rank(v, rank, flags);
- irouter = vgic_fetch_irouter(rank, gicd_reg - GICD_IROUTER);
+ irouter = vgic_fetch_irouter(rank, offset);
vreg_reg64_update(&irouter, r, info);
- vgic_store_irouter(v->domain, rank, gicd_reg - GICD_IROUTER, irouter);
+ if ( gicd_reg < GICD_IROUTERnE )
+ virq = offset / NR_BYTES_PER_IROUTER;
+ else
+ virq = espi_idx_to_intid(offset / NR_BYTES_PER_IROUTER);
+ vgic_store_irouter(v->domain, rank, virq, irouter);
vgic_unlock_rank(v, rank, flags);
return 1;
}
- case VRANGE32(0x7FE0, 0xBFFC):
+ case VRANGE32(0x3700, 0x60FC):
+ case VRANGE32(0xA004, 0xBFFC):
goto write_reserved;
case VRANGE32(0xC000, 0xFFCC):
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index b42aee8d0c..9458ba93f7 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -170,6 +170,18 @@ int domain_vgic_register(struct domain *d, unsigned int *mmio_count)
}
#ifdef CONFIG_GICV3_ESPI
+/*
+ * The function behavior is the same as for regular SPIs (vgic_rank_offset),
+ * but it operates with extended SPI ranks.
+ */
+struct vgic_irq_rank *vgic_ext_rank_offset(struct vcpu *v, unsigned int b,
+ unsigned int n, unsigned int s)
+{
+ unsigned int rank = REG_RANK_NR(b, (n >> s));
+
+ return vgic_get_rank(v, rank + EXT_RANK_MIN);
+}
+
static unsigned int vgic_num_spi_lines(struct domain *d)
{
return d->arch.vgic.nr_spis + d->arch.vgic.nr_espis;
@@ -210,6 +222,16 @@ static unsigned int vgic_num_spi_lines(struct domain *d)
return d->arch.vgic.nr_spis;
}
+/*
+ * It is expected that, in the case of CONFIG_GICV3_ESPI=n, the function will
+ * return NULL. In this scenario, mmio_read/mmio_write will be treated as
+ * RAZ/WI, as expected.
+ */
+struct vgic_irq_rank *vgic_ext_rank_offset(struct vcpu *v, unsigned int b,
+ unsigned int n, unsigned int s)
+{
+ return NULL;
+}
#endif
static unsigned int vgic_num_alloc_irqs(struct domain *d)
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v6 10/12] xen/arm: vgic-v3: add emulation of GICv3.1 eSPI registers
2025-09-03 14:30 ` [PATCH v6 10/12] xen/arm: vgic-v3: add emulation of GICv3.1 eSPI registers Leonid Komarianskyi
@ 2025-09-03 19:27 ` Oleksandr Tyshchenko
2025-09-03 21:37 ` Volodymyr Babchuk
2025-09-04 6:15 ` Leonid Komarianskyi
0 siblings, 2 replies; 34+ messages in thread
From: Oleksandr Tyshchenko @ 2025-09-03 19:27 UTC (permalink / raw)
To: Leonid Komarianskyi, xen-devel@lists.xenproject.org
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
On 03.09.25 17:30, Leonid Komarianskyi wrote:
Hello Leonid
> Implemented support for GICv3.1 extended SPI registers for vGICv3,
> allowing the emulation of eSPI-specific behavior for guest domains.
> The implementation includes read and write emulation for eSPI-related
> registers (e.g., GICD_ISENABLERnE, GICD_IROUTERnE, and others),
> following a similar approach to the handling of regular SPIs.
>
> The eSPI registers, previously located in reserved address ranges,
> are now adjusted to support MMIO read and write operations correctly
> when CONFIG_GICV3_ESPI is enabled.
>
> The availability of eSPIs and the number of emulated extended SPIs
> for guest domains is reported by setting the appropriate bits in the
> GICD_TYPER register, based on the number of eSPIs requested by the
> domain and supported by the hardware. In cases where the configuration
> option is disabled, the hardware does not support eSPIs, or the domain
> does not request such interrupts, the functionality remains unchanged.
>
> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
>
> ---
> Changes in V6:
> - introduced helper functions: vgic_get_rank and vgic_get_reg_offset to
> avoid boilerplate code and simplify changes
> - fixed index initialization in the previous patch ([08/12] xen/arm:
> vgic: add resource management for extended SPIs) and removed index
> conversion for vgic_enable_irqs(), vgic_disable_irqs(),
> vgic_set_irqs_pending(), and vgic_check_inflight_irqs_pending()
> - grouped SPI and eSPI registers
> - updated the comment for vgic_store_irouter to reflect parameter
> changes
> - minor change: changed the macros ESPI_INTID2IDX and ESPI_IDX2INTID
> into appropriate inline functions introduced in the previous patch
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
preferably with the comments below
>
> Changes in V5:
> - since eSPI-specific defines and macros are now available even when the
> appropriate config is disabled, this allows us to remove many
> #ifdef guards and reuse the existing code for regular SPIs for eSPIs as
> well, as eSPIs are processed similarly. This improves code readability
> and consolidates the register ranges for SPIs and eSPIs in a single
> place, simplifying future changes or fixes for SPIs and their
> counterparts from the extended range
> - moved vgic_ext_rank_offset() from
> [08/12] xen/arm: vgic: add resource management for extended SPIs
> as the function was unused before this patch
> - added stub implementation of vgic_ext_rank_offset() when CONFIG_GICV3_ESPI=n
> - removed unnecessary defines for reserved ranges, which were introduced in
> V4 to reduce the number of #ifdefs. The issue is now resolved by
> allowing the use of SPI-specific offsets and reworking the logic
>
> Changes in V4:
> - added missing RAZ and write ignore eSPI-specific registers ranges:
> GICD_NSACRnE and GICD_IGRPMODRnE
> - changed previously reserved range to cover GICD_NSACRnE and
> GICD_IGRPMODRnE
> - introduced GICD_RESERVED_RANGE<n>_START/END defines to remove
> hardcoded values and reduce the number of ifdefs
> - fixed reserved ranges with eSPI option enabled: added missing range
> 0x0F30-0x0F7C
> - updated the logic for domains that do not support eSPI, but Xen is
> compiled with the eSPI option. Now, prior to other MMIO checks, we
> verify whether eSPI is available for the domain or not. If not, it
> behaves as it does currently - RAZ and WI
> - fixed print for GICD_ICACTIVERnE
> - fixed new lines formatting for switch-case
>
> Changes in V3:
> - changed vgic_store_irouter parameters - instead of offset virq is
> used, to remove the additional bool espi parameter and simplify
> checks. Also, adjusted parameters for regular SPI. Since the offset
> parameter was used only for calculating virq number and then reused for
> finding rank offset, it will not affect functionality.
> - fixed formatting for goto lables - added newlines after condition
> - fixed logs for GICD_ISACTIVERnE and GICD_ICACTIVERnE handlers
> - removed #ifdefs in 2 places where they were adjacent and could be merged
>
> Changes in V2:
> - add missing rank index conversion for pending and inflight irqs
> ---
> xen/arch/arm/include/asm/vgic.h | 4 +
> xen/arch/arm/vgic-v3.c | 198 +++++++++++++++++++++++++-------
> xen/arch/arm/vgic.c | 22 ++++
> 3 files changed, 180 insertions(+), 44 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
> index c52bbcb115..dec08a75a4 100644
> --- a/xen/arch/arm/include/asm/vgic.h
> +++ b/xen/arch/arm/include/asm/vgic.h
> @@ -314,6 +314,10 @@ extern struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v,
> unsigned int b,
> unsigned int n,
> unsigned int s);
> +extern struct vgic_irq_rank *vgic_ext_rank_offset(struct vcpu *v,
> + unsigned int b,
> + unsigned int n,
> + unsigned int s);
> extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq);
> extern void vgic_disable_irqs(struct vcpu *v, uint32_t r, unsigned int n);
> extern void vgic_enable_irqs(struct vcpu *v, uint32_t r, unsigned int n);
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 4369c55177..27af247d30 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -107,17 +107,12 @@ static uint64_t vgic_fetch_irouter(struct vgic_irq_rank *rank,
> /*
> * Store an IROUTER register in a convenient way and migrate the vIRQ
> * if necessary. This function only deals with IROUTER32 and onwards.
> - *
> - * Note the offset will be aligned to the appropriate boundary.
> */
> static void vgic_store_irouter(struct domain *d, struct vgic_irq_rank *rank,
> - unsigned int offset, uint64_t irouter)
> + unsigned int virq, uint64_t irouter)
> {
> struct vcpu *new_vcpu, *old_vcpu;
> - unsigned int virq;
> -
> - /* There is 1 vIRQ per IROUTER */
You seem to have dropped a comment, but not just moved it to virq
calculation outside of the vgic_store_irouter() in "case
VRANGE64(GICD_IROUTERnE, GICD_IROUTERnEN):". If the comment is valid (I
assume so), it would be better to retain it.
> - virq = offset / NR_BYTES_PER_IROUTER;
> + unsigned int offset;
>
> /*
> * The IROUTER0-31, used for SGIs/PPIs, are reserved and should
> @@ -673,6 +668,34 @@ write_reserved:
> return 1;
> }
>
> +/*
> + * Since all eSPI counterparts of SPI registers belong to lower offsets,
> + * we can check whether the register offset is less than espi_base and,
> + * if so, return the rank for regular SPI. Otherwise, return the rank
> + * for eSPI.
> + */
NIT: I would just write the following:
The assumption is that offsets below espi_base are for regular SPI,
while those at or above are for eSPI.
> +static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
> + unsigned int b,
> + uint32_t reg,
> + unsigned int s,
> + uint32_t spi_base,
> + uint32_t espi_base)
I find the name "vgic_get_rank" a bit confusing since the vgic.c already
contains the helper with the same name:
static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
unsigned int rank)
So what we have for regular SPIs is:
vgic_get_rank()->vgic_rank_offset()->vgic_get_rank()
and for eSPIs is:
vgic_get_rank()->vgic_ext_rank_offset()->vgic_get_rank()
I would rename it, e.g. vgic_common_rank_offset (not ideal though)
> +{
> + if ( reg < espi_base )
> + return vgic_rank_offset(v, b, reg - spi_base, s);
> + else
> + return vgic_ext_rank_offset(v, b, reg - espi_base, s);
> +}
> +
> +static inline uint32_t vgic_get_reg_offset(uint32_t reg, uint32_t spi_base,
> + uint32_t espi_base)
> +{
> + if ( reg < espi_base )
> + return reg - spi_base;
> + else
> + return reg - espi_base;
> +}
I am wondering (I do not request a change) whether vgic_get_reg_offset()
is really helpfull,
e.g. is
offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR, GICD_IPRIORITYRnE);
much better than:
offset = reg < GICD_IPRIORITYRnE ? reg - GICD_IPRIORITYR : reg -
GICD_IPRIORITYRnE;
?
[snip]
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v6 10/12] xen/arm: vgic-v3: add emulation of GICv3.1 eSPI registers
2025-09-03 19:27 ` Oleksandr Tyshchenko
@ 2025-09-03 21:37 ` Volodymyr Babchuk
2025-09-03 23:04 ` Julien Grall
2025-09-04 10:49 ` Oleksandr Tyshchenko
2025-09-04 6:15 ` Leonid Komarianskyi
1 sibling, 2 replies; 34+ messages in thread
From: Volodymyr Babchuk @ 2025-09-03 21:37 UTC (permalink / raw)
To: Oleksandr Tyshchenko
Cc: Leonid Komarianskyi, xen-devel@lists.xenproject.org,
Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel
Hi Oleksandr,
Oleksandr Tyshchenko <olekstysh@gmail.com> writes:
[...]
>> +static inline uint32_t vgic_get_reg_offset(uint32_t reg, uint32_t spi_base,
>> + uint32_t espi_base)
>> +{
>> + if ( reg < espi_base )
>> + return reg - spi_base;
>> + else
>> + return reg - espi_base;
>> +}
>
> I am wondering (I do not request a change) whether
> vgic_get_reg_offset() is really helpfull,
> e.g. is
> offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR, GICD_IPRIORITYRnE);
> much better than:
> offset = reg < GICD_IPRIORITYRnE ? reg - GICD_IPRIORITYR : reg -
> GICD_IPRIORITYRnE;
>
IMO, it is easy to make a mistake, because you need to write register
name 3 times. Can cause errors during copy-pasting. But I saw clever
trick by Mykola Kvach, something like this:
#define vgic_get_reg_offset(addr, reg_name) ( addr < reg_name##nE ? \
addr - reg_name : addr - reg_name##nE )
And then you can just use this as
offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR)
I don't know what maintainers think about this type of preprocessor
trickery, but in my opinion it is justified in this case, because it
leaves less room for a mistake.
--
WBR, Volodymyr
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v6 10/12] xen/arm: vgic-v3: add emulation of GICv3.1 eSPI registers
2025-09-03 21:37 ` Volodymyr Babchuk
@ 2025-09-03 23:04 ` Julien Grall
2025-09-04 5:52 ` Leonid Komarianskyi
2025-09-04 10:49 ` Oleksandr Tyshchenko
1 sibling, 1 reply; 34+ messages in thread
From: Julien Grall @ 2025-09-03 23:04 UTC (permalink / raw)
To: Volodymyr Babchuk, Oleksandr Tyshchenko
Cc: Leonid Komarianskyi, xen-devel@lists.xenproject.org,
Stefano Stabellini, Bertrand Marquis, Michal Orzel
Hi,
On 03/09/2025 22:37, Volodymyr Babchuk wrote:
> Hi Oleksandr,
>
> Oleksandr Tyshchenko <olekstysh@gmail.com> writes:
>
>
> [...]
>
>>> +static inline uint32_t vgic_get_reg_offset(uint32_t reg, uint32_t spi_base,
>>> + uint32_t espi_base)
>>> +{
>>> + if ( reg < espi_base )
>>> + return reg - spi_base;
>>> + else
>>> + return reg - espi_base;
>>> +}
>>
>> I am wondering (I do not request a change) whether
>> vgic_get_reg_offset() is really helpfull,
>> e.g. is
>> offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR, GICD_IPRIORITYRnE);
>> much better than:
>> offset = reg < GICD_IPRIORITYRnE ? reg - GICD_IPRIORITYR : reg -
>> GICD_IPRIORITYRnE;
>>>
> IMO, it is easy to make a mistake, because you need to write register
> name 3 times. Can cause errors during copy-pasting.
+1.
But I saw clever
> trick by Mykola Kvach, something like this:
>
> #define vgic_get_reg_offset(addr, reg_name) ( addr < reg_name##nE ? \
> addr - reg_name : addr - reg_name##nE )
>
> And then you can just use this as
>
> offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR)
>
> I don't know what maintainers think about this type of preprocessor
> trickery, but in my opinion it is justified in this case, because it
> leaves less room for a mistake.
I don't have a strong opinion between the macro version or the static
inline helper. However:
* for the macro version, you want to store 'addr' in a local variable
to ensure it is only evaluated once.
* for both case, I would prefer if we assert (for the static inline
helper) or use BUILD_BUG_ON() to confirm that spi_base < espi_base
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v6 10/12] xen/arm: vgic-v3: add emulation of GICv3.1 eSPI registers
2025-09-03 23:04 ` Julien Grall
@ 2025-09-04 5:52 ` Leonid Komarianskyi
2025-09-04 11:17 ` Oleksandr Tyshchenko
0 siblings, 1 reply; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-04 5:52 UTC (permalink / raw)
To: Julien Grall, Volodymyr Babchuk, Oleksandr Tyshchenko
Cc: xen-devel@lists.xenproject.org, Stefano Stabellini,
Bertrand Marquis, Michal Orzel
Hi Julien, Volodymyr and Oleksandr,
Thank you for your comments.
On 04.09.25 02:04, Julien Grall wrote:
> Hi,
>
> On 03/09/2025 22:37, Volodymyr Babchuk wrote:
>> Hi Oleksandr,
>>
>> Oleksandr Tyshchenko <olekstysh@gmail.com> writes:
>>
>>
>> [...]
>>
>>>> +static inline uint32_t vgic_get_reg_offset(uint32_t reg, uint32_t
>>>> spi_base,
>>>> + uint32_t espi_base)
>>>> +{
>>>> + if ( reg < espi_base )
>>>> + return reg - spi_base;
>>>> + else
>>>> + return reg - espi_base;
>>>> +}
>>>
>>> I am wondering (I do not request a change) whether
>>> vgic_get_reg_offset() is really helpfull,
>>> e.g. is
>>> offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR, GICD_IPRIORITYRnE);
>>> much better than:
>>> offset = reg < GICD_IPRIORITYRnE ? reg - GICD_IPRIORITYR : reg -
>>> GICD_IPRIORITYRnE;
> >>>
>> IMO, it is easy to make a mistake, because you need to write register
>> name 3 times. Can cause errors during copy-pasting.
>
> +1.
>
> But I saw clever
>> trick by Mykola Kvach, something like this:
>>
>> #define vgic_get_reg_offset(addr, reg_name) ( addr < reg_name##nE ? \
>> addr - reg_name : addr - reg_name##nE )
>>
>> And then you can just use this as
>>
>> offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR)
>>
>> I don't know what maintainers think about this type of preprocessor
>> trickery, but in my opinion it is justified in this case, because it
>> leaves less room for a mistake.
>
> I don't have a strong opinion between the macro version or the static
> inline helper. However:
> * for the macro version, you want to store 'addr' in a local variable
> to ensure it is only evaluated once.
> * for both case, I would prefer if we assert (for the static inline
> helper) or use BUILD_BUG_ON() to confirm that spi_base < espi_base
>
> Cheers,
>
I was considering introducing this kind of macro, but I think it may
lead to issues in the future because it requires us to always maintain
the pattern reg_name/reg_name##nE for all registers. I understand that
the names of the defines are unlikely to change, but I would prefer to
use an inline function along with the suggested ASSERT(), as it seems
more versatile to me.
Best regards,
Leonid
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v6 10/12] xen/arm: vgic-v3: add emulation of GICv3.1 eSPI registers
2025-09-04 5:52 ` Leonid Komarianskyi
@ 2025-09-04 11:17 ` Oleksandr Tyshchenko
0 siblings, 0 replies; 34+ messages in thread
From: Oleksandr Tyshchenko @ 2025-09-04 11:17 UTC (permalink / raw)
To: Leonid Komarianskyi, Julien Grall, Volodymyr Babchuk
Cc: xen-devel@lists.xenproject.org, Stefano Stabellini,
Bertrand Marquis, Michal Orzel
On 04.09.25 08:52, Leonid Komarianskyi wrote:
> Hi Julien, Volodymyr and Oleksandr,
Hello all
>
> Thank you for your comments.
>
> On 04.09.25 02:04, Julien Grall wrote:
>> Hi,
>>
>> On 03/09/2025 22:37, Volodymyr Babchuk wrote:
>>> Hi Oleksandr,
>>>
>>> Oleksandr Tyshchenko <olekstysh@gmail.com> writes:
>>>
>>>
>>> [...]
>>>
>>>>> +static inline uint32_t vgic_get_reg_offset(uint32_t reg, uint32_t
>>>>> spi_base,
>>>>> + uint32_t espi_base)
>>>>> +{
>>>>> + if ( reg < espi_base )
>>>>> + return reg - spi_base;
>>>>> + else
>>>>> + return reg - espi_base;
>>>>> +}
>>>>
>>>> I am wondering (I do not request a change) whether
>>>> vgic_get_reg_offset() is really helpfull,
>>>> e.g. is
>>>> offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR, GICD_IPRIORITYRnE);
>>>> much better than:
>>>> offset = reg < GICD_IPRIORITYRnE ? reg - GICD_IPRIORITYR : reg -
>>>> GICD_IPRIORITYRnE;
>> >>>
>>> IMO, it is easy to make a mistake, because you need to write register
>>> name 3 times. Can cause errors during copy-pasting.
>>
>> +1.
>>
>> But I saw clever
>>> trick by Mykola Kvach, something like this:
>>>
>>> #define vgic_get_reg_offset(addr, reg_name) ( addr < reg_name##nE ? \
>>> addr - reg_name : addr - reg_name##nE )
>>>
>>> And then you can just use this as
>>>
>>> offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR)
>>>
>>> I don't know what maintainers think about this type of preprocessor
>>> trickery, but in my opinion it is justified in this case, because it
>>> leaves less room for a mistake.
>>
>> I don't have a strong opinion between the macro version or the static
>> inline helper. However:
>> * for the macro version, you want to store 'addr' in a local variable
>> to ensure it is only evaluated once.
>> * for both case, I would prefer if we assert (for the static inline
>> helper) or use BUILD_BUG_ON() to confirm that spi_base < espi_base
>>
>> Cheers,
>>
>
> I was considering introducing this kind of macro, but I think it may
> lead to issues in the future because it requires us to always maintain
> the pattern reg_name/reg_name##nE for all registers. I understand that
> the names of the defines are unlikely to change, but I would prefer to
> use an inline function along with the suggested ASSERT(), as it seems
> more versatile to me.
I was leaning more towards the macro, but would be happy with static
inline helper (my R-b will stay).
I guess, the suggested ASSERT() for the static inline
helper vgic_get_reg_offset would be also applicable for helper
vgic_get_rank (or whatever name it will gain).
>
> Best regards,
> Leonid
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 10/12] xen/arm: vgic-v3: add emulation of GICv3.1 eSPI registers
2025-09-03 21:37 ` Volodymyr Babchuk
2025-09-03 23:04 ` Julien Grall
@ 2025-09-04 10:49 ` Oleksandr Tyshchenko
1 sibling, 0 replies; 34+ messages in thread
From: Oleksandr Tyshchenko @ 2025-09-04 10:49 UTC (permalink / raw)
To: Volodymyr Babchuk
Cc: Leonid Komarianskyi, xen-devel@lists.xenproject.org,
Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel
On 04.09.25 00:37, Volodymyr Babchuk wrote:
> Hi Oleksandr,
Hello Volodymyr
>
> Oleksandr Tyshchenko <olekstysh@gmail.com> writes:
>
>
> [...]
>
>>> +static inline uint32_t vgic_get_reg_offset(uint32_t reg, uint32_t spi_base,
>>> + uint32_t espi_base)
>>> +{
>>> + if ( reg < espi_base )
>>> + return reg - spi_base;
>>> + else
>>> + return reg - espi_base;
>>> +}
>>
>> I am wondering (I do not request a change) whether
>> vgic_get_reg_offset() is really helpfull,
>> e.g. is
>> offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR, GICD_IPRIORITYRnE);
>> much better than:
>> offset = reg < GICD_IPRIORITYRnE ? reg - GICD_IPRIORITYR : reg -
>> GICD_IPRIORITYRnE;
>>
>
> IMO, it is easy to make a mistake, because you need to write register
> name 3 times. Can cause errors during copy-pasting.
I got it
But I saw clever
> trick by Mykola Kvach, something like this:
>
> #define vgic_get_reg_offset(addr, reg_name) ( addr < reg_name##nE ? \
> addr - reg_name : addr - reg_name##nE )
>
> And then you can just use this as
>
> offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR)
From my PoV, the idea looks good
>
> I don't know what maintainers think about this type of preprocessor
> trickery, but in my opinion it is justified in this case, because it
> leaves less room for a mistake.
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 10/12] xen/arm: vgic-v3: add emulation of GICv3.1 eSPI registers
2025-09-03 19:27 ` Oleksandr Tyshchenko
2025-09-03 21:37 ` Volodymyr Babchuk
@ 2025-09-04 6:15 ` Leonid Komarianskyi
1 sibling, 0 replies; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-04 6:15 UTC (permalink / raw)
To: Oleksandr Tyshchenko, xen-devel@lists.xenproject.org
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Hello Oleksandr,
Thank you for your review, comments and RB.
On 03.09.25 22:27, Oleksandr Tyshchenko wrote:
>
>
> On 03.09.25 17:30, Leonid Komarianskyi wrote:
>
> Hello Leonid
>
>> Implemented support for GICv3.1 extended SPI registers for vGICv3,
>> allowing the emulation of eSPI-specific behavior for guest domains.
>> The implementation includes read and write emulation for eSPI-related
>> registers (e.g., GICD_ISENABLERnE, GICD_IROUTERnE, and others),
>> following a similar approach to the handling of regular SPIs.
>>
>> The eSPI registers, previously located in reserved address ranges,
>> are now adjusted to support MMIO read and write operations correctly
>> when CONFIG_GICV3_ESPI is enabled.
>>
>> The availability of eSPIs and the number of emulated extended SPIs
>> for guest domains is reported by setting the appropriate bits in the
>> GICD_TYPER register, based on the number of eSPIs requested by the
>> domain and supported by the hardware. In cases where the configuration
>> option is disabled, the hardware does not support eSPIs, or the domain
>> does not request such interrupts, the functionality remains unchanged.
>>
>> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
>>
>> ---
>> Changes in V6:
>> - introduced helper functions: vgic_get_rank and vgic_get_reg_offset to
>> avoid boilerplate code and simplify changes
>> - fixed index initialization in the previous patch ([08/12] xen/arm:
>> vgic: add resource management for extended SPIs) and removed index
>> conversion for vgic_enable_irqs(), vgic_disable_irqs(),
>> vgic_set_irqs_pending(), and vgic_check_inflight_irqs_pending()
>> - grouped SPI and eSPI registers
>> - updated the comment for vgic_store_irouter to reflect parameter
>> changes
>> - minor change: changed the macros ESPI_INTID2IDX and ESPI_IDX2INTID
>> into appropriate inline functions introduced in the previous patch
>
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> preferably with the comments below
>
>>
>> Changes in V5:
>> - since eSPI-specific defines and macros are now available even when the
>> appropriate config is disabled, this allows us to remove many
>> #ifdef guards and reuse the existing code for regular SPIs for
>> eSPIs as
>> well, as eSPIs are processed similarly. This improves code readability
>> and consolidates the register ranges for SPIs and eSPIs in a single
>> place, simplifying future changes or fixes for SPIs and their
>> counterparts from the extended range
>> - moved vgic_ext_rank_offset() from
>> [08/12] xen/arm: vgic: add resource management for extended SPIs
>> as the function was unused before this patch
>> - added stub implementation of vgic_ext_rank_offset() when
>> CONFIG_GICV3_ESPI=n
>> - removed unnecessary defines for reserved ranges, which were
>> introduced in
>> V4 to reduce the number of #ifdefs. The issue is now resolved by
>> allowing the use of SPI-specific offsets and reworking the logic
>>
>> Changes in V4:
>> - added missing RAZ and write ignore eSPI-specific registers ranges:
>> GICD_NSACRnE and GICD_IGRPMODRnE
>> - changed previously reserved range to cover GICD_NSACRnE and
>> GICD_IGRPMODRnE
>> - introduced GICD_RESERVED_RANGE<n>_START/END defines to remove
>> hardcoded values and reduce the number of ifdefs
>> - fixed reserved ranges with eSPI option enabled: added missing range
>> 0x0F30-0x0F7C
>> - updated the logic for domains that do not support eSPI, but Xen is
>> compiled with the eSPI option. Now, prior to other MMIO checks, we
>> verify whether eSPI is available for the domain or not. If not, it
>> behaves as it does currently - RAZ and WI
>> - fixed print for GICD_ICACTIVERnE
>> - fixed new lines formatting for switch-case
>>
>> Changes in V3:
>> - changed vgic_store_irouter parameters - instead of offset virq is
>> used, to remove the additional bool espi parameter and simplify
>> checks. Also, adjusted parameters for regular SPI. Since the offset
>> parameter was used only for calculating virq number and then reused
>> for
>> finding rank offset, it will not affect functionality.
>> - fixed formatting for goto lables - added newlines after condition
>> - fixed logs for GICD_ISACTIVERnE and GICD_ICACTIVERnE handlers
>> - removed #ifdefs in 2 places where they were adjacent and could be
>> merged
>>
>> Changes in V2:
>> - add missing rank index conversion for pending and inflight irqs
>> ---
>> xen/arch/arm/include/asm/vgic.h | 4 +
>> xen/arch/arm/vgic-v3.c | 198 +++++++++++++++++++++++++-------
>> xen/arch/arm/vgic.c | 22 ++++
>> 3 files changed, 180 insertions(+), 44 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/
>> asm/vgic.h
>> index c52bbcb115..dec08a75a4 100644
>> --- a/xen/arch/arm/include/asm/vgic.h
>> +++ b/xen/arch/arm/include/asm/vgic.h
>> @@ -314,6 +314,10 @@ extern struct vgic_irq_rank
>> *vgic_rank_offset(struct vcpu *v,
>> unsigned int b,
>> unsigned int n,
>> unsigned int s);
>> +extern struct vgic_irq_rank *vgic_ext_rank_offset(struct vcpu *v,
>> + unsigned int b,
>> + unsigned int n,
>> + unsigned int s);
>> extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned
>> int irq);
>> extern void vgic_disable_irqs(struct vcpu *v, uint32_t r, unsigned
>> int n);
>> extern void vgic_enable_irqs(struct vcpu *v, uint32_t r, unsigned
>> int n);
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index 4369c55177..27af247d30 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -107,17 +107,12 @@ static uint64_t vgic_fetch_irouter(struct
>> vgic_irq_rank *rank,
>> /*
>> * Store an IROUTER register in a convenient way and migrate the vIRQ
>> * if necessary. This function only deals with IROUTER32 and onwards.
>> - *
>> - * Note the offset will be aligned to the appropriate boundary.
>> */
>> static void vgic_store_irouter(struct domain *d, struct
>> vgic_irq_rank *rank,
>> - unsigned int offset, uint64_t irouter)
>> + unsigned int virq, uint64_t irouter)
>> {
>> struct vcpu *new_vcpu, *old_vcpu;
>> - unsigned int virq;
>> -
>> - /* There is 1 vIRQ per IROUTER */
>
> You seem to have dropped a comment, but not just moved it to virq
> calculation outside of the vgic_store_irouter() in "case
> VRANGE64(GICD_IROUTERnE, GICD_IROUTERnEN):". If the comment is valid (I
> assume so), it would be better to retain it.
>
Okay, I will restore the comment.
>> - virq = offset / NR_BYTES_PER_IROUTER;
>> + unsigned int offset;
>> /*
>> * The IROUTER0-31, used for SGIs/PPIs, are reserved and should
>> @@ -673,6 +668,34 @@ write_reserved:
>> return 1;
>> }
>> +/*
>> + * Since all eSPI counterparts of SPI registers belong to lower offsets,
>> + * we can check whether the register offset is less than espi_base and,
>> + * if so, return the rank for regular SPI. Otherwise, return the rank
>> + * for eSPI.
>> + */
>
> NIT: I would just write the following:
>
> The assumption is that offsets below espi_base are for regular SPI,
> while those at or above are for eSPI.
>
I agree, your comment is simpler and easier to understand. :) Thank you,
I will update it to your version.
>> +static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
>> + unsigned int b,
>> + uint32_t reg,
>> + unsigned int s,
>> + uint32_t spi_base,
>> + uint32_t espi_base)
>
> I find the name "vgic_get_rank" a bit confusing since the vgic.c already
> contains the helper with the same name:
>
> static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
> unsigned int rank)
>
> So what we have for regular SPIs is:
> vgic_get_rank()->vgic_rank_offset()->vgic_get_rank()
> and for eSPIs is:
> vgic_get_rank()->vgic_ext_rank_offset()->vgic_get_rank()
>
> I would rename it, e.g. vgic_common_rank_offset (not ideal though)
>
>
I wanted to use a short name for it to avoid long lines with function
calls because it has six parameters. I chose this naming, but then
realized that a static function with the same name is already present in
vgic.c, but I decided to leave it...
But yes, I agree - the call stack looks weird in this case, so I will
rename the function to vgic_common_rank_offset().
>> +{
>> + if ( reg < espi_base )
>> + return vgic_rank_offset(v, b, reg - spi_base, s);
>> + else
>> + return vgic_ext_rank_offset(v, b, reg - espi_base, s);
>> +}
>> +
>> +static inline uint32_t vgic_get_reg_offset(uint32_t reg, uint32_t
>> spi_base,
>> + uint32_t espi_base)
>> +{
>> + if ( reg < espi_base )
>> + return reg - spi_base;
>> + else
>> + return reg - espi_base;
>> +}
>
> I am wondering (I do not request a change) whether vgic_get_reg_offset()
> is really helpfull,
> e.g. is
> offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR, GICD_IPRIORITYRnE);
> much better than:
> offset = reg < GICD_IPRIORITYRnE ? reg - GICD_IPRIORITYR : reg -
> GICD_IPRIORITYRnE;
>
> ?
>
> [snip]
Best regards,
Leonid
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v6 11/12] doc/man: update description for nr_spis with eSPI
2025-09-03 14:29 [PATCH v6 00/12] Introduce eSPI support Leonid Komarianskyi
` (9 preceding siblings ...)
2025-09-03 14:30 ` [PATCH v6 10/12] xen/arm: vgic-v3: add emulation of GICv3.1 eSPI registers Leonid Komarianskyi
@ 2025-09-03 14:30 ` Leonid Komarianskyi
2025-09-03 14:30 ` [PATCH v6 12/12] CHANGELOG.md: add mention of GICv3.1 eSPI support Leonid Komarianskyi
11 siblings, 0 replies; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-03 14:30 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: olekstysh@gmail.com, Leonid Komarianskyi, Anthony PERARD
Since eSPI support has been introduced, update the documentation with
the appropriate description.
Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
---
The discussion is ongoing and can be addressed in V5.
Clarification is needed from the maintainers.
Link:
- https://lore.kernel.org/xen-devel/87y0r4z717.fsf@epam.com/
Changes in V6:
- no changes
Changes in V5:
- no changes
Changes in V4:
- introduced this patch
---
docs/man/xl.cfg.5.pod.in | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 5362fb0e9a..292ab10331 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -3072,11 +3072,14 @@ interval of colors (such as "0-4").
=item B<nr_spis="NR_SPIS">
An optional integer parameter specifying the number of SPIs (Shared
-Peripheral Interrupts) to allocate for the domain. Max is 960 SPIs. If
-the `nr_spis` parameter is not specified, the value calculated by the toolstack
-will be used for the domain. Otherwise, the value specified by the `nr_spis`
-parameter will be used. The number of SPIs should match the highest interrupt
-ID that will be assigned to the domain.
+Peripheral Interrupts) to allocate for the domain. Max is 960 for regular SPIs
+or 5088 for eSPIs (Extended SPIs). The eSPIs includes an additional 1024 SPIs
+from the eSPI range (4096 to 5119) if the hardware supports extended SPIs
+(GICv3.1+) and CONFIG_GICV3_ESPI is enabled. If the `nr_spis` parameter is not
+specified, the value calculated by the toolstack will be used for the domain.
+Otherwise, the value specified by the `nr_spis` parameter will be used. The
+number of SPIs should match the highest interrupt ID that will be assigned to
+the domain.
=item B<trap_unmapped_accesses=BOOLEAN>
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v6 12/12] CHANGELOG.md: add mention of GICv3.1 eSPI support
2025-09-03 14:29 [PATCH v6 00/12] Introduce eSPI support Leonid Komarianskyi
` (10 preceding siblings ...)
2025-09-03 14:30 ` [PATCH v6 11/12] doc/man: update description for nr_spis with eSPI Leonid Komarianskyi
@ 2025-09-03 14:30 ` Leonid Komarianskyi
11 siblings, 0 replies; 34+ messages in thread
From: Leonid Komarianskyi @ 2025-09-03 14:30 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: olekstysh@gmail.com, Leonid Komarianskyi, Oleksii Kurochko,
Community Manager
The GICv3.1 eSPI (Extended Shared Peripheral Interrupts) range is
already supported with CONFIG_GICV3_ESPI enabled, so this feature should
be mentioned in CHANGELOG.md.
Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
---
Changes in V6:
- no changes
Changes in V5:
- extended changelog update with a brief explanation of what eSPI is
- added acked-by from Oleksii Kurochko, which was received for V3:
https://lore.kernel.org/xen-devel/bce5e07c-eee7-481b-a833-4d5d8bbce78f@gmail.com/
Changes in V4:
- no changes
Changes in V3:
- introduced this patch
---
CHANGELOG.md | 2 ++
1 file changed, 2 insertions(+)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5f31ca08fe..31b4ded444 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -29,6 +29,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- On Arm:
- Ability to enable stack protector
+ - GICv3.1 eSPI (Extended Shared Peripheral Interrupts) support for Xen
+ and guest domains.
### Removed
- On x86:
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread