Re: [PATCH v2 03/10] xen/arm: vgic: implement helper functions for virq checks

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On 07.08.25 15:33, Leonid Komarianskyi wrote:

Hello Leonid


> Introduced two new helper functions for vGIC: vgic_is_valid_irq and
> vgic_is_shared_irq. The functions are similar to the newly introduced
> gic_is_valid_irq and gic_is_shared_irq, but they verify whether a vIRQ
> is available for a specific domain, while GIC-specific functions
> validate INTIDs for the real GIC hardware. For example, the GIC may
> support all 992 SPI lines, but the domain may use only some part of them
> (e.g., 640), depending on the highest IRQ number defined in the domain
> configuration. Therefore, for vGIC-related code and checks, the
> appropriate functions should be used. Also, updated the appropriate
> checks to use these new helper functions.
>
> The purpose of introducing new helper functions for vGIC is essentially
> the same as for GIC: to avoid potential confusion with GIC-related
> checks and to consolidate similar code into separate functions, which
> can be more easily extended by additional conditions, e.g., when
> implementing extended SPI interrupts.
>
> Only the validation change in vgic_inject_irq may affect existing
> functionality, as it currently checks whether the vIRQ is less than or
> equal to vgic_num_irqs. Since IRQ indexes start from 0 (where 32 is the
> first SPI), the check should behave consistently with similar logic in
> other places and should check if the vIRQ number is less than
> vgic_num_irqs. The remaining changes, which replace open-coded checks
> with the use of these new helper functions, do not introduce any
> functional changes, as the helper functions follow the current vIRQ
> index verification logic.
>
> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@xxxxxxxx>
>
> ---
> Changes in V2:
> - introduced this patch
> ---
>   xen/arch/arm/gic.c              |  3 +--
>   xen/arch/arm/include/asm/vgic.h |  7 +++++++
>   xen/arch/arm/irq.c              |  4 ++--
>   xen/arch/arm/vgic.c             | 10 ++++++++--
>   4 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index eb0346a898..47fccf21d8 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -133,8 +133,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int 
> virq,
>   
>       ASSERT(spin_is_locked(&desc->lock));
>       /* Caller has already checked that the IRQ is an SPI */
> -    ASSERT(virq >= 32);
> -    ASSERT(virq < vgic_num_irqs(d));
> +    ASSERT(vgic_is_shared_irq(d, virq));
>       ASSERT(!is_lpi(virq));
>   
>       ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
> index 35c0c6a8b0..45201f4ca5 100644
> --- a/xen/arch/arm/include/asm/vgic.h
> +++ b/xen/arch/arm/include/asm/vgic.h
> @@ -335,6 +335,13 @@ extern void vgic_check_inflight_irqs_pending(struct vcpu 
> *v,
>   /* Default number of vGIC SPIs. 32 are substracted to cover local IRQs. */
>   #define VGIC_DEF_NR_SPIS (min(gic_number_lines(), VGIC_MAX_IRQS) - 32)
>   
> +extern bool vgic_is_valid_irq(struct domain *d, unsigned int virq);
> +
> +static inline bool vgic_is_shared_irq(struct domain *d, unsigned int virq)
> +{
> +    return (virq >= NR_LOCAL_IRQS && vgic_is_valid_irq(d, virq));
> +}
> +
>   /*
>    * Allocate a guest VIRQ
>    *  - spi == 0 => allocate a PPI. It will be the same on every vCPU
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 12c70d02cc..50e57aaea7 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -442,7 +442,7 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
>       unsigned long flags;
>       int retval = 0;
>   
> -    if ( virq >= vgic_num_irqs(d) )
> +    if ( !vgic_is_valid_irq(d, virq) )

This file is common for all VGIC implementations, so 
route_irq_to_guest() is used with CONFIG_NEW_VGIC=y as well.
If your series is built with CONFIG_NEW_VGIC=y (I know, that NEW_VGIC 
does not support GICV3 HW) I have got the following error:

aarch64-poky-linux-ld: prelink.o: in function `route_irq_to_guest':
/usr/src/debug/xen/4.18.0+gitAUTOINC+ce58f56108-r0/git/xen/arch/arm/irq.c:445: 
undefined reference to `vgic_is_valid_irq'
/usr/src/debug/xen/4.18.0+gitAUTOINC+ce58f56108-r0/git/xen/arch/arm/irq.c:445:(.text+0x5e2f8): 
relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol 
`vgic_is_valid_irq'
...

From the quick look, vgic_is_valid_irq() needs a stub (or NEW_VGIC's 
counterpart).
[snip]