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

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hello Julien,

Thank you for your close review and comments.

On 29.07.25 16:33, Julien Grall wrote:
> Hi Leonid,
> 
> On 24/07/2025 15:57, 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>
>> ---
>>   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);
> 
> This panic is worrying me because we have multiple switch above and it 
> is difficult to figure out whether everything was covered. ...

Initially, I was thinking about handling the errors more gently when 
dealing with invalid INTIDs or offsets. It looked like this:

     addr = get_addr_by_offset(desc, GICD_ICFGR);
     if ( !addr ) {
         pr_err("GICv3: cannot perform GIC operation invalid offset 0x%x 
for IRQ#%d\n", offset, irq);
         return;
     }

However, there is an issue with the gicv3_peek_irq function. Since it 
returns a bool, we cannot simply return either true or false from the 
function, as it will be interpreted as a valid value by the caller, even 
with an error message. For this reason, I decided to cover all valid 
INTIDs and offsets and trigger a panic only in an exceptionally rare 
case where we cannot obtain the correct address due to unexpected 
parameters.

Regarding your concerns about the panic:
After your comments, I rechecked the code and found that there is only 
one combination of INTID and offset where an invalid offset would 
trigger a panic compared with the mainline implementation:
gicv3_irq_set_affinity is invoked for INTIDs < NR_GIC_LOCAL_IRQS. 
Currently, in mainline implementation it does nothing in this particular 
case:

     if ( desc->irq >= NR_GIC_LOCAL_IRQS )
         writeq_relaxed_non_atomic(affinity, (GICD + GICD_IROUTER + 
desc->irq * 8));

This is not a valid situation because, according to the GICv3 
specification: GICD_IROUTER<n>, where n in range from 32 to 1019. 
Nevertheless, I will update the code to maintain the existing behavior 
unchanged without potential panic in this case.

Also, here are details, why panic will not be happened in real-life 
scenario:

The function is invoked from 5 places, and its behavior depends on two 
input parameters - the interrupt number and the offset. The main 
switch-case covers all valid INTIDs from 0 to 1019 (and will also cover 
the additional eSPI range from 4096 to 5119 with further changes). The 
current implementation prevents calling this function with different 
INTIDs, meaning that passing an invalid INTID represents a truly 
critical issue that could potentially result only from
improper code changes or unpredictable errors (e.g., memory corruption).
Moreover, compared to the current mainline implementation, passing an 
invalid IRQ would silently write to an unknown address, which is at 
least harder to debug. On the other hand, triggering a panic makes it 
much easier to identify, debug, and resolve this potential issue.

The second part is nested switch-case that handles all of possible 
defined values that can be passed to the function such as:
GICD_ISENABLER, GICD_ICENABLER, GICD_ISPENDR, GICD_ICPENDR, 
GICD_ISACTIVER, GICD_ICACTIVER, GICD_ICFGR, GICD_IROUTER and 
GICD_IPRIORITYR.
Even with the current mainline implementation, there are no ways to 
invoke e.g. gicv3_poke/peek_irq, gicv3_set_irq_type, 
gicv3_set_irq_priority or gicv3_irq_set_affinity with some differ offset 
values as mentioned previously.

To summarize, a panic would occur only in two cases: improper future 
code modifications or unpredictable errors (e.g., memory corruption).
In my opinion, it would require a major issue to trigger the panic.

What do you think? Is it acceptable to leave the panic here after the 
clarifications described above, or would it be better to handle it in a 
different way?


> ... Therefore if you want to consolidate some code, then I would prefer if 
> we introduce an helper just to deal with that logic. The rest (i.e. 
> ICFGR, IROUTER, IPRIORITY) should stay inlined.

The changes in this patch not only refactor existing logic but also 
simplify the implementation of eSPI handling in the third patch of this 
series:
https://patchew.org/Xen/cover.1753367178.git.leonid._5Fkomarianskyi@xxxxxxxx/790b1d3876206c8236986d7237173dacae99cac2.1753367178.git.leonid._5Fkomarianskyi@xxxxxxxx/

If this function were to handle only some of the required GIC registers, 
it would necessitate many additional #ifdefs and if conditions in the 
remaining functions. For example, if get_addr_by_offset did not handle 
GICD_IROUTER, GICD_ICFGR, and GICD_IPRIORITYR offsets, adding 
eSPI-related code would look like this:

gicv3_set_irq_type:
...
     if ( irq >= NR_GIC_LOCAL_IRQS)
         base = GICD + GICD_ICFGR + (irq / 16) * 4;
     else
         base = GICD_RDIST_SGI_BASE + GICR_ICFGR1;
#ifdef CONFIG_GICV3_ESPI
     if (is_espi(irq))
         base = (GICD + GICD_ICFGRnE + (ESPI_INTID2IDX(irq) / 16) * 4);
#endif

gicv3_irq_set_affinity:
...
#ifdef CONFIG_GICV3_ESPI
     if (is_espi(desc->irq))
         writeq_relaxed_non_atomic(affinity, (GICD + GICD_IROUTERnE + 
ESPI_INTID2IDX(desc->irq) * 8));
     else
#endif
     if ( desc->irq >= NR_GIC_LOCAL_IRQS )
         writeq_relaxed_non_atomic(affinity, (GICD + GICD_IROUTER + 
desc->irq * 8));

gicv3_set_irq_priority:
...
     /* Set priority */
     if ( irq < NR_GIC_LOCAL_IRQS )
         writeb_relaxed(priority, GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 
+ irq);
     else
#ifdef CONFIG_GICV3_ESPI
     if (is_espi(irq))
         writeb_relaxed(priority, GICD + GICD_IPRIORITYRnE + 
ESPI_INTID2IDX(irq));
     else
#endif
         writeb_relaxed(priority, GICD + GICD_IPRIORITYR + irq);


This code seems less readable, so I opted to refactor all relevant 
locations and prepare a single function that can be easily modified and 
maintained. By the way, I was inspired by a Linux kernel patch that was 
introduced in the same series as the eSPI support implementation for 
Linux. That patch refactored the code prior to implementing eSPI:
https://github.com/torvalds/linux/commit/e91b036e1c20d80419164ddfc32125052df3fb39
So I decided to make the changes in a similar way - on the first step, 
refactoring the existing functionality to make implementing eSPI support 
smoother. And then introduce the eSPI implementation.

I would really like to hear your thoughts after my comments, on whether 
keeping the current handling of the entire register group makes sense, 
or not.

Cheers,
Leonid