Hi Julien,
Thank you for your review.
On 28.08.25 15:10, Julien Grall wrote:
> 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_komarians...@epam.com>
>> Reviewed-by: Volodymyr Babchuk <volodymyr_babc...@epam.com>
>
> With one remark below:
>
> Acked-by: Julien Grall <jgr...@amazon.com>
>
>> ---
>> 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?
Sure, I will fix formatting in V5.
>
>> ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
>> ASSERT(spin_is_locked(&desc->lock));
>
> Cheers,
>
Best regards,
Leonid
