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

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hi Volodymyr,

Thank tou for your review.

On 29.08.25 22:55, Volodymyr Babchuk wrote:
> 
> Hi Leonid,
> 
> Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx> writes:
> 
>> 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 V5:
>> - fixed minor nits, no functional changes: changed u32 to uint32_t and
>>    added a comment noting that the configuration for eSPIs is the same as
>>    for regular SPIs
>> - removed ifdefs for eSPI-specific offsets to reduce the number of
>>    ifdefs and code duplication in further changes
>> - removed reviewed-by as moving offset from ifdefs requires additional
>>    confirmation from reviewers
>>
>> Changes in V4:
>> - added offsets for GICD_IGRPMODRnE and GICD_NSACRnE that are required
>>    for vGIC emulation
>> - added a log banner with eSPI information, similar to the one for
>>    regular SPI
>> - added newline after ifdef and before gic_is_valid_line
>> - added reviewed-by from Volodymyr Babchuk
>>
>> Changes in V3:
>> - add __init attribute to gicv3_dist_espi_common_init
>> - change open-codded eSPI register initialization to the appropriate
>>    gen-mask macro
>> - fixed formatting for lines with more than 80 symbols
>> - introduced gicv3_dist_espi_init_aff to be able to use stubs in case of
>>    CONFIG_GICV3_ESPI disabled
>> - renamed parameter in the GICD_TYPER_ESPI_RANGE macro to espi_range
>>    (name was taken from GIC specification) to avoid confusion
>> - changed type for i variable to unsigned int since it cannot be
>>    negative
>>
>> 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                  | 83 ++++++++++++++++++++++++++
>>   xen/arch/arm/include/asm/gic.h         | 22 +++++++
>>   xen/arch/arm/include/asm/gic_v3_defs.h | 38 ++++++++++++
>>   3 files changed, 143 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 29b7f68cba..4a7ce12f26 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:
>> +    {
>> +        uint32_t 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;
>>       }
>> @@ -655,6 +685,55 @@ 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 __init gicv3_dist_espi_common_init(uint32_t type)
>> +{
>> +    unsigned int espi_nr, i;
>> +
>> +    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;
>> +
>> +    printk("GICv3: %d eSPI lines\n", gicv3_info.nr_espi);
>> +
>> +    /* The configuration for eSPIs is similar to that for regular SPIs */
>> +    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(GENMASK(31, 0), GICD + GICD_ICENABLERnE + (i / 32) * 
>> 4);
>> +        writel_relaxed(GENMASK(31, 0), 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);
>> +}
>> +
>> +static void __init gicv3_dist_espi_init_aff(uint64_t affinity)
>> +{
>> +    unsigned int i;
>> +
>> +    for ( i = 0; i < gicv3_info.nr_espi; i++ )
>> +        writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTERnE + i * 8);
>> +}
>> +#else
>> +static void __init gicv3_dist_espi_common_init(uint32_t type) { }
>> +
>> +static void __init gicv3_dist_espi_init_aff(uint64_t affinity) { }
>> +#endif
>> +
>>   static void __init gicv3_dist_init(void)
>>   {
>>       uint32_t type;
>> @@ -700,6 +779,8 @@ 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);
>>   
>> +    gicv3_dist_espi_common_init(type);
>> +
>>       gicv3_dist_wait_for_rwp();
>>   
>>       /* Turn on the distributor */
>> @@ -713,6 +794,8 @@ 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);
>> +
>> +    gicv3_dist_espi_init_aff(affinity);
>>   }
>>   
>>   static int gicv3_enable_redist(void)
>> diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
>> index 3fcee42675..1e747dcd99 100644
>> --- a/xen/arch/arm/include/asm/gic.h
>> +++ b/xen/arch/arm/include/asm/gic.h
>> @@ -306,8 +306,26 @@ extern void gic_dump_vgic_info(struct vcpu *v);
>>   
>>   /* Number of interrupt lines */
>>   extern unsigned int gic_number_lines(void);
>> +#ifdef CONFIG_GICV3_ESPI
>> +extern unsigned int gic_number_espis(void);
>> +
>> +static inline bool gic_is_valid_espi(unsigned int irq)
>> +{
>> +    return (irq >= ESPI_BASE_INTID &&
>> +            irq < ESPI_IDX2INTID(gic_number_espis()));
> 
> You don't need external () here.
> 

Oh, I missed that, sorry:( I will remove them in V6 and recheck the 
other patches for such isses.

>> +}
>> +#else
>> +static inline bool gic_is_valid_espi(unsigned int irq)
>> +{
>> +    return false;
>> +}
>> +#endif
>> +
>>   static inline bool gic_is_valid_line(unsigned int irq)
>>   {
>> +    if ( gic_is_valid_espi(irq) )
>> +        return true;
>> +
>>       return irq < gic_number_lines();
>>   }
> 
> As you are going to rework this patch anyways, my I ask to rewrite this
> function in the following way?
> 
> static inline bool gic_is_valid_line(unsigned int irq)
> {
>       return irq < gic_number_lines() || gic_is_valid_espi(irq);
> }
> 
> My justification is that (irq < gic_number_lines()) case is more likely,
> so it is better to evaluate it first, only then check for eSPIs.
> 
> I am sorry, I should asked it earlier, but only after removing #ifdef I
> saw that this part could be more optimal.
> 

Thank you for pointing this out:) It is really make sence, so I will fix 
this in V6.

