Hi Oleksandr,
Thank you for your review.
On 28.08.25 17:15, Oleksandr Tyshchenko wrote:
>
>
> On 27.08.25 21:24, 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_komarians...@epam.com>
>> Reviewed-by: Volodymyr Babchuk <volodymyr_babc...@epam.com>
>>
>> ---
>> 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
>
> only NITs below
>
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
>
>>
>> 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
>>
>> Changes for V4:
>>
>> Changes in V4:
>> - added offsets for GICD_IGRPMODRnE and GICD_NSACRnE that are required
>> for vGIC emulation
>> - added newline after ifdef and before gic_is_valid_line
>> - added reviewed-by from Volodymyr Babchuk
>> - added a log banner with eSPI information, similar to the one for
>> regular SPI
>> ---
>> xen/arch/arm/gic-v3.c | 82 ++++++++++++++++++++++++++
>> xen/arch/arm/include/asm/gic.h | 22 +++++++
>> xen/arch/arm/include/asm/gic_v3_defs.h | 38 ++++++++++++
>> 3 files changed, 142 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index a959fefebe..b939a1f490 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);
>
> NIT: I have heard that no uN for new code, but uintN_t (sorry for didn't
> spot this before), so I would use uint32_t
>
Okay, I will change u32 to uint32_t in V5.
>
>> +
>> + 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,54 @@ 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);
>> +
>> + 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);
>
> NIT: From what I see, the eSPIs are configured exactly as regular SPIs
> in gicv3_dist_init() (i.e. the eSPIs are level-triggered, disabled and
> deactivated, belong to the same group, etc). In gicv3_dist_init() we
> have comments clarying the actions, but here we do not. I would at least
> write a sentence in patch description/in-code comment saying that eSPIs
> configuration is the same as for regular SPIs.
>
Sure, I will add comments in the code, similar to how it is done for
regular SPIs.
>
>> +}
>> +
>> +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 +778,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 +793,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)
>
>
> [snip]
>
Best regards,
Leonid
