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

[Julien Grall <julien@xxxxxxx>]

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.
> 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,

--
Julien Grall