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

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hello Oleksandr,

Thank you for your comment and RB.

On 31.08.25 15:42, Oleksandr Tyshchenko wrote:
> 
> 
> On 29.08.25 19:06, Leonid Komarianskyi wrote:
> 
> Hello Leonid
> 
> 
>> 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>
>> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
> 
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> 
> with one NIT ...
> 
>>
>> ---
>> Changes in V5:
>> - fixed a minor nit: changed %d to %u in the warning message because the
>>    IRQ number cannot be less than zero
>> - added acked-by from Julien Grall
>>
>> 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
>>
>> 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..29b7f68cba 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)
> 
>   ... please use uint32_t for new code
> 
> (could probably updated on commit)
> 
> 

I will fix that in V6 and review the other patches to avoid the use of 
u32/u64.

>> +{
>> +    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#%u",
>> +           offset, irqd->irq);
>> +    ASSERT_UNREACHABLE();
>> +
>> +    return NULL;
>> +}
>> +
>>   static void gicv3_poke_irq(struct irq_desc *irqd, u32 offset, bool 
>> wait_for_rwp)
>>   {
>>       u32 mask = 1U << (irqd->irq % 32);
>> -    void __iomem *base;
>> +    void __iomem *addr = get_addr_by_offset(irqd, offset);
>> -    if ( irqd->irq < NR_GIC_LOCAL_IRQS )
>> -        base = GICD_RDIST_SGI_BASE;
>> -    else
>> -        base = GICD;
>> +    if ( addr == NULL )
>> +        return;
>> -    writel_relaxed(mask, base + offset + (irqd->irq / 32) * 4);
>> +    writel_relaxed(mask, addr);
>>       if ( wait_for_rwp )
>>           gicv3_wait_for_rwp(irqd->irq);
>> @@ -463,15 +513,12 @@ static void gicv3_poke_irq(struct irq_desc 
>> *irqd, u32 offset, bool wait_for_rwp)
>>   static bool gicv3_peek_irq(struct irq_desc *irqd, u32 offset)
>>   {
>> -    void __iomem *base;
>> -    unsigned int irq = irqd->irq;
>> +    void __iomem *addr = get_addr_by_offset(irqd, offset);
>> -    if ( irq >= NR_GIC_LOCAL_IRQS)
>> -        base = GICD + (irq / 32) * 4;
>> -    else
>> -        base = GICD_RDIST_SGI_BASE;
>> +    if ( addr == NULL )
>> +        return false;
>> -    return !!(readl(base + offset) & (1U << (irq % 32)));
>> +    return !!(readl(addr) & (1U << (irqd->irq % 32)));
>>   }
>>   static void gicv3_unmask_irq(struct irq_desc *irqd)
>> @@ -558,30 +605,28 @@ static inline uint64_t 
>> gicv3_mpidr_to_affinity(int cpu)
>>   static void gicv3_set_irq_type(struct irq_desc *desc, unsigned int 
>> type)
>>   {
>>       uint32_t cfg, actual, edgebit;
>> -    void __iomem *base;
>> -    unsigned int irq = desc->irq;
>> +    void __iomem *addr;
>>       /* SGI's are always edge-triggered not need to call GICD_ICFGR0 */
>> -    ASSERT(irq >= NR_GIC_SGI);
>> +    ASSERT(desc->irq >= NR_GIC_SGI);
>> -    spin_lock(&gicv3.lock);
>> +    addr = get_addr_by_offset(desc, GICD_ICFGR);
>> +    if ( addr == NULL )
>> +        return;
>> -    if ( irq >= NR_GIC_LOCAL_IRQS)
>> -        base = GICD + GICD_ICFGR + (irq / 16) * 4;
>> -    else
>> -        base = GICD_RDIST_SGI_BASE + GICR_ICFGR1;
>> +    spin_lock(&gicv3.lock);
>> -    cfg = readl_relaxed(base);
>> +    cfg = readl_relaxed(addr);
>> -    edgebit = 2u << (2 * (irq % 16));
>> +    edgebit = 2u << (2 * (desc->irq % 16));
>>       if ( type & IRQ_TYPE_LEVEL_MASK )
>>           cfg &= ~edgebit;
>>       else if ( type & IRQ_TYPE_EDGE_BOTH )
>>           cfg |= edgebit;
>> -    writel_relaxed(cfg, base);
>> +    writel_relaxed(cfg, addr);
>> -    actual = readl_relaxed(base);
>> +    actual = readl_relaxed(addr);
>>       if ( ( cfg & edgebit ) ^ ( actual & edgebit ) )
>>       {
>>           printk(XENLOG_WARNING "GICv3: WARNING: "
>> @@ -600,16 +645,13 @@ static void gicv3_set_irq_type(struct irq_desc 
>> *desc, unsigned int type)
>>   static void gicv3_set_irq_priority(struct irq_desc *desc,
>>                                      unsigned int priority)
>>   {
>> -    unsigned int irq = desc->irq;
>> +    void __iomem *addr = get_addr_by_offset(desc, GICD_IPRIORITYR);
>> -    spin_lock(&gicv3.lock);
>> -
>> -    /* Set priority */
>> -    if ( irq < NR_GIC_LOCAL_IRQS )
>> -        writeb_relaxed(priority, GICD_RDIST_SGI_BASE + 
>> GICR_IPRIORITYR0 + irq);
>> -    else
>> -        writeb_relaxed(priority, GICD + GICD_IPRIORITYR + irq);
>> +    if ( addr == NULL )
>> +        return;
>> +    spin_lock(&gicv3.lock);
>> +    writeb_relaxed(priority, addr);
>>       spin_unlock(&gicv3.lock);
>>   }
>> @@ -1273,6 +1315,10 @@ static void gicv3_irq_set_affinity(struct 
>> irq_desc *desc, const cpumask_t *mask)
>>   {
>>       unsigned int cpu;
>>       uint64_t affinity;
>> +    void __iomem *addr = get_addr_by_offset(desc, GICD_IROUTER);
>> +
>> +    if ( addr == NULL )
>> +        return;
>>       ASSERT(!cpumask_empty(mask));
>> @@ -1284,7 +1330,7 @@ static void gicv3_irq_set_affinity(struct 
>> irq_desc *desc, const cpumask_t *mask)
>>       affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>>       if ( desc->irq >= NR_GIC_LOCAL_IRQS )
>> -        writeq_relaxed_non_atomic(affinity, (GICD + GICD_IROUTER + 
>> desc->irq * 8));
>> +        writeq_relaxed_non_atomic(affinity, addr);
>>       spin_unlock(&gicv3.lock);
>>   }
>> diff --git a/xen/arch/arm/include/asm/irq.h b/xen/arch/arm/include/ 
>> asm/irq.h
>> index fce7e42a33..5bc6475eb4 100644
>> --- a/xen/arch/arm/include/asm/irq.h
>> +++ b/xen/arch/arm/include/asm/irq.h
>> @@ -29,6 +29,7 @@ struct arch_irq_desc {
>>    */
>>   #define NR_IRQS        1024
>> +#define SPI_MAX_INTID   1019
>>   #define LPI_OFFSET      8192
>>   /* LPIs are always numbered starting at 8192, so 0 is a good invalid 
>> case. */
> 

Best regards,
Leonid