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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

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.

> +
> +    clear_bit(virq_to_idx(d, virq), d->arch.vgic.allocated_irqs);
>  }
>  
>  unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> index ba029b8a3b..84212bbefe 100644
> --- a/xen/arch/arm/vgic/vgic.c
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -712,7 +712,7 @@ bool vgic_evtchn_irq_pending(struct vcpu *v)
>  
>  bool vgic_reserve_virq(struct domain *d, unsigned int virq)
>  {
> -    if ( virq >= vgic_num_irqs(d) )
> +    if ( !vgic_is_valid_line(d, virq) )
>          return false;
>  
>      return !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
> @@ -756,6 +756,9 @@ int vgic_allocate_virq(struct domain *d, bool spi)
>  
>  void vgic_free_virq(struct domain *d, unsigned int virq)
>  {
> +    if ( !vgic_is_valid_line(d, virq) )
> +        return;
> +
>      clear_bit(virq, d->arch.vgic.allocated_irqs);
>  }

-- 
WBR, Volodymyr