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

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hi Julien,

Thank you for your close review.

On 21.08.25 19:14, Julien Grall wrote:
> Hi Leonid,
> 
> On 07/08/2025 13:33, Leonid Komarianskyi wrote:
>> 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>
>>
>> ---
>> Changes in V2:
>> - no changes
> 
> I am a bit surprised this is just saying no changes given the discussion 
> in v1. I feel you should have pinged on the v1 to close off the 
> discussion before sending v2.
> 

Sorry, my bad. I should have reached out to you and clarified the 
unfinished discussion in V1. I just wanted to publish the changes in V2 
that were agreed upon in V1. But yes, I understand - it was a bad idea 
to push V2 without completing the discussion in V1. I apologize for that.

> While I understand your point and could accept we consolidate the code...
> 
> 

Thank you for your understanding and acceptance :) It really simplifies 
the changes in the next patches.
>> ---
>>   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.
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?

Best regards,
Leonid