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

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hi Volodymyr and Julien,

Thank you for your comments and for your time.

On 21.08.25 19:24, Julien Grall wrote:
> 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()?

Oh, thank you. It would be much better to name the function 
gic_is_valid_line(), so I will rename it in V3.

>>
>>> +{
>>> +    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,
> 

Okay, I will rename gic_is_shared_irq to gic_is_spi in V3.

Best regards,
Leonid