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
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]
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
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
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.
>
>
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
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
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
© 2016 - 2026 Red Hat, Inc.