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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Leonid,

Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx> writes:

> 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 V2:
> - 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

... with these changes, still:

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>


> - added RB from Volodymyr Babchuk
> ---
>  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",
> +           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. */

-- 
WBR, Volodymyr