Hello Oleksandr,
Thank you for your close review.
On 23.08.25 17:39, Oleksandr Tyshchenko wrote:
>
>
> On 07.08.25 15:33, Leonid Komarianskyi wrote:
>
>
> Hello Leonid
>
>> 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_komarians...@epam.com>
>>
>> ---
>> 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) );
>> +}
>> +#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;
>
> please use unsigned int if "i" cannot be negative
>
Sure, I will change the type to unsigned int in V3.
>> +
>> + 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,
>
> domain_vgic_init() is also called for dom0less DomUs (if present), which
> are created at Xen boot. So the toolstack might even be absent on the
> target system.
>
Thank you for pointing this out. I revised the patch series and found
that I did not change the behavior for dom0less builds. I will fix it in
V3 and update the comment.
>> + * 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.
>> + */
>> + 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++ )
>
> NIT: no need for () around d->arch.vgic.nr_espis
>
I will remove them in V3.
>> + {
>> + 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++ )
>
> NIT: no need for () around d
I will remove them in V3.
>
> [snip]
Best regards,
Leonid
