Hello Oleksandr, Thank you for your explanation.
On 27.08.25 16:44, Oleksandr Tyshchenko wrote: > > > 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 <[email protected]> >>>> >>>> --- >>>> 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. > > This is really interesting.. I think it might be worth introducing one more eSPI-specific field for vgic, such as a bool has_epsi (or by checking if nr_espis is non-zero). In cases where the hardware does not support eSPIs (or eSPIs are not assigned to the guest domain), but the eSPI config is enabled, we could go to write_reserved for all eSPI-specific registers. This would definitely work the same way it does now. Additionally, we could return RAZ in the same case while reading eSPI-specific registers. This can be done like this (for mmio_write): case VRANGE32(GICD_ISENABLERnE, GICD_ISENABLERnEN): if (v->domain->arch.vgic.nr_espi == 0) goto write_ignore; if ( dabt.size != DABT_WORD ) goto bad_width; And for mmio_read: case VRANGE32(GICD_ISENABLERnE, GICD_ISENABLERnEN): if (v->domain->arch.vgic.nr_espi == 0) goto read_as_zero; if ( dabt.size != DABT_WORD ) goto bad_width; (and for other eSPI registers, including GICD_ISACTIVER<n>E) Also with has_epsi, it would be much easier: if (!v->domain->arch.vgic.has_epsi) .... What do you think about this? >> >>>> + >>>> + 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] Best regards, Leonid
