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
