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

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

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