[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 10/11] xen/arm: vgic-v3: add emulation of GICv3.1 eSPI registers
On 27.08.25 14:13, Leonid Komarianskyi wrote:
Hello Oleksandr,
Hello Leonid
Thank you for your close review.
On 26.08.25 22:57, Oleksandr Tyshchenko wrote:
On 26.08.25 17:05, 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@xxxxxxxx>
---
Changes in V2:
- add missing rank index conversion for pending and inflight irqs
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
---
xen/arch/arm/vgic-v3.c | 275 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 266 insertions(+), 9 deletions(-)
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 4369c55177..56c539bb1b 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -111,13 +111,10 @@ static uint64_t vgic_fetch_irouter(struct
vgic_irq_rank *rank,
* 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
@@ -685,6 +682,9 @@ 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):
+#ifdef CONFIG_GICV3_ESPI
+ case VRANGE32(GICD_IGROUPRnE, GICD_IGROUPRnEN):
+#endif
/* We do not implement security extensions for guests, read
zero */
if ( dabt.size != DABT_WORD ) goto bad_width;
goto read_as_zero;
@@ -710,11 +710,19 @@ 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):
+#ifdef CONFIG_GICV3_ESPI
+ case VRANGE32(GICD_ISPENDRnE, GICD_ISPENDRnEN):
+ case VRANGE32(GICD_ICPENDRnE, GICD_ICPENDRnEN):
+#endif
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):
+#ifdef CONFIG_GICV3_ESPI
+ case VRANGE32(GICD_ISACTIVERnE, GICD_ISACTIVERnEN):
+ case VRANGE32(GICD_ICACTIVERnE, GICD_ICACTIVERnEN):
+#endif
goto read_as_zero;
case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
@@ -752,6 +760,69 @@ static int __vgic_v3_distr_common_mmio_read(const
char *name, struct vcpu *v,
return 1;
}
+#ifdef CONFIG_GICV3_ESPI
+ case VRANGE32(GICD_ISENABLERnE, GICD_ISENABLERnEN):
+ if ( dabt.size != DABT_WORD )
+ goto bad_width;
+ rank = vgic_ext_rank_offset(v, 1, reg - GICD_ISENABLERnE,
DABT_WORD);
+ if ( rank == NULL )
+ goto read_as_zero;
+ vgic_lock_rank(v, rank, flags);
+ *r = vreg_reg32_extract(rank->ienable, info);
+ vgic_unlock_rank(v, rank, flags);
+ return 1;
+
+ case VRANGE32(GICD_ICENABLERnE, GICD_ICENABLERnEN):
+ if ( dabt.size != DABT_WORD )
+ goto bad_width;
+ rank = vgic_ext_rank_offset(v, 1, reg - GICD_ICENABLERnE,
DABT_WORD);
+ if ( rank == NULL )
+ goto read_as_zero;
+ vgic_lock_rank(v, rank, flags);
+ *r = vreg_reg32_extract(rank->ienable, info);
+ vgic_unlock_rank(v, rank, flags);
+ return 1;
+
+ case VRANGE32(GICD_IPRIORITYRnE, GICD_IPRIORITYRnEN):
+ {
+ uint32_t ipriorityr;
+ uint8_t rank_index;
+
+ if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD )
+ goto bad_width;
+ rank = vgic_ext_rank_offset(v, 8, reg - GICD_IPRIORITYRnE,
DABT_WORD);
+ if ( rank == NULL )
+ goto read_as_zero;
+ rank_index = REG_RANK_INDEX(8, reg - GICD_IPRIORITYRnE,
DABT_WORD);
+
+ vgic_lock_rank(v, rank, flags);
+ ipriorityr = ACCESS_ONCE(rank->ipriorityr[rank_index]);
+ vgic_unlock_rank(v, rank, flags);
+
+ *r = vreg_reg32_extract(ipriorityr, info);
+
+ return 1;
+ }
+
+ case VRANGE32(GICD_ICFGRnE, GICD_ICFGRnEN):
+ {
+ uint32_t icfgr;
+
+ if ( dabt.size != DABT_WORD )
+ goto bad_width;
+ rank = vgic_ext_rank_offset(v, 2, reg - GICD_ICFGRnE,
DABT_WORD);
+ if ( rank == NULL )
+ goto read_as_zero;
+ vgic_lock_rank(v, rank, flags);
+ icfgr = rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGRnE,
DABT_WORD)];
+ vgic_unlock_rank(v, rank, flags);
+
+ *r = vreg_reg32_extract(icfgr, info);
+
+ return 1;
+ }
+#endif
+
default:
printk(XENLOG_G_ERR
"%pv: %s: unhandled read r%d offset %#08x\n",
@@ -782,6 +853,9 @@ 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):
+#ifdef CONFIG_GICV3_ESPI
+ case VRANGE32(GICD_IGROUPRnE, GICD_IGROUPRnEN):
+#endif
/* We do not implement security extensions for guests, write
ignore */
goto write_ignore_32;
@@ -871,6 +945,99 @@ static int
__vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
vgic_unlock_rank(v, rank, flags);
return 1;
+#ifdef CONFIG_GICV3_ESPI
+ case VRANGE32(GICD_ISENABLERnE, GICD_ISENABLERnEN):
+ if ( dabt.size != DABT_WORD )
+ goto bad_width;
+ rank = vgic_ext_rank_offset(v, 1, reg - GICD_ISENABLERnE,
DABT_WORD);
+ if ( rank == NULL )
+ goto write_ignore;
+ vgic_lock_rank(v, rank, flags);
+ tr = rank->ienable;
+ vreg_reg32_setbits(&rank->ienable, r, info);
+ vgic_enable_irqs(v, (rank->ienable) & (~tr),
EXT_RANK_IDX2NUM(rank->index));
+ vgic_unlock_rank(v, rank, flags);
+ return 1;
+
+ case VRANGE32(GICD_ICENABLERnE, GICD_ICENABLERnEN):
+ if ( dabt.size != DABT_WORD )
+ goto bad_width;
+ rank = vgic_ext_rank_offset(v, 1, reg - GICD_ICENABLERnE,
DABT_WORD);
+ if ( rank == NULL )
+ goto write_ignore;
+ vgic_lock_rank(v, rank, flags);
+ tr = rank->ienable;
+ vreg_reg32_clearbits(&rank->ienable, r, info);
+ vgic_disable_irqs(v, (~rank->ienable) & tr,
EXT_RANK_IDX2NUM(rank->index));
+ vgic_unlock_rank(v, rank, flags);
+ return 1;
+
+ case VRANGE32(GICD_ISPENDRnE, GICD_ISPENDRnEN):
+ if ( dabt.size != DABT_WORD )
+ goto bad_width;
+ rank = vgic_ext_rank_offset(v, 1, reg - GICD_ISPENDRnE,
DABT_WORD);
+ if ( rank == NULL )
+ goto write_ignore;
+
+ vgic_set_irqs_pending(v, r, EXT_RANK_IDX2NUM(rank->index));
+
+ return 1;
+
+ case VRANGE32(GICD_ICPENDRnE, GICD_ICPENDRnEN):
+ if ( dabt.size != DABT_WORD )
+ goto bad_width;
+ rank = vgic_ext_rank_offset(v, 1, reg - GICD_ICPENDRnE,
DABT_WORD);
+ if ( rank == NULL )
+ goto write_ignore;
+
+ vgic_check_inflight_irqs_pending(v, EXT_RANK_IDX2NUM(rank-
index), r);
+
+ goto write_ignore;
+ 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%dE\n",
+ v, name, r, reg - GICD_ISACTIVERnE);
+ return 0;
Guest write access to GICD_ISACTIVER<n>E will lead to abort. But, I know
you just repeated the logic for regular GICD_ISACTIVER<n>.
Could you please clarify the scenario in which it leads to an abort?
Since it is actually a stub case, it should just print an error message
and that's it..
"return 0;" will lead to injecting a fault into the guest.
Do you mean that, in this case, we need to goto
write_ignore_32 label instead of returning 0?
My comment was not a direct request to change anything, but rather
thinking out loud.
Unfortunally, I cannot answer why regular GICD_ISACTIVER<n> is emulated
in that way, but perhaps the injecting fault into the guest is the
lesser evil than emulating it incorrectly...
Interestingly, for GICv2 we have a slighly relaxed version; it looks
like writing 0 will not cause an abort and will be WI.
25f9e80201f3688e0c4d5c4e43e4b6143b441c52
xen/arm: Ignore write to GICD_ISACTIVERn registers (vgic-v2)
Now, with the introduction of extended GICD_ISACTIVER<n>E you retained
the logic. One thing that needs mentioning is that before your series,
guest write access to extended GICD_ISACTIVER<n>E would be WI, but
with your series and CONFIG_GICV3_ESPI=y it will be an abort even
if running on GIC3 HW where eSPI is not supported.
At least, I would mention that in the patch description.
+
+ case VRANGE32(GICD_ICACTIVERnE, GICD_ICACTIVERnEN):
+ printk(XENLOG_G_ERR
+ "%pv: %s: unhandled word write %#"PRIregister" to
ICACTIVER%dE\n",
+ v, name, r, reg - GICD_ICACTIVER);
s/GICD_ICACTIVER/GICD_ICACTIVERnE
I will fix that in V4.
+ goto write_ignore_32;
[snip]
|
