Re: [PATCH v2 10/10] xen/arm: vgic-v3: add emulation of GICv3.1 eSPI registers

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hello Oleksandr,

Thank you for your review comments.

On 21.08.25 19:17, Oleksandr Tyshchenko wrote:
> 
> 
> On 07.08.25 15:33, 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
>> ---
>>   xen/arch/arm/vgic-v3.c | 248 ++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 245 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index 4369c55177..1cacbb6e43 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -111,7 +111,7 @@ 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 offset, uint64_t irouter, 
>> bool espi)
>>   {
>>       struct vcpu *new_vcpu, *old_vcpu;
>>       unsigned int virq;
>> @@ -123,7 +123,8 @@ static void vgic_store_irouter(struct domain *d, 
>> struct vgic_irq_rank *rank,
>>        * The IROUTER0-31, used for SGIs/PPIs, are reserved and should
>>        * never call this function.
>>        */
>> -    ASSERT(virq >= 32);
>> +    if ( !espi )
>> +        ASSERT(virq >= 32);
>>       /* Get the index in the rank */
>>       offset = virq & INTERRUPT_RANK_MASK;
>> @@ -146,6 +147,11 @@ static void vgic_store_irouter(struct domain *d, 
>> struct vgic_irq_rank *rank,
>>       /* Only migrate the IRQ if the target vCPU has changed */
>>       if ( new_vcpu != old_vcpu )
>>       {
>> +#ifdef CONFIG_GICV3_ESPI
>> +        /* Convert virq index to eSPI range */
>> +        if ( espi )
>> +            virq = ESPI_IDX2INTID(virq);
>> +#endif
>>           if ( vgic_migrate_irq(old_vcpu, new_vcpu, virq) )
>>               write_atomic(&rank->vcpu[offset], new_vcpu->vcpu_id);
>>       }
>> @@ -685,6 +691,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 +719,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 +769,61 @@ 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;
> 
> 
> NIT: If I am not mistaken, the goto should be on the next line (here and 
> in similar places throughout the added code).
>

I just replicated some existing code with formatting for regular SPIs in 
eSPI. According to CODING_STYLE, there is no explicit rule against 
putting such code on one line (or I missed this point). However, in 
examples for a single-statement block after an if:

 > Braces should be omitted for blocks with a single statement. e.g.,
 >
 > if ( condition )
 >    single_statement();
 >

The single statement is placed on a new line. So, I would prefer to 
address your nit and add newlines here and in similar places in V3.

>> +        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 +854,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 +946,87 @@ 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%d\n",
> 
> I would use ISACTIVER%dE in the printed message to distinguish between 
> normal and "extended" registers (here and in similar places throughout 
> the added code).
> 
>> +               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%d\n",
>> +               v, name, r, reg - GICD_ICACTIVER);
> 
> s/GICD_ICACTIVER/GICD_ICACTIVERnE
> 
> 
> [snip]
Oh, I missed this. I will fix it in V3. Thank you.

Best regards,
Leonid