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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 21/08/2025 16:39, Volodymyr Babchuk wrote:
> Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx> writes:
>
> > Introduced two new helper functions: gic_is_valid_irq and
> > gic_is_shared_irq. 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>
> >
> > ---
> > Changes in V2:
> > - introduced this patch
> > ---
> >   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..eb0346a898 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_irq(desc->irq));/* Can't route interrupts that don't 
> > exist */
> >       ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
> >       ASSERT(spin_is_locked(&desc->lock));
> >   
> > diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
> > index 541f0eeb80..ac0b7b783e 100644
> > --- a/xen/arch/arm/include/asm/gic.h
> > +++ b/xen/arch/arm/include/asm/gic.h
> > @@ -306,6 +306,15 @@ extern void gic_dump_vgic_info(struct vcpu *v);
> >   
> >   /* Number of interrupt lines */
> >   extern unsigned int gic_number_lines(void);
> > +static inline bool gic_is_valid_irq(unsigned int irq)
> We need to do something about naming, because this function completely
> ignores presence of LPIs. What I mean, that it will return "false" for
> any LPI, while you can't argue that LPI is a valid interrupt :)
> I understand that this is expected behavior by current callers, but the
> function name is misleading.
>
> Name like "gic_is_valid_non_lpi()" seems to mouthful, but it is the best
> I can come up with.
AFAIU, there is no interrupt lines for LPIs. So what about 
gic_is_valid_line()?
> > +{
> > +    return irq < gic_number_lines();
> > +}
> > +
> > +static inline bool gic_is_shared_irq(unsigned int irq)
> > +{
> > +    return (irq >= NR_LOCAL_IRQS && gic_is_valid_irq(irq));
> Again, because of misleading name of gic_is_valid_irq() it may seem that
> this function will return "true" for LPIs as well...
Even if we rename gic_is_valid_irq(), the function name would be 
misleading because LPIs are shared. I think it would be better named
gic_is_spi(...);

Cheers,

--
Julien Grall