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

[Julien Grall <julien@xxxxxxx>]

Hi Leonid,

On 22/08/2025 10:09, Leonid Komarianskyi wrote:
> Thank you for your close review.>>> ---
> > >    xen/arch/arm/gic-v3.c          | 99 ++++++++++++++++++++++------------
> > >    xen/arch/arm/include/asm/irq.h |  1 +
> > >    2 files changed, 67 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> > > index cd3e1acf79..8fd78aba44 100644
> > > --- a/xen/arch/arm/gic-v3.c
> > > +++ b/xen/arch/arm/gic-v3.c
> > > @@ -445,17 +445,62 @@ 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 */
> >   > +    panic("Invalid offset 0x%x for IRQ#%d", offset, irqd->irq);
> >
> > ... I still quite concerned about using panic here. We need to try to
> > handle the error more gracefully in production.
> >
> > Cheers,
> I was thinking about this again, and maybe it would be better to change
> the panic into simply printing an error using printk(XENLOG_G_ERR ...)
> and adding proper checks to ensure the return value is not NULL in the
> callers.
Given the error is not meant to happen, after the printk() I would add 
an ASSERT_UNREACHABLE() so we can catch issue in DEBUG build more easily.
> Also, in the case of gicv3_peek_irq, which must return a boolean value
> (due to the generic API for gicv3_read_pending_state), we could return
> false with an additional warning message that we are unable to read the
> actual value due to incorrect parameters; therefore, we return false.
> What do you think about this approach?
It makes sense to read false as the interrupt technically doesn't exist. 
But I don't think we should add an extra warning. The one in 
get_addr_by_offset() should be sufficient.
Cheers,

--
Julien Grall