Hello Oleksandr,
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_komarians...@epam.com>
>>
>> ---
>> 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.. Do you mean that, in this case, we need to goto
write_ignore_32 label instead of returning 0?
>> +
>> + 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;
>> +
>> + case VRANGE32(GICD_IPRIORITYRnE, GICD_IPRIORITYRnEN):
>> + {
>> + uint32_t *ipriorityr, priority;
>> +
>> + 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 write_ignore;
>> + vgic_lock_rank(v, rank, flags);
>> + ipriorityr = &rank->ipriorityr[REG_RANK_INDEX(8, reg -
>> GICD_IPRIORITYRnE,
>> + DABT_WORD)];
>> + priority = ACCESS_ONCE(*ipriorityr);
>> + vreg_reg32_update(&priority, r, info);
>> + ACCESS_ONCE(*ipriorityr) = priority;
>> + vgic_unlock_rank(v, rank, flags);
>> + return 1;
>> + }
>
> NIT: emply line please (and in similar places)
>
I will recheck formatting and fix it in V4.
>> + case VRANGE32(GICD_ICFGRnE, GICD_ICFGRnEN):
>> + if ( dabt.size != DABT_WORD )
>> + goto bad_width;
>> + rank = vgic_ext_rank_offset(v, 2, reg - GICD_ICFGRnE,
>> DABT_WORD);
>> + if ( rank == NULL )
>> + goto write_ignore;
>> + vgic_lock_rank(v, rank, flags);
>> + vreg_reg32_update(&rank->icfg[REG_RANK_INDEX(2, reg -
>> GICD_ICFGRnE,
>> + DABT_WORD)],
>> + r, info);
>> + vgic_unlock_rank(v, rank, flags);
>> + return 1;
>> +#endif
>> +
>> default:
>> printk(XENLOG_G_ERR
>> "%pv: %s: unhandled write r%d=%"PRIregister" offset
>> %#08x\n",
>> @@ -1129,6 +1296,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 +1371,18 @@ 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):
>> +
>> +#ifdef CONFIG_GICV3_ESPI
>> + 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):
>> +#endif
>
> GICD_IGRPMODR<n>E is missed? I guess, it should be RAZ as regular
> GICD_IGRPMODR<n>.
>
> Also GICD_NSACR<n>E is missed, although the case for regular
> GICD_NSACR<n> is present (not visible in patch context).
>
Yes, I missed them, since they are not required for real GIC hardware..
I will update the patch that adds eSPI-specific defines and make the
appropriate changes in this patch.
>> /*
>> * Above all register are common with GICR and GICD
>> * Manage in common
>> @@ -1216,7 +1405,11 @@ 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;
>> +#ifdef CONFIG_GICV3_ESPI
>> + case VRANGE32(0x3100, 0x60FC):
>> +#else
>> case VRANGE32(0x0F30, 0x60FC):
>> +#endif
>> goto read_reserved;
>> case VRANGE64(GICD_IROUTER32, GICD_IROUTER1019):
>> @@ -1235,8 +1428,30 @@ static int vgic_v3_distr_mmio_read(struct vcpu
>> *v, mmio_info_t *info,
>> return 1;
>> }
>> +#ifdef CONFIG_GICV3_ESPI
>> + case VRANGE64(GICD_IROUTERnE, GICD_IROUTERnEN):
>> + {
>> + uint64_t irouter;
>> +
>> + if ( !vgic_reg64_check_access(dabt) )
>> + goto bad_width;
>> + rank = vgic_ext_rank_offset(v, 64, gicd_reg - GICD_IROUTERnE,
>> + DABT_DOUBLE_WORD);
>> + if ( rank == NULL )
>> + goto read_as_zero;
>> + vgic_lock_rank(v, rank, flags);
>> + irouter = vgic_fetch_irouter(rank, gicd_reg - GICD_IROUTERnE);
>> + vgic_unlock_rank(v, rank, flags);
>> + *r = vreg_reg64_extract(irouter, info);
>> +
>> + return 1;
>> + }
>> +
>> + case VRANGE32(0xA004, 0xBFFC):
>> +#else
>> case VRANGE32(0x7FE0, 0xBFFC):
>> +#endif
>> goto read_reserved;
>> case VRANGE32(0xC000, 0xFFCC):
>> @@ -1382,6 +1597,18 @@ 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):
>> +
>> +#ifdef CONFIG_GICV3_ESPI
>> + 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):
>> +#endif
>
> GICD_IGRPMODR<n>E is missed? I guess, it should be WI as regular
> GICD_IGRPMODR<n>.
>
>
> Also GICD_NSACR<n>E is missed, although the case for regular
> GICD_NSACR<n> is present (not visible in patch context).
I will update the patch that adds eSPI-specific defines and make the
appropriate changes in this patch.
>
>> /* Above registers are common with GICR and GICD
>> * Manage in common */
>> return __vgic_v3_distr_common_mmio_write("vGICD", v, info,
>> @@ -1405,26 +1632,56 @@ static int vgic_v3_distr_mmio_write(struct
>> vcpu *v, mmio_info_t *info,
>> if ( dabt.size != DABT_WORD ) goto bad_width;
>> return 0;
>> +#ifdef CONFIG_GICV3_ESPI
>> + case VRANGE32(0x3100, 0x60FC):
>> +#else
>> case VRANGE32(0x0F30, 0x60FC):
>> +#endif
>
> I wonder, can we have #defines for these magics (at least for the start
> of the reserved range)?
>
>> goto write_reserved;
>> case VRANGE64(GICD_IROUTER32, GICD_IROUTER1019):
>> {
>> 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);
>> + offset = gicd_reg - GICD_IROUTER;
>> + rank = vgic_rank_offset(v, 64, offset, DABT_DOUBLE_WORD);
>> if ( rank == NULL ) goto write_ignore;
>> 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);
>> + virq = offset / NR_BYTES_PER_IROUTER;
>> + vgic_store_irouter(v->domain, rank, virq, irouter);
>> + vgic_unlock_rank(v, rank, flags);
>> + return 1;
>> + }
>> +
>> +#ifdef CONFIG_GICV3_ESPI
>> + case VRANGE64(GICD_IROUTERnE, GICD_IROUTERnEN):
>> + {
>> + uint64_t irouter;
>> + unsigned int offset, virq;
>> +
>> + if ( !vgic_reg64_check_access(dabt) )
>> + goto bad_width;
>> + offset = gicd_reg - GICD_IROUTERnE;
>> + rank = vgic_ext_rank_offset(v, 64, offset, DABT_DOUBLE_WORD);
>> + if ( rank == NULL )
>> + goto write_ignore;
>> + vgic_lock_rank(v, rank, flags);
>> + irouter = vgic_fetch_irouter(rank, offset);
>> vreg_reg64_update(&irouter, r, info);
>> - vgic_store_irouter(v->domain, rank, gicd_reg - GICD_IROUTER,
>> irouter);
>> + virq = ESPI_IDX2INTID(offset / NR_BYTES_PER_IROUTER);
>> + vgic_store_irouter(v->domain, rank, virq, irouter);
>> vgic_unlock_rank(v, rank, flags);
>> return 1;
>> }
>> + case VRANGE32(0xA004, 0xBFFC):
>> +#else
>> case VRANGE32(0x7FE0, 0xBFFC):
>> +#endif
>> goto write_reserved;
>> case VRANGE32(0xC000, 0xFFCC):
>
Best regards,
Leonid.
