Hi Volodymyr,
Thank you for your review.
On 04.09.25 00:24, Volodymyr Babchuk wrote:
> Hi Leonid,
>
> 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 for V6:
>> - introduced a new function for index to virq conversion, idx_to_virq.
>> This allows the removal of eSPI-specific functions and enables the
>> reuse of existing code for regular SPIs
>> - fixed the return value of vgic_allocate_virq in the case of eSPI
>> - updated the error message in route_irq_to_guest(). To simplify it and
>> avoid overcomplicating with INTID ranges, propose removing the
>> information about the max number of IRQs
>> - fixed eSPI rank initialization to avoid using EXT_RANK_IDX2NUM for
>> converting the eSPI rank to the extended range
>> - updated the recalculation logic to avoid the nr_spis > 1020 -
>> NR_LOCAL_IRQS check when nr_spis is reassigned in the case of eSPI
>> - fixed formatting issues for macros
>> - minor change: changed the macros ESPI_INTID2IDX and ESPI_IDX2INTID
>> into appropriate inline functions introduced in the previous patch
>> - minor change: changed int to unsigned int for nr_espis
>>
>> Changes in V5:
>> - removed the has_espi field because it can be determined by checking
>> whether domain->arch.vgic.nr_espis is zero or not
>> - since vgic_ext_rank_offset is not used in this patch, it has been
>> moved to the appropriate patch in the patch series, which implements
>> vgic eSPI registers emulation and requires this function
>> - removed ifdefs for eSPI-specific macros to reduce the number of ifdefs
>> and code duplication in further changes
>> - fixed minor nit: used %pd for printing domain with its ID
>>
>> Changes in V4:
>> - added has_espi field to simplify determining whether a domain is able
>> to operate with eSPI
>> - fixed formatting issues and misspellings
>>
>> Changes in V3:
>> - fixed formatting for lines with more than 80 symbols
>> - introduced helper functions to be able to use stubs in case of
>> CONFIG_GICV3_ESPI disabled, and as a result, reduce the number of
>> #ifdefs
>> - fixed checks for nr_spis in domain_vgic_init
>> - updated comment about nr_spis adjustments with dom0less mention
>> - moved comment with additional explanations before checks
>> - used unsigned int for indexes since they cannot be negative
>> - removed unnecessary parentheses
>> - move vgic_ext_rank_offset to the below ifdef guard, to reduce the
>> number of ifdefs
>>
>> 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 | 12 +++
>> xen/arch/arm/irq.c | 3 +-
>> xen/arch/arm/vgic.c | 174 ++++++++++++++++++++++++++++++--
>> 3 files changed, 176 insertions(+), 13 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/vgic.h
>> b/xen/arch/arm/include/asm/vgic.h
>> index 3e7cbbb196..1cf0a05832 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;
>> + unsigned 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)
>> +#endif
>> +#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)
>> +
>> #define vgic_lock(v) spin_lock_irq(&(v)->domain->arch.vgic.lock)
>> #define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)
>>
>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>> index c934d39bf6..c2f1ceb91f 100644
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -494,8 +494,7 @@ int route_irq_to_guest(struct domain *d, unsigned int
>> virq,
>> if ( !vgic_is_valid_line(d, virq) )
>> {
>> printk(XENLOG_G_ERR
>> - "the vIRQ number %u is too high for domain %u (max = %u)\n",
>> - irq, d->domain_id, vgic_num_irqs(d));
>> + "invalid vIRQ number %u for domain %pd\n", irq, d);
>> return -EINVAL;
>> }
>>
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 2bbf4d99aa..b42aee8d0c 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -25,11 +25,61 @@
>> #include <asm/vgic.h>
>>
>>
>> +static inline unsigned int idx_to_virq(struct domain *d, unsigned int idx)
>> +{
>> + if ( idx >= vgic_num_irqs(d) )
>> + return espi_idx_to_intid(idx - vgic_num_irqs(d));
>> +
>> + return idx;
>> +}
>> +
>> bool vgic_is_valid_line(struct domain *d, unsigned int virq)
>> {
>> +#ifdef CONFIG_GICV3_ESPI
>> + if ( virq >= ESPI_BASE_INTID &&
>> + virq < espi_idx_to_intid(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);
>> +}
>> +
>> +static inline struct vgic_irq_rank *vgic_get_espi_rank(struct vcpu *v,
>> + unsigned int rank)
>> +{
>> + return &v->domain->arch.vgic.ext_shared_irqs[EXT_RANK_NUM2IDX(rank)];
>> +}
>> +
>> +#else
>> +static inline bool is_valid_espi_rank(struct domain *d, unsigned int rank)
>> +{
>> + return false;
>> +}
>> +
>> +/*
>> + * This function is stub and will not be called if CONFIG_GICV3_ESPI=n,
>> + * because in this case, is_valid_espi_rank will always return false.
>> + */
>> +static inline struct vgic_irq_rank *vgic_get_espi_rank(struct vcpu *v,
>> + unsigned int rank)
>> +{
>> + ASSERT_UNREACHABLE();
>> + return NULL;
>> +}
>> +#endif
>> +
>> static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
>> unsigned int rank)
>> {
>> @@ -37,6 +87,8 @@ 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];
>> + else if ( is_valid_espi_rank(v->domain, rank) )
>> + return vgic_get_espi_rank(v, rank);
>> else
>> return NULL;
>> }
>> @@ -117,6 +169,54 @@ int domain_vgic_register(struct domain *d, unsigned int
>> *mmio_count)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_GICV3_ESPI
>> +static unsigned int vgic_num_spi_lines(struct domain *d)
>> +{
>> + return d->arch.vgic.nr_spis + d->arch.vgic.nr_espis;
>> +}
>> +
>> +static int init_vgic_espi(struct domain *d)
>> +{
>> + unsigned int i, idx;
>> +
>> + 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 = d->arch.vgic.nr_spis, idx = 0;
>> + i < vgic_num_spi_lines(d); i++, idx++ )
>> + vgic_init_pending_irq(&d->arch.vgic.pending_irqs[i],
>> + espi_idx_to_intid(idx));
>> +
>> + for ( i = 0; i < DOMAIN_NR_EXT_RANKS(d); i++ )
>> + vgic_rank_init(&d->arch.vgic.ext_shared_irqs[i],
>> + EXT_RANK_IDX2NUM(i), 0);
>> +
>> + return 0;
>> +}
>> +
>> +#else
>> +static int init_vgic_espi(struct domain *d)
>> +{
>> + return 0;
>> +}
>> +
>> +static unsigned int vgic_num_spi_lines(struct domain *d)
>> +{
>> + return d->arch.vgic.nr_spis;
>> +}
>> +
>> +#endif
>> +
>> +static unsigned int vgic_num_alloc_irqs(struct domain *d)
>> +{
>> + return vgic_num_spi_lines(d) + NR_LOCAL_IRQS;
>
> This is good that you are using NR_LOCAL_IRQS here.
>
>> +}
>> +
>> int domain_vgic_init(struct domain *d, unsigned int nr_spis)
>> {
>> int i;
>> @@ -133,7 +233,39 @@ int domain_vgic_init(struct domain *d, unsigned int
>> nr_spis)
>>
>> /* Limit the number of virtual SPIs supported to (1020 - 32) = 988 */
>> if ( nr_spis > (1020 - NR_LOCAL_IRQS) )
>> +#ifndef CONFIG_GICV3_ESPI
>> return -EINVAL;
>> +#else
>> + {
>> + /*
>> + * During domain creation, the dom0less DomUs code or toolstack
>> + * specifies the maximum INTID, which is defined in the domain
>> + * config subtracted by 32 to cover the local IRQs (please see
>> + * the comment to VGIC_DEF_NR_SPIS). To compute the actual number
>> + * of eSPI that will be usable for, add back 32.
>> + */
>> + nr_spis += 32;
>
> I think that it is better to use NR_LOCAL_IRQS here.
>
> (Also, I just noticed the same problem as with documentation, meaning
> that nr_spis actually does not represent number of SPIs anymore, and
> behaves more like max_spi, but let's set this issue aside for now)
>
>> + if ( nr_spis > espi_idx_to_intid(NR_ESPI_IRQS) )
>> + return -EINVAL;
>> +
>> + if ( nr_spis >= ESPI_BASE_INTID )
>> + {
>> + unsigned int nr_espis = min(nr_spis - ESPI_BASE_INTID, 1024U);
>> +
>> + /* Verify if GIC HW can handle provided INTID */
>> + if ( nr_espis > gic_number_espis() )
>> + return -EINVAL;
>> +
>> + d->arch.vgic.nr_espis = nr_espis;
>> + /* Set the maximum available number for regular SPIs */
>> + nr_spis = VGIC_DEF_NR_SPIS;
>> + }
>> + else
>> + {
>> + return -EINVAL;
>> + }
>> + }
>> +#endif
>>
>> d->arch.vgic.nr_spis = nr_spis;
>>
>> @@ -145,7 +277,7 @@ int domain_vgic_init(struct domain *d, unsigned int
>> nr_spis)
>> return -ENOMEM;
>>
>> d->arch.vgic.pending_irqs =
>> - xzalloc_array(struct pending_irq, d->arch.vgic.nr_spis);
>> + xzalloc_array(struct pending_irq, vgic_num_spi_lines(d));
>> if ( d->arch.vgic.pending_irqs == NULL )
>> return -ENOMEM;
>>
>> @@ -156,12 +288,16 @@ 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);
>>
>> + ret = init_vgic_espi(d);
>> + if ( ret )
>> + return ret;
>> +
>> ret = d->arch.vgic.handler->domain_init(d);
>> if ( ret )
>> return ret;
>>
>> d->arch.vgic.allocated_irqs =
>> - xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d)));
>> + xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_alloc_irqs(d)));
>> if ( !d->arch.vgic.allocated_irqs )
>> return -ENOMEM;
>>
>> @@ -182,9 +318,12 @@ void domain_vgic_free(struct domain *d)
>> int i;
>> int ret;
>>
>> - for ( i = 0; i < (d->arch.vgic.nr_spis); i++ )
>> + for ( i = 32; i < vgic_num_alloc_irqs(d); i++ )
>
> s/32/NR_LOCAL_IRQS
>
>> {
>> - struct pending_irq *p = spi_to_pending(d, i + 32);
>> + struct pending_irq *p;
>> + unsigned int virq = idx_to_virq(d, i);
>> +
>> + p = spi_to_pending(d, virq);
>>
>> if ( p->desc )
>> {
>> @@ -198,6 +337,9 @@ void domain_vgic_free(struct domain *d)
>> 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);
>> }
>> @@ -323,10 +465,12 @@ void arch_move_irqs(struct vcpu *v)
>> */
>> ASSERT(!is_lpi(vgic_num_irqs(d) - 1));
>>
>> - for ( i = 32; i < vgic_num_irqs(d); i++ )
>> + for ( i = 32; i < vgic_num_alloc_irqs(d); i++ )
>
> It is great idea to start using NR_LOCAL_IRQS here instead of hard-coded 32.
>
>> {
>> - v_target = vgic_get_target_vcpu(v, i);
>> - p = irq_to_pending(v_target, i);
>> + unsigned int virq = idx_to_virq(d, i);
>> +
>> + v_target = vgic_get_target_vcpu(v, virq);
>> + p = irq_to_pending(v_target, virq);
>>
>> if ( v_target == v && !test_bit(GIC_IRQ_GUEST_MIGRATING,
>> &p->status) )
>> irq_set_affinity(p->desc, cpu_mask);
>> @@ -539,7 +683,7 @@ struct pending_irq *irq_to_pending(struct vcpu *v,
>> unsigned int irq)
>> else if ( is_lpi(irq) )
>> n = v->domain->arch.vgic.handler->lpi_to_pending(v->domain, irq);
>> else
>> - n = &v->domain->arch.vgic.pending_irqs[irq - 32];
>> + n = spi_to_pending(v->domain, irq);
>> return n;
>> }
>>
>> @@ -547,7 +691,12 @@ struct pending_irq *spi_to_pending(struct domain *d,
>> unsigned int irq)
>> {
>> ASSERT(irq >= NR_LOCAL_IRQS);
>>
>> - return &d->arch.vgic.pending_irqs[irq - 32];
>> + if ( is_espi(irq) )
>> + irq = espi_intid_to_idx(irq) + d->arch.vgic.nr_spis;
>> + else
>> + irq -= 32;
>
> s/32/NR_LOCAL_IRQS
>
Okay, I will replace the hardcoded values with NR_LOCAL_IRQS here and in
similar places. :)
> Also, I want you to consider adding local variable "idx" instead of
> reusing "irq" parameter.
>
And also use idx in this case.
>> +
>> + return &d->arch.vgic.pending_irqs[irq];
>> }
>>
>> void vgic_clear_pending_irqs(struct vcpu *v)
>> @@ -668,6 +817,9 @@ bool vgic_reserve_virq(struct domain *d, unsigned int
>> virq)
>> if ( !vgic_is_valid_line(d, virq) )
>> return false;
>>
>> + if ( is_espi(virq) )
>> + virq = espi_intid_to_idx(virq) + vgic_num_irqs(d);
>
> Here as well. It is very confusing when you are updating virq with a
> value that does not corresponds in IRQ number anymore.
>
>> +
>> return !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
>> }
>>
>> @@ -685,7 +837,7 @@ int vgic_allocate_virq(struct domain *d, bool spi)
>> else
>> {
>> first = 32;
>> - end = vgic_num_irqs(d);
>> + end = vgic_num_alloc_irqs(d);
>> }
>>
>> /*
>> @@ -700,7 +852,7 @@ int vgic_allocate_virq(struct domain *d, bool spi)
>> }
>> while ( test_and_set_bit(virq, d->arch.vgic.allocated_irqs) );
>>
>> - return virq;
>> + return idx_to_virq(d, virq);
>> }
>>
>> void vgic_free_virq(struct domain *d, unsigned int virq)
>
Best regards,
Leonid
