Re: [PATCH v5 4/4] xen/arm: vgic: free eSPIs using the bitmap index

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 25-Sep-26 00:55, Volodymyr Babchuk wrote:
> Hi Mykola,
> 
> Mykola Kvach <mykola_kvach@xxxxxxxx> writes:
> 
>> The allocated_irqs bitmap in the existing vGIC implementation stores eSPI
>> allocation bits immediately after the regular vIRQ bits.
>> vgic_reserve_virq() converts an eSPI INTID to this compressed bitmap index,
>> but vgic_free_virq() used the raw INTID.
>>
>> Freeing INTID 4096 therefore clears bit 4096 instead of the first eSPI bit.
>> This writes beyond allocated_irqs and leaves the intended eSPI bit set.
>> Valid eSPIs reach this path during DOMCTL bind failure cleanup and unbind,
>> and during vPL011 teardown.
>>
>> Add virq_to_idx(), the inverse of idx_to_virq(), and use it when reserving
>> and freeing vIRQs. Use vgic_is_valid_line() for the validity checks when
>> reserving and freeing vIRQs in both vGIC implementations.
>>
>> Fixes: bdde400c6e1b ("xen/arm: vgic: add resource management for extended 
>> SPIs")
>> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
>> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>> Changes in v5:
>> - Add the vgic_is_valid_line() guard to vgic_free_virq() in the new
>>   vGIC implementation and use the same helper in vgic_reserve_virq().
>>
>> Changes in v4:
>> - Document the compressed bitmap layout with an ASCII diagram above the
>>   conversion helpers and refer to struct vgic_dist for the full layout.
>>
>> Changes in v3:
>> - Adapt virq_to_idx() to the configuration-neutral is_espi() helper.
>>
>> Changes in v2:
>> - Call is_espi() without a configuration guard.
>> ---
>>  xen/arch/arm/vgic.c      | 42 +++++++++++++++++++++++++++++-----------
>>  xen/arch/arm/vgic/vgic.c |  5 ++++-
>>  2 files changed, 35 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 0ba13e18da..acded6a40f 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -25,6 +25,21 @@
>>  #include <asm/vgic.h>
>>  
>>  
>> +/*
>> + * The allocated_irqs bitmap is compressed: eSPI bits immediately follow
>> + * regular IRQ bits, skipping the gap in the INTID space.
>> + *
>> + *   +-----------+-----------+-------------------+-------------------+
>> + *   |   SGIs    |   PPIs    |       SPIs        |       eSPIs       |
>> + *   +-----------+-----------+-------------------+-------------------+
>> + *   0           16          32                  vgic_num_irqs(d)
>> + *
>> + * INTID ESPI_BASE_INTID maps to bitmap index vgic_num_irqs(d).
>> + * The following idx_to_virq() and virq_to_idx() convert between INTIDs
>> + * and bitmap indexes.
>> + *
>> + * See also the allocated_irqs comment in struct vgic_dist.
>> + */
>>  static inline unsigned int idx_to_virq(struct domain *d, unsigned int idx)
>>  {
>>      if ( idx >= vgic_num_irqs(d) )
>> @@ -33,6 +48,16 @@ static inline unsigned int idx_to_virq(struct domain *d, 
>> unsigned int idx)
>>      return idx;
>>  }
>>  
>> +static inline unsigned int virq_to_idx(struct domain *d, unsigned int virq)
>> +{
>> +    ASSERT(IS_ENABLED(CONFIG_GICV3_ESPI) || !is_espi(virq));
>> +
>> +    if ( IS_ENABLED(CONFIG_GICV3_ESPI) && is_espi(virq) )
>> +        return espi_intid_to_idx(virq) + vgic_num_irqs(d);
>> +
>> +    return virq;
>> +}
>> +
>>  bool vgic_is_valid_line(struct domain *d, unsigned int virq)
>>  {
>>  #ifdef CONFIG_GICV3_ESPI
>> @@ -854,19 +879,11 @@ bool vgic_emulate(struct cpu_user_regs *regs, union 
>> hsr hsr)
>>  
>>  bool vgic_reserve_virq(struct domain *d, unsigned int virq)
>>  {
>> -    unsigned int idx = virq;
>> -
>>      if ( !vgic_is_valid_line(d, virq) )
>>          return false;
>>  
>> -    if ( is_espi(virq) )
>> -    {
>> -        unsigned int num_regular_irqs = vgic_num_irqs(d);
>> -
>> -        idx = espi_intid_to_idx(virq) + num_regular_irqs;
>> -    }
>> -
>> -    return !test_and_set_bit(idx, d->arch.vgic.allocated_irqs);
>> +    return !test_and_set_bit(virq_to_idx(d, virq),
>> +                             d->arch.vgic.allocated_irqs);
>>  }
>>  
>>  int vgic_allocate_virq(struct domain *d, bool spi)
>> @@ -903,7 +920,10 @@ int vgic_allocate_virq(struct domain *d, bool spi)
>>  
>>  void vgic_free_virq(struct domain *d, unsigned int virq)
>>  {
>> -    clear_bit(virq, d->arch.vgic.allocated_irqs);
>> +    if ( !vgic_is_valid_line(d, virq) )
>> +        return;
> 
> I don't think that silently failing is a good idea. It needs an ASSERT()
> at least.
We cannot add ASSERT() here. Take a look at vpl011 init failure path on
vgic_reserve_virq() that is taken to domain_vpl011_deinit(). If you specify
nr_spis not enough to cover for UART IRQ, this can happen. I think I answered
this question some time ago.

~Michal