Re: [PATCH v4 01/12] xen/arm: gicv3: refactor obtaining GIC addresses for common operations

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hi Julien,


On 28.08.25 15:00, Julien Grall wrote:
> Hi Leonid,
> 
> On 27/08/2025 19:24, Leonid Komarianskyi wrote:
>> Currently, many common functions perform the same operations to calculate
>> GIC register addresses. This patch consolidates the similar code into
>> a separate helper function to improve maintainability and reduce 
>> duplication.
>> This refactoring also simplifies the implementation of eSPI support in 
>> future
>> changes.
>>
>> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@xxxxxxxx>
>> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
>>
>> ---
>> Changes in V4:
>> - no changes
>>
>> Changes in V3:
>> - changed panic() in get_addr_by_offset() to printing warning and
>>    ASSERT_UNREACHABLE()
>> - added verification of return pointer from get_addr_by_offset() in the
>>    callers
>> - moved invocation of get_addr_by_offset() from spinlock guards, since
>>    it is not necessarry
>> - added RB from Volodymyr Babchuk
> 
> Procces remark, here you said the Reviewed-by from Volodymyr was added 
> in v3. However, given the changes you made this should have been 
> invalidated (reviewed-by means the person read the code and confirmed it 
> is correct).
> 
> I see Volodymyr confirmed his reviewed-by on v3. So no issue, but this 
> should have been clarified in the changelog.
> 

Thank you for your explanation.
Just to clarify: would it be okay to leave the RB tag (with appropriate 
text in the changelog) if I fix some minor nit from another reviewer in 
the next version, like in this patch?

>>
>> Changes in V2:
>> - no changes
>> ---
>>   xen/arch/arm/gic-v3.c          | 114 +++++++++++++++++++++++----------
>>   xen/arch/arm/include/asm/irq.h |   1 +
>>   2 files changed, 81 insertions(+), 34 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index cd3e1acf79..a959fefebe 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -445,17 +445,67 @@ static void gicv3_dump_state(const struct vcpu *v)
>>       }
>>   }
>> +static void __iomem *get_addr_by_offset(struct irq_desc *irqd, u32 
>> offset)
>> +{
>> +    switch ( irqd->irq )
>> +    {
>> +    case 0 ... (NR_GIC_LOCAL_IRQS - 1):
>> +        switch ( offset )
>> +        {
>> +        case GICD_ISENABLER:
>> +        case GICD_ICENABLER:
>> +        case GICD_ISPENDR:
>> +        case GICD_ICPENDR:
>> +        case GICD_ISACTIVER:
>> +        case GICD_ICACTIVER:
>> +            return (GICD_RDIST_SGI_BASE + offset);
>> +        case GICD_ICFGR:
>> +            return (GICD_RDIST_SGI_BASE + GICR_ICFGR1);
>> +        case GICD_IPRIORITYR:
>> +            return (GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 + irqd->irq);
>> +        default:
>> +            break;
>> +        }
>> +    case NR_GIC_LOCAL_IRQS ... SPI_MAX_INTID:
>> +        switch ( offset )
>> +        {
>> +        case GICD_ISENABLER:
>> +        case GICD_ICENABLER:
>> +        case GICD_ISPENDR:
>> +        case GICD_ICPENDR:
>> +        case GICD_ISACTIVER:
>> +        case GICD_ICACTIVER:
>> +            return (GICD + offset + (irqd->irq / 32) * 4);
>> +        case GICD_ICFGR:
>> +            return (GICD + GICD_ICFGR + (irqd->irq / 16) * 4);
>> +        case GICD_IROUTER:
>> +            return (GICD + GICD_IROUTER + irqd->irq * 8);
>> +        case GICD_IPRIORITYR:
>> +            return (GICD + GICD_IPRIORITYR + irqd->irq);
>> +        default:
>> +            break;
>> +        }
>> +    default:
>> +        break;
>> +    }
>> +
>> +    /* Something went wrong, we shouldn't be able to reach here */
>> +    printk(XENLOG_WARNING "GICv3: WARNING: Invalid offset 0x%x for 
>> IRQ#%d",
> 
> NIT: I am not expecting the interrupt to be < 0. So it would be 
> preferable to use %u.
> 
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
> 
> Cheers,
> 

Thank you for your AB. I will fix the nit in V5.

Best regards,
Leonid