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

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hi Volodymyr,

Thank you for your close review.

On 21.08.25 20:26, Volodymyr Babchuk wrote:
> 
> Hi Leonid,
> 
> Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx> writes:
> 
>> 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)
> 
> I am wondering: maybe it is better to pass virq instead of offset into
> this function? In that case you can get rid of espi parameter.
> 
Yes, it makes sense, because in any case we recalculate the offset 
later, after getting the virq:
 >    /* Get the index in the rank */
 >    offset = virq & INTERRUPT_RANK_MASK;

And as a bonus, I can get rid of the espi parameter. However, it 
requires the caller to calculate the virq by itself.
If applicable, I can add one more preparation patch in V3 before 
actually implementing eSPI with such changes. Would that be better?

>>   {
>>       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);
> 
> better to write
> ASSERT (!espi & (virq>=32))
> 
I agree with that, it looks much better. I will change it in V3 or drop 
at all if it would be okay to use virq as a parameter.

> and probably you need symmetrical ASSERT for espi == true
This is not needed for eSPI because, unlike regular SPIs that have 
indexes starting from 32, eSPI INTIDs start from 4096 and cover all 1024 
IRQ lines.

> 
>>       /* 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 you define ESPI_IDX2INTID() uncoditionally, you can get rid of #ifdef
> CONFIG_GICV3_ESPI here
>
Actually, ESPI_IDX2INTID is defined under ifdef condition:

 > #ifdef CONFIG_GICV3_ESPI
 > #define ESPI_BASE_INTID 4096
 > #define ESPI_MAX_INTID  5119

 > #define ESPI_INTID2IDX(intid) ((intid) - ESPI_BASE_INTID)
 > #define ESPI_IDX2INTID(idx)   ((idx) + ESPI_BASE_INTID)
 > #endif

So it should be used under CONFIG_GICV3_ESPI, like in other places.

>>           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
> 
> Do you really need ifdef here?
> 

Oh, yes, this ifdef is redundant because, without CONFIG_GICV3_ESPI 
enabled, we cannot reach this function with eSPI-specific offsets from 
the caller.
So, yes, I will remove this ifdef in V3.
>> +    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
> 
> Same as here

I will remove it in V3.
> 
>> +    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
> 
> ... and here and in all other places
> 

Thank you, that's a really good point. I will revise all the places with 
ifdefs in this patch and fix them in V3.

Best regards,
Leonid