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_komarians...@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_tyshche...@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]
