Hi Julien,
On 22.08.25 12:38, Julien Grall wrote:
> 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.
>
Okay, so I will change the panic to printk + ASSERT_UNREACHABLE() in V3.
Thank you.
>> 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,
>
Understood, thank you again.
Best regards,
Leonid
