Hi Volodymyr,
Thank you for your review comments.
On 21.08.25 19:43, Volodymyr Babchuk wrote:
>
> Leonid Komarianskyi <leonid_komarians...@epam.com> 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_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) );
>
> I am pretty sure that max line length in 80 symbols, and here you have
> more. As well in couple of other places below.
>
Previously, I had seen code in the mainline with lines longer than 80
characters, so I thought the maximum length was 100.. However, I
rechecked the CODING_STYLE file in Xen and it indeed contains the following:
> Lines should be less than 80 characters in length. Long lines should
> be split at sensible places and the trailing portions indented.
So, yes, my mistake. I will revise the code in the patch series and fix
the places where the line length exceeds 80 characters.
>> +}
>> +#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?
Oh, yes, the verifications above do not take into account this
subtraction of 32, so they will fail for the first 32 eSPI INTIDs and
mistakenly allow processing INTIDs greater than 5119. I will fix that in
V3 by adding 32 to nr_spis if checks and moving this comment above the
if checks.
>
>> + */
>> + 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
>> +
>> }
>>
>> /*
>
Best regards,
Leonid
