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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

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)
> +{
> +    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?

> +        return;
> +
> +    clear_bit(virq_to_idx(d, virq), d->arch.vgic.allocated_irqs);
>  }
>  
>  unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)

-- 
WBR, Volodymyr