Hello Oleksandr,
Thank you for your review and your good qestion.
On 01.09.25 15:38, Oleksandr Tyshchenko wrote:
>
>
> On 29.08.25 19:06, 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 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
>
>
> Looks very good now, thanks. Just one NIT and one question below ...
>
>>
>> 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 | 23 ++++
>> 3 files changed, 192 insertions(+), 33 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/
>> asm/vgic.h
>> index 3aa22114ba..103bc3c74b 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..b5d766c98f 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,13 +682,20 @@ 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);
>> + if ( reg >= GICD_ISENABLERnE )
>> + rank = vgic_ext_rank_offset(v, 1, reg - GICD_ISENABLERnE,
>> + DABT_WORD);
>> + else
>> + rank = vgic_rank_offset(v, 1, reg - GICD_ISENABLER,
>> DABT_WORD);
>> if ( rank == NULL ) goto read_as_zero;
>> vgic_lock_rank(v, rank, flags);
>> *r = vreg_reg32_extract(rank->ienable, info);
>> @@ -699,8 +703,13 @@ 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);
>> + if ( reg >= GICD_ICENABLERnE )
>> + rank = vgic_ext_rank_offset(v, 1, reg - GICD_ICENABLERnE,
>> + DABT_WORD);
>> + else
>> + rank = vgic_rank_offset(v, 1, reg - GICD_ICENABLER,
>> DABT_WORD);
>> if ( rank == NULL ) goto read_as_zero;
>> vgic_lock_rank(v, rank, flags);
>> *r = vreg_reg32_extract(rank->ienable, info);
>> @@ -710,20 +719,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;
>> 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);
>> + if ( reg >= GICD_IPRIORITYRnE )
>> + rank = vgic_ext_rank_offset(v, 8, reg - GICD_IPRIORITYRnE,
>> + DABT_WORD);
>> + else
>> + rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR,
>> DABT_WORD);
>> if ( rank == NULL ) goto read_as_zero;
>> rank_index = REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
>> DABT_WORD);
>> @@ -737,11 +755,15 @@ 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;
>> if ( dabt.size != DABT_WORD ) goto bad_width;
>> - rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
>> + if ( reg >= GICD_ICFGRnE )
>> + rank = vgic_ext_rank_offset(v, 2, reg - GICD_ICFGRnE,
>> DABT_WORD);
>> + else
>> + rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
>> if ( rank == NULL ) goto read_as_zero;
>> vgic_lock_rank(v, rank, flags);
>> icfgr = rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR,
>> DABT_WORD)];
>> @@ -782,46 +804,81 @@ 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);
>> + if ( reg >= GICD_ISENABLERnE )
>> + rank = vgic_ext_rank_offset(v, 1, reg - GICD_ISENABLERnE,
>> + DABT_WORD);
>> + else
>> + rank = vgic_rank_offset(v, 1, reg - GICD_ISENABLER,
>> 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), rank->index);
>> + if ( reg >= GICD_ISENABLERnE )
>> + vgic_enable_irqs(v, (rank->ienable) & (~tr),
>> + EXT_RANK_IDX2NUM(rank->index));
>> + else
>> + vgic_enable_irqs(v, (rank->ienable) & (~tr), rank->index);
>> vgic_unlock_rank(v, rank, flags);
>> 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);
>> + if ( reg >= GICD_ICENABLERnE )
>> + rank = vgic_ext_rank_offset(v, 1, reg - GICD_ICENABLERnE,
>> + DABT_WORD);
>> + else
>> + rank = vgic_rank_offset(v, 1, reg - GICD_ICENABLER,
>> 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, rank->index);
>> + if ( reg >= GICD_ICENABLERnE )
>> + vgic_disable_irqs(v, (~rank->ienable) & tr,
>> + EXT_RANK_IDX2NUM(rank->index));
>> + else
>> + vgic_disable_irqs(v, (~rank->ienable) & tr, rank->index);
>> vgic_unlock_rank(v, rank, flags);
>> 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);
>> + if ( reg >= GICD_ISPENDRnE )
>> + rank = vgic_ext_rank_offset(v, 1, reg - GICD_ISPENDRnE,
>> DABT_WORD);
>> + else
>> + rank = vgic_rank_offset(v, 1, reg - GICD_ISPENDR,
>> DABT_WORD);
>> if ( rank == NULL ) goto write_ignore;
>> - vgic_set_irqs_pending(v, r, rank->index);
>> + if ( reg >= GICD_ISPENDRnE )
>> + vgic_set_irqs_pending(v, r, EXT_RANK_IDX2NUM(rank->index));
>> + else
>> + vgic_set_irqs_pending(v, r, rank->index);
>> 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);
>> + if ( reg >= GICD_ICPENDRnE )
>> + rank = vgic_ext_rank_offset(v, 1, reg - GICD_ICPENDRnE,
>> DABT_WORD);
>> + else
>> + rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR,
>> DABT_WORD);
>> if ( rank == NULL ) goto write_ignore;
>> - vgic_check_inflight_irqs_pending(v, rank->index, r);
>> + if ( reg >= GICD_ICPENDRnE )
>> + vgic_check_inflight_irqs_pending(v,
>> + EXT_RANK_IDX2NUM(rank-
>> >index), r);
>> + else
>> + vgic_check_inflight_irqs_pending(v, rank->index, r);
>> goto write_ignore;
>> @@ -838,16 +895,38 @@ static int
>> __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
>> v, name, r, reg - GICD_ICACTIVER);
>> goto write_ignore_32;
>> + 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;
>> +
>> + 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_ICACTIVERnE);
>> + goto write_ignore_32;
>
>
> NIT: I would group with regular SPI ranges (taking into account that all
> other ranges were already grouped including the read accesses),
> something like that (non tested):
>
> 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%dE\n",
> + v, name, r, reg - GICD_ISACTIVERnE);
> + else
> + printk(XENLOG_G_ERR
> + "%pv: %s: unhandled word write %#"PRIregister" to
> ISACTIVER%d\n",
> + v, name, r, reg - GICD_ISACTIVER);
> 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%dE\n",
> + v, name, r, reg - GICD_ICACTIVERnE);
> + else
> + printk(XENLOG_G_ERR
> + "%pv: %s: unhandled word write %#"PRIregister" to
> ICACTIVER%d\n",
> + v, name, r, reg - GICD_ICACTIVER);
> goto write_ignore_32;
>
>
I agree with that. With such changes, it will be much easier to modify
the code for SPI and eSPI counterparts if needed. I will regroup them in V6.
>> +
>> 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);
>> + if ( reg >= GICD_IPRIORITYRnE ) {
>> + offset = reg - GICD_IPRIORITYRnE;
>> + rank = vgic_ext_rank_offset(v, 8, offset, DABT_WORD);
>> + }
>> + else
>> + {
>> + offset = reg - GICD_IPRIORITYR;
>> + rank = vgic_rank_offset(v, 8, offset, DABT_WORD);
>> + }
>> if ( rank == NULL ) goto write_ignore;
>> 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)];
>
> Here
>
>
>> priority = ACCESS_ONCE(*ipriorityr);
>> vreg_reg32_update(&priority, r, info);
>> ACCESS_ONCE(*ipriorityr) = priority;
>> @@ -859,10 +938,14 @@ 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):
>> /* 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);
>> + if ( reg >= GICD_ICFGRnE )
>> + rank = vgic_ext_rank_offset(v, 2, reg - GICD_ICFGRnE,
>> DABT_WORD);
>> + else
>> + rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, 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_ICFGR,
>> @@ -1129,6 +1212,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 +1287,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 +1304,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,16 +1320,21 @@ 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;
>> if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
>> - rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER,
>> - DABT_DOUBLE_WORD);
>> + if ( gicd_reg >= GICD_IROUTERnE )
>> + rank = vgic_ext_rank_offset(v, 64, gicd_reg -
>> GICD_IROUTERnE,
>> + DABT_DOUBLE_WORD);
>> + else
>> + rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER,
>> + DABT_DOUBLE_WORD);
>> if ( rank == NULL ) goto read_as_zero;
>> vgic_lock_rank(v, rank, flags);
>> irouter = vgic_fetch_irouter(rank, gicd_reg - GICD_IROUTER);
>
> Here you use the same offset for regular and extended SPI ranges
> (gicd_reg - GICD_IROUTER) ...
>
>
Oh, this is an error, and it only works because GICD_IROUTER has an
offset of 0x6000, GICD_IROUTERnE has 0x8000, and vgic_fetch_irouter
applies the mask INTERRUPT_RANK_MASK (0x1F):
/* There is exactly 1 vIRQ per IROUTER */
offset /= NR_BYTES_PER_IROUTER;
/* Get the index in the rank */
offset &= INTERRUPT_RANK_MASK;
After applying the mask, the 'additional' difference of 0x2000/8 was
removed..
The offset should be recalculated according to the register being
processed, as in the code below.
I will fix this and review other places with similar code.
>> @@ -1235,8 +1344,8 @@ static int vgic_v3_distr_mmio_read(struct vcpu
>> *v, mmio_info_t *info,
>> return 1;
>> }
>> -
>> - case VRANGE32(0x7FE0, 0xBFFC):
>> + case VRANGE32(0x3700, 0x60FC):
>> + case VRANGE32(0xA004, 0xBFFC):
>> goto read_reserved;
>> case VRANGE32(0xC000, 0xFFCC):
>> @@ -1382,12 +1491,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 +1525,38 @@ 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);
>> + if ( gicd_reg >= GICD_IROUTERnE ) {
>> + offset = gicd_reg - GICD_IROUTERnE;
>> + rank = vgic_ext_rank_offset(v, 64, offset,
>> DABT_DOUBLE_WORD);
>> + } else {
>> + 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);
>
> ... But here you use different offsets for regular and extended SPI
> ranges (gicd_reg - GICD_IROUTER vs gicd_reg - GICD_IROUTERnE). Could you
> please clarify why (what did I miss)?
>
>
>> vreg_reg64_update(&irouter, r, info);
>> - vgic_store_irouter(v->domain, rank, gicd_reg - GICD_IROUTER,
>> irouter);
>> + if ( gicd_reg >= GICD_IROUTERnE )
>> + virq = ESPI_IDX2INTID(offset / NR_BYTES_PER_IROUTER);
>> + else
>> + virq = 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 c9b9528c66..27ffdf316c 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -193,6 +193,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;
>> @@ -241,6 +253,17 @@ struct pending_irq *espi_to_pending(struct domain
>> *d, unsigned int irq)
>> {
>> return NULL;
>> }
>> +
>> +/*
>> + * 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)
>
Best regards,
Leonid
