Re: [PATCH v2 08/10] xen/arm: vgic: add resource management for extended SPIs

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx> writes:

> This change introduces resource management in the VGIC to handle
> extended SPIs introduced in GICv3.1. The pending_irqs and
> allocated_irqs arrays are resized to support the required
> number of eSPIs, based on what is supported by the hardware and
> requested by the guest. A new field, ext_shared_irqs, is added
> to the VGIC structure to store information about eSPIs, similar
> to how shared_irqs is used for regular SPIs.
>
> Since the eSPI range starts at INTID 4096 and INTIDs between 1025
> and 4095 are reserved, helper macros are introduced to simplify the
> transformation of indices and to enable easier access to eSPI-specific
> resources. These changes prepare the VGIC for processing eSPIs as
> required by future functionality.
>
> The initialization and deinitialization paths for vgic have been updated
> to allocate and free these resources appropriately. Additionally,
> updated handling of INTIDs greater than 1024, passed from the toolstack
> during domain creation, and verification logic ensures only valid SPI or
> eSPI INTIDs are used.
>
> The existing SPI behavior remains unaffected when guests do not request
> eSPIs, GIC hardware does not support them, or the CONFIG_GICV3_ESPI
> option is disabled.
>
> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@xxxxxxxx>
>
> ---
> Changes in V2:
> - change is_espi_rank to is_valid_espi_rank to verify whether the array
>   element ext_shared_irqs exists. The previous version, is_espi_rank,
>   only checked if the rank index was less than the maximum possible eSPI
>   rank index, but this could potentially result in accessing a
>   non-existing array element. To address this, is_valid_espi_rank was
>   introduced, which ensures that the required eSPI rank exists
> - move gic_number_espis to
>   xen/arm: gicv3: implement handling of GICv3.1 eSPI
> - update vgic_is_valid_irq checks to allow operating with eSPIs
> - remove redundant newline in vgic_allocate_virq
> ---
>  xen/arch/arm/include/asm/vgic.h |  18 ++++
>  xen/arch/arm/vgic.c             | 145 ++++++++++++++++++++++++++++++++
>  2 files changed, 163 insertions(+)
>
> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
> index 45201f4ca5..9fa4523018 100644
> --- a/xen/arch/arm/include/asm/vgic.h
> +++ b/xen/arch/arm/include/asm/vgic.h
> @@ -146,6 +146,10 @@ struct vgic_dist {
>      int nr_spis; /* Number of SPIs */
>      unsigned long *allocated_irqs; /* bitmap of IRQs allocated */
>      struct vgic_irq_rank *shared_irqs;
> +#ifdef CONFIG_GICV3_ESPI
> +    struct vgic_irq_rank *ext_shared_irqs;
> +    int nr_espis; /* Number of extended SPIs */
> +#endif
>      /*
>       * SPIs are domain global, SGIs and PPIs are per-VCPU and stored in
>       * struct arch_vcpu.
> @@ -243,6 +247,14 @@ struct vgic_ops {
>  /* Number of ranks of interrupt registers for a domain */
>  #define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_spis+31)/32)
>  
> +#ifdef CONFIG_GICV3_ESPI
> +#define DOMAIN_NR_EXT_RANKS(d) (((d)->arch.vgic.nr_espis+31)/32)
> +#define EXT_RANK_MIN (ESPI_BASE_INTID/32)
> +#define EXT_RANK_MAX ((ESPI_MAX_INTID+31)/32)
> +#define EXT_RANK_NUM2IDX(num) ((num)-EXT_RANK_MIN)
> +#define EXT_RANK_IDX2NUM(idx) ((idx)+EXT_RANK_MIN)
> +#endif
> +
>  #define vgic_lock(v)   spin_lock_irq(&(v)->domain->arch.vgic.lock)
>  #define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)
>  
> @@ -302,6 +314,12 @@ extern struct vgic_irq_rank *vgic_rank_offset(struct 
> vcpu *v,
>                                                unsigned int b,
>                                                unsigned int n,
>                                                unsigned int s);
> +#ifdef CONFIG_GICV3_ESPI
> +extern struct vgic_irq_rank *vgic_ext_rank_offset(struct vcpu *v,
> +                                                  unsigned int b,
> +                                                  unsigned int n,
> +                                                  unsigned int s);
> +#endif
>  extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq);
>  extern void vgic_disable_irqs(struct vcpu *v, uint32_t r, unsigned int n);
>  extern void vgic_enable_irqs(struct vcpu *v, uint32_t r, unsigned int n);
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 48fbaf56fb..1a6c765af9 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -27,9 +27,26 @@
>  
>  bool vgic_is_valid_irq(struct domain *d, unsigned int virq)
>  {
> +#ifdef CONFIG_GICV3_ESPI
> +    if ( virq >= ESPI_BASE_INTID && virq < 
> ESPI_IDX2INTID(d->arch.vgic.nr_espis) )
> +        return true;
> +#endif
> +
>      return virq < vgic_num_irqs(d);
>  }
>  
> +#ifdef CONFIG_GICV3_ESPI
> +/*
> + * Since eSPI indexes start from 4096 and numbers from 1024 to
> + * 4095 are forbidden, we need to check both lower and upper
> + * limits for ranks.
> + */
> +static inline bool is_valid_espi_rank(struct domain *d, unsigned int rank)
> +{
> +    return ( rank >= EXT_RANK_MIN && EXT_RANK_NUM2IDX(rank) < 
> DOMAIN_NR_EXT_RANKS(d) );

I am pretty sure that max line length in 80 symbols, and here you have
more. As well in couple of other places below.

> +}
> +#endif
> +
>  static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
>                                                    unsigned int rank)
>  {
> @@ -37,6 +54,10 @@ static inline struct vgic_irq_rank *vgic_get_rank(struct 
> vcpu *v,
>          return v->arch.vgic.private_irqs;
>      else if ( rank <= DOMAIN_NR_RANKS(v->domain) )
>          return &v->domain->arch.vgic.shared_irqs[rank - 1];
> +#ifdef CONFIG_GICV3_ESPI
> +    else if ( is_valid_espi_rank(v->domain, rank) )
> +        return &v->domain->arch.vgic.ext_shared_irqs[EXT_RANK_NUM2IDX(rank)];
> +#endif
>      else
>          return NULL;
>  }
> @@ -53,6 +74,16 @@ struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, 
> unsigned int b,
>      return vgic_get_rank(v, rank);
>  }
>  
> +#ifdef CONFIG_GICV3_ESPI
> +struct vgic_irq_rank *vgic_ext_rank_offset(struct vcpu *v, unsigned int b,
> +                                           unsigned int n, unsigned int s)
> +{
> +    unsigned int rank = REG_RANK_NR(b, (n >> s));
> +
> +    return vgic_get_rank(v, rank + EXT_RANK_MIN);
> +}
> +#endif
> +
>  struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
>  {
>      unsigned int rank = irq / 32;
> @@ -117,6 +148,29 @@ int domain_vgic_register(struct domain *d, unsigned int 
> *mmio_count)
>      return 0;
>  }
>  
> +#ifdef CONFIG_GICV3_ESPI
> +static int init_vgic_espi(struct domain *d)
> +{
> +    int i;
> +
> +    if ( d->arch.vgic.nr_espis == 0 )
> +        return 0;
> +
> +    d->arch.vgic.ext_shared_irqs =
> +        xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_EXT_RANKS(d));
> +    if ( d->arch.vgic.ext_shared_irqs == NULL )
> +        return -ENOMEM;
> +
> +    for ( i = 0; i < d->arch.vgic.nr_espis; i++ )
> +        vgic_init_pending_irq(&d->arch.vgic.pending_irqs[i + 
> d->arch.vgic.nr_spis], ESPI_IDX2INTID(i));
> +
> +    for ( i = 0; i < DOMAIN_NR_EXT_RANKS(d); i++ )
> +        vgic_rank_init(&d->arch.vgic.ext_shared_irqs[i], i, 0);
> +
> +    return 0;
> +}
> +#endif
> +
>  int domain_vgic_init(struct domain *d, unsigned int nr_spis)
>  {
>      int i;
> @@ -131,6 +185,30 @@ int domain_vgic_init(struct domain *d, unsigned int 
> nr_spis)
>       */
>      nr_spis = ROUNDUP(nr_spis, 32);
>  
> +#ifdef CONFIG_GICV3_ESPI
> +    if ( nr_spis > ESPI_MAX_INTID )
> +        return -EINVAL;
> +
> +    if ( is_espi(nr_spis) )
> +    {
> +        /*
> +         * During domain creation, the toolstack specifies the maximum INTID,
> +         * which is defined in the domain config subtracted by 32. To 
> compute the
> +         * actual number of eSPI that will be usable for, add back 32.

I think, according to this, your if ( is_espi() ) check is wrong. Should
you add 32 here as well?

> +         */
> +        d->arch.vgic.nr_espis = min(nr_spis - ESPI_BASE_INTID + 32, 1024U);
> +        /* Verify if GIC HW can handle provided INTID */
> +        if ( d->arch.vgic.nr_espis > gic_number_espis() )
> +            return -EINVAL;
> +        /* Set the maximum available number for defult SPI to pass the next 
> check */
> +        nr_spis = VGIC_DEF_NR_SPIS;
> +    } else
> +    {
> +        /* Domain will use the regular SPI range */
> +        d->arch.vgic.nr_espis = 0;
> +    }
> +#endif
> +
>      /* Limit the number of virtual SPIs supported to (1020 - 32) = 988  */
>      if ( nr_spis > (1020 - NR_LOCAL_IRQS) )
>          return -EINVAL;
> @@ -145,7 +223,12 @@ int domain_vgic_init(struct domain *d, unsigned int 
> nr_spis)
>          return -ENOMEM;
>  
>      d->arch.vgic.pending_irqs =
> +#ifdef CONFIG_GICV3_ESPI
> +        xzalloc_array(struct pending_irq, d->arch.vgic.nr_spis +
> +                      d->arch.vgic.nr_espis);
> +#else
>          xzalloc_array(struct pending_irq, d->arch.vgic.nr_spis);
> +#endif
>      if ( d->arch.vgic.pending_irqs == NULL )
>          return -ENOMEM;
>  
> @@ -156,12 +239,23 @@ int domain_vgic_init(struct domain *d, unsigned int 
> nr_spis)
>      for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
>          vgic_rank_init(&d->arch.vgic.shared_irqs[i], i + 1, 0);
>  
> +#ifdef CONFIG_GICV3_ESPI
> +    ret = init_vgic_espi(d);
> +    if ( ret )
> +        return ret;
> +#endif
> +
>      ret = d->arch.vgic.handler->domain_init(d);
>      if ( ret )
>          return ret;
>  
>      d->arch.vgic.allocated_irqs =
> +#ifdef CONFIG_GICV3_ESPI
> +        xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d) +
> +                      d->arch.vgic.nr_espis));
> +#else
>          xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d)));
> +#endif
>      if ( !d->arch.vgic.allocated_irqs )
>          return -ENOMEM;
>  
> @@ -195,9 +289,27 @@ void domain_vgic_free(struct domain *d)
>          }
>      }
>  
> +#ifdef CONFIG_GICV3_ESPI
> +    for ( i = 0; i < (d->arch.vgic.nr_espis); i++ )
> +    {
> +        struct pending_irq *p = spi_to_pending(d, ESPI_IDX2INTID(i));
> +
> +        if ( p->desc )
> +        {
> +            ret = release_guest_irq(d, p->irq);
> +            if ( ret )
> +                dprintk(XENLOG_G_WARNING, "d%u: Failed to release virq %u 
> ret = %d\n",
> +                        d->domain_id, p->irq, ret);
> +        }
> +    }
> +#endif
> +
>      if ( d->arch.vgic.handler )
>          d->arch.vgic.handler->domain_free(d);
>      xfree(d->arch.vgic.shared_irqs);
> +#ifdef CONFIG_GICV3_ESPI
> +    xfree(d->arch.vgic.ext_shared_irqs);
> +#endif
>      xfree(d->arch.vgic.pending_irqs);
>      xfree(d->arch.vgic.allocated_irqs);
>  }
> @@ -331,6 +443,17 @@ void arch_move_irqs(struct vcpu *v)
>          if ( v_target == v && !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) 
> )
>              irq_set_affinity(p->desc, cpu_mask);
>      }
> +
> +#ifdef CONFIG_GICV3_ESPI
> +    for ( i = ESPI_BASE_INTID; i < (d)->arch.vgic.nr_espis; i++ )
> +    {
> +        v_target = vgic_get_target_vcpu(v, i);
> +        p = irq_to_pending(v_target, i);
> +
> +        if ( v_target == v && !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) 
> )
> +            irq_set_affinity(p->desc, cpu_mask);
> +    }
> +#endif
>  }
>  
>  void vgic_disable_irqs(struct vcpu *v, uint32_t r, unsigned int n)
> @@ -538,6 +661,10 @@ struct pending_irq *irq_to_pending(struct vcpu *v, 
> unsigned int irq)
>          n = &v->arch.vgic.pending_irqs[irq];
>      else if ( is_lpi(irq) )
>          n = v->domain->arch.vgic.handler->lpi_to_pending(v->domain, irq);
> +#ifdef CONFIG_GICV3_ESPI
> +    else if ( is_espi(irq) )
> +        n = &v->domain->arch.vgic.pending_irqs[ESPI_INTID2IDX(irq) + 
> v->domain->arch.vgic.nr_spis];
> +#endif
>      else
>          n = &v->domain->arch.vgic.pending_irqs[irq - 32];
>      return n;
> @@ -547,6 +674,14 @@ struct pending_irq *spi_to_pending(struct domain *d, 
> unsigned int irq)
>  {
>      ASSERT(irq >= NR_LOCAL_IRQS);
>  
> +#ifdef CONFIG_GICV3_ESPI
> +    if ( is_espi(irq) )
> +    {
> +        irq = ESPI_INTID2IDX(irq) + d->arch.vgic.nr_spis;
> +        return &d->arch.vgic.pending_irqs[irq];
> +    }
> +#endif
> +
>      return &d->arch.vgic.pending_irqs[irq - 32];
>  }
>  
> @@ -668,6 +803,11 @@ bool vgic_reserve_virq(struct domain *d, unsigned int 
> virq)
>      if ( !vgic_is_valid_irq(d, virq) )
>          return false;
>  
> +#ifdef CONFIG_GICV3_ESPI
> +    if ( is_espi(virq) )
> +        return !test_and_set_bit(ESPI_INTID2IDX(virq) + vgic_num_irqs(d), 
> d->arch.vgic.allocated_irqs);
> +#endif
> +
>      return !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
>  }
>  
> @@ -686,6 +826,11 @@ int vgic_allocate_virq(struct domain *d, bool spi)
>      {
>          first = 32;
>          end = vgic_num_irqs(d);
> +#ifdef CONFIG_GICV3_ESPI
> +        /* Take into account extended SPI range */
> +        end += d->arch.vgic.nr_espis;
> +#endif
> +
>      }
>  
>      /*

-- 
WBR, Volodymyr