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

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

On 25-Aug-26 02:35, Volodymyr Babchuk wrote:
> Hi,
> 
> 
> I have only one small question to this patch. Please see below.
> 
> 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. Validate a vIRQ before clearing its allocation bit.
>>
>> Fixes: bdde400c6e1b ("xen/arm: vgic: add resource management for extended 
>> SPIs")
>> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
>> ---
>> 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 | 27 ++++++++++++++++-----------
>>  1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index e14123a30a..e541348a5c 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -33,6 +33,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)
Please add a comment at the top of the function about the layout these two
helpers encode to prevent such problems in the future.

>> +{
>> +    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
>> @@ -849,19 +859,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)
>> @@ -898,7 +900,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) )
> 
> Is this really can happen during normal runtime?
Yes, it can. A dom0less domU with direct-map, vpl011 and an explicit
nr_spis.

With the comment added:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal