Re: [PATCH v4 02/12] xen/arm: gic: implement helper functions for INTID checks

[Julien Grall <julien@xxxxxxx>]

Hi Leonid,

On 27/08/2025 19:24, Leonid Komarianskyi wrote:
> Introduced two new helper functions: gic_is_valid_line and
> gic_is_spi. The first function helps determine whether an IRQ
> number is less than the number of lines supported by hardware. The
> second function additionally checks if the IRQ number falls within the
> SPI range. Also, updated the appropriate checks to use these new helper
> functions.
>
> The current checks for the real GIC are very similar to those for the
> vGIC but serve a different purpose. For GIC-related code, the interrupt
> numbers should be validated based on whether the hardware can operate
> with such interrupts. On the other hand, for the vGIC, the indexes must
> also be verified to ensure they are available for a specific domain. The
> first reason for introducing these helper functions is to avoid
> potential confusion with vGIC-related checks. The second reason is to
> consolidate similar code into separate functions, which can be more
> easily extended by additional conditions, e.g., when implementing
> extended SPI interrupts.
>
> The changes, which replace open-coded checks with the use of the new
> helper functions, do not introduce any functional changes, as the helper
> functions follow the current IRQ index verification logic.
>
> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@xxxxxxxx>
> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
With one remark below:

Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

> ---
> Changes in V4:
> - removed redundant parentheses
> - added reviewed-by from Volodymyr Babchuk
>
> Changes in V3:
> - renamed gic_is_valid_irq to gic_is_valid_line and gic_is_shared_irq to
>    gic_is_spi
> - updated commit message
>
> Changes in V2:
> - introduced this patch
>
> Changes for V4:
>
> Changes in V4:
> - removed redundant parentheses
> - added reviewed-by from Volodymyr Babchuk
> ---
>   xen/arch/arm/gic.c             | 2 +-
>   xen/arch/arm/include/asm/gic.h | 9 +++++++++
>   xen/arch/arm/irq.c             | 2 +-
>   3 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index e80fe0ca24..9220eef6ea 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -111,7 +111,7 @@ static void gic_set_irq_priority(struct irq_desc *desc, 
> unsigned int priority)
>   void gic_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
>   {
>       ASSERT(priority <= 0xff);     /* Only 8 bits of priority */
> -    ASSERT(desc->irq < gic_number_lines());/* Can't route interrupts that 
> don't exist */
> +    ASSERT(gic_is_valid_line(desc->irq));/* Can't route interrupts that don't 
> exist */
As you are touching the line. It is over 80 characters. Can you move the 
command in a separate line?
>       ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
>       ASSERT(spin_is_locked(&desc->lock));
Cheers,

--
Julien Grall