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_komarians...@epam.com>
>> Reviewed-by: Volodymyr Babchuk <volodymyr_babc...@epam.com>
>>
>> ---
>> 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 <jgr...@amazon.com>
>
> Cheers,
>
Thank you for your AB. I will fix the nit in V5.
Best regards,
Leonid