>>   
>> @@ -325,6 +343,10 @@ struct gic_info {
>>       enum gic_version hw_version;
>>       /* Number of GIC lines supported */
>>       unsigned int nr_lines;
>> +#ifdef CONFIG_GICV3_ESPI
>> +    /* Number of GIC eSPI supported */
>> +    unsigned int nr_espi;
>> +#endif
>>       /* Number of LR registers */
>>       uint8_t nr_lrs;
>>       /* Maintenance irq number */
>> diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h 
>> b/xen/arch/arm/include/asm/gic_v3_defs.h
>> index 2af093e774..3370b4cd52 100644
>> --- a/xen/arch/arm/include/asm/gic_v3_defs.h
>> +++ b/xen/arch/arm/include/asm/gic_v3_defs.h
>> @@ -37,6 +37,44 @@
>>   #define GICD_IROUTER1019             (0x7FD8)
>>   #define GICD_PIDR2                   (0xFFE8)
>>   
>> +/* Additional registers for GICv3.1 */
>> +#define GICD_IGROUPRnE               (0x1000)
>> +#define GICD_IGROUPRnEN              (0x107C)
>> +#define GICD_ISENABLERnE             (0x1200)
>> +#define GICD_ISENABLERnEN            (0x127C)
>> +#define GICD_ICENABLERnE             (0x1400)
>> +#define GICD_ICENABLERnEN            (0x147C)
>> +#define GICD_ISPENDRnE               (0x1600)
>> +#define GICD_ISPENDRnEN              (0x167C)
>> +#define GICD_ICPENDRnE               (0x1800)
>> +#define GICD_ICPENDRnEN              (0x187C)
>> +#define GICD_ISACTIVERnE             (0x1A00)
>> +#define GICD_ISACTIVERnEN            (0x1A7C)
>> +#define GICD_ICACTIVERnE             (0x1C00)
>> +#define GICD_ICACTIVERnEN            (0x1C7C)
>> +#define GICD_IPRIORITYRnE            (0x2000)
>> +#define GICD_IPRIORITYRnEN           (0x23FC)
>> +#define GICD_ICFGRnE                 (0x3000)
>> +#define GICD_ICFGRnEN                (0x30FC)
>> +#define GICD_IGRPMODRnE              (0x3400)
>> +#define GICD_IGRPMODRnEN             (0x347C)
>> +#define GICD_NSACRnE                 (0x3600)
>> +#define GICD_NSACRnEN                (0x36FC)
>> +#define GICD_IROUTERnE               (0x8000)
>> +#define GICD_IROUTERnEN              (0x9FFC)
>> +
>> +#ifdef CONFIG_GICV3_ESPI
>> +#define GICD_TYPER_ESPI_SHIFT        8
>> +#define GICD_TYPER_ESPI_RANGE_SHIFT  27
>> +#define GICD_TYPER_ESPI_RANGE_MASK   (0x1F)
>> +#define GICD_TYPER_ESPI              (1U << GICD_TYPER_ESPI_SHIFT)
>> +#define GICD_TYPER_ESPI_RANGE(espi_range) ((((espi_range) & \
>> +        GICD_TYPER_ESPI_RANGE_MASK) + 1) * 32)
>> +#define GICD_TYPER_ESPIS_NUM(typer)    \
>> +        (((typer) & GICD_TYPER_ESPI) ? \
>> +        GICD_TYPER_ESPI_RANGE((typer) >> GICD_TYPER_ESPI_RANGE_SHIFT) : 0)
>> +#endif
>> +
>>   /* Common between GICD_PIDR2 and GICR_PIDR2 */
>>   #define GIC_PIDR2_ARCH_MASK         (0xf0)
>>   #define GIC_PIDR2_ARCH_GICv3        (0x30)
> 

Best regards,
Leonid