Re: [PATCH v2 05/10] xen/arm: gicv3: implement handling of GICv3.1 eSPI

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hi Oleksandr,

Thank you for your review comments.

On 23.08.25 17:23, Oleksandr Tyshchenko wrote:
> [You don't often get email from olekstysh@xxxxxxxxx. Learn why this is 
> important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On 07.08.25 15:33, Leonid Komarianskyi wrote:
> 
> Hello Leonid
> 
> 
>> Introduced appropriate register definitions, helper macros,
>> and initialization of required GICv3.1 distributor registers
>> to support eSPI. This type of interrupt is handled in the
>> same way as regular SPI interrupts, with the following
>> differences:
>>
>> 1) eSPIs can have up to 1024 interrupts, starting from the
>> beginning of the range, whereas regular SPIs use INTIDs from
>> 32 to 1019, totaling 988 interrupts;
>> 2) eSPIs start at INTID 4096, necessitating additional interrupt
>> index conversion during register operations.
>>
>> In case if appropriate config is disabled, or GIC HW doesn't
>> support eSPI, the existing functionality will remain the same.
>>
>> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@xxxxxxxx>
>>
>> ---
>> Changes in V2:
>> - move gic_number_espis function from
>>    [PATCH 08/10] xen/arm: vgic: add resource management for extended SPIs
>>    to use it in the newly introduced gic_is_valid_espi
>> - add gic_is_valid_espi which checks if IRQ number is in supported
>>    by HW eSPI range
>> - update gic_is_valid_irq conditions to allow operations with eSPIs
>> ---
>>   xen/arch/arm/gic-v3.c                  | 73 ++++++++++++++++++++++++++
>>   xen/arch/arm/include/asm/gic.h         | 17 ++++++
>>   xen/arch/arm/include/asm/gic_v3_defs.h | 33 ++++++++++++
>>   3 files changed, 123 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 8fd78aba44..a0e8ee1a1e 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -485,6 +485,36 @@ static void __iomem *get_addr_by_offset(struct 
>> irq_desc *irqd, u32 offset)
>>           default:
>>               break;
>>           }
>> +#ifdef CONFIG_GICV3_ESPI
>> +    case ESPI_BASE_INTID ... ESPI_MAX_INTID:
>> +    {
>> +        u32 irq_index = ESPI_INTID2IDX(irqd->irq);
>> +
>> +        switch ( offset )
>> +        {
>> +        case GICD_ISENABLER:
>> +            return (GICD + GICD_ISENABLERnE + (irq_index / 32) * 4);
>> +        case GICD_ICENABLER:
>> +            return (GICD + GICD_ICENABLERnE + (irq_index / 32) * 4);
>> +        case GICD_ISPENDR:
>> +            return (GICD + GICD_ISPENDRnE + (irq_index / 32) * 4);
>> +        case GICD_ICPENDR:
>> +            return (GICD + GICD_ICPENDRnE + (irq_index / 32) * 4);
>> +        case GICD_ISACTIVER:
>> +            return (GICD + GICD_ISACTIVERnE + (irq_index / 32) * 4);
>> +        case GICD_ICACTIVER:
>> +            return (GICD + GICD_ICACTIVERnE + (irq_index / 32) * 4);
>> +        case GICD_ICFGR:
>> +            return (GICD + GICD_ICFGRnE + (irq_index / 16) * 4);
>> +        case GICD_IROUTER:
>> +            return (GICD + GICD_IROUTERnE + irq_index * 8);
>> +        case GICD_IPRIORITYR:
>> +            return (GICD + GICD_IPRIORITYRnE + irq_index);
>> +        default:
>> +            break;
>> +        }
>> +    }
>> +#endif
>>       default:
>>           break;
>>       }
>> @@ -645,6 +675,40 @@ static void gicv3_set_irq_priority(struct 
>> irq_desc *desc,
>>       spin_unlock(&gicv3.lock);
>>   }
>>
>> +#ifdef CONFIG_GICV3_ESPI
>> +unsigned int gic_number_espis(void)
>> +{
>> +    return gic_hw_ops->info->nr_espi;
>> +}
>> +
>> +static void gicv3_dist_espi_common_init(uint32_t type)
> 
> missing __init ?
> 

Yes, I missed this. I will add it in V3.

>> +{
>> +    unsigned int espi_nr;
>> +    int i;
> 
> please use unsigned int if "i" cannot be negative
> 

Sure, I will change int to unsigned int, because it really cannot be 
negative.

>> +
>> +    espi_nr = min(1024U, GICD_TYPER_ESPIS_NUM(type));
>> +    gicv3_info.nr_espi = espi_nr;
>> +    /* The GIC HW doesn't support eSPI, so we can leave from here */
>> +    if ( gicv3_info.nr_espi == 0 )
>> +        return;
>> +
>> +    for ( i = 0; i < espi_nr; i += 16 )
>> +        writel_relaxed(0, GICD + GICD_ICFGRnE + (i / 16) * 4);
>> +
>> +    for ( i = 0; i < espi_nr; i += 4 )
>> +        writel_relaxed(GIC_PRI_IRQ_ALL, GICD + GICD_IPRIORITYRnE + 
>> (i / 4) * 4);
>> +
>> +    for ( i = 0; i < espi_nr; i += 32 )
>> +    {
>> +        writel_relaxed(0xffffffffU, GICD + GICD_ICENABLERnE + (i / 
>> 32) * 4);
>> +        writel_relaxed(0xffffffffU, GICD + GICD_ICACTIVERnE + (i / 
>> 32) * 4);
>> +    }
>> +
>> +    for ( i = 0; i < espi_nr; i += 32 )
>> +        writel_relaxed(GENMASK(31, 0), GICD + GICD_IGROUPRnE + (i / 
>> 32) * 4);
>> +}
>> +#endif
>> +
>>   static void __init gicv3_dist_init(void)
>>   {
>>       uint32_t type;
>> @@ -690,6 +754,10 @@ static void __init gicv3_dist_init(void)
>>       for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i += 32 )
>>           writel_relaxed(GENMASK(31, 0), GICD + GICD_IGROUPR + (i / 
>> 32) * 4);
>>
>> +#ifdef CONFIG_GICV3_ESPI
>> +    gicv3_dist_espi_common_init(type);
>> +#endif
>> +
>>       gicv3_dist_wait_for_rwp();
>>
>>       /* Turn on the distributor */
>> @@ -703,6 +771,11 @@ static void __init gicv3_dist_init(void)
>>
>>       for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i++ )
>>           writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTER + i 
>> * 8);
>> +
>> +#ifdef CONFIG_GICV3_ESPI
>> +    for ( i = 0; i < gicv3_info.nr_espi; i++ )
>> +        writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTERnE + i 
>> * 8);
>> +#endif
>>   }
>>
>>   static int gicv3_enable_redist(void)
> 
> 
> [snip]
> 

Best regards,
Leonid