Hello Oleksandr,
Thank you for your comment.
On 04.09.25 17:37, Oleksandr Tyshchenko wrote:
>
>
> On 03.09.25 17:30, 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: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
>>
>> ---
>> Changes in V6:
>> - removed unnecessary parentheses in gic_is_valid_espi()
>> - updated gic_is_valid_line(): it now verifies the condition irq <
>> gic_number_lines() first, as it is more likely that the irq number
>> will be from the non-eSPI range
>> - minor change: changed the macros ESPI_INTID2IDX and ESPI_IDX2INTID
>> into appropriate inline functions introduced in the previous patch
>> - added reviewed-by from Oleksandr Tyshchenko
>>
>> 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 | 21 ++++++-
>> xen/arch/arm/include/asm/gic_v3_defs.h | 38 ++++++++++++
>> 3 files changed, 141 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index a1e302fea2..a69263e461 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, uint32_t offset)
>> default:
>> break;
>> }
>> +#ifdef CONFIG_GICV3_ESPI
>> + case ESPI_BASE_INTID ... ESPI_MAX_INTID:
>> + {
>> + uint32_t irq_index = espi_intid_to_idx(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;
>
>
> Sorry, I have just noticed one thing, and gicv3_cpu_init() probably
> would be a more correct place to write about it, but since you don't
> modify that function (it is not visible in the context), so writing here:
>
> From "Arm IHI 0069H.b (ID041224)"
> 10.1.2 GICv3.1 extended INTID range support
>
> Note
> Arm recommends that Armv8-R AArch64 PEs report ICC_CTLR_EL1.ExtRange==1,
> indicating that the GICv3.1 extended SPI and PPI ranges are supported.
>
> Linux driver has an extra check for that:
>
> WARN((gic_data.ppi_nr > 16 || GIC_ESPI_NR != 0) &&
> !(gic_read_ctlr() & ICC_CTLR_EL1_ExtRange),
> "Distributor has extended ranges, but CPU%d doesn't\n",
> smp_processor_id());
>
> added by the following commit:
> irqchip/gic-v3: Warn about inconsistent implementations of extended ranges
> https://eur01.safelinks.protection.outlook.com/?
> url=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fcommit%2Fad5a78d3da81836c88d1f2d53310484462660997&data=05%7C02%7CLeonid_Komarianskyi%40epam.com%7C910bd935bb4f497df12b08ddebc08b2c%7Cb41b72d04e9f4c268a69f949f367c91d%7C1%7C0%7C638925934406096963%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=JjXAx9z0%2Frdf9ul%2Fv79d7q%2Fyb4Mn8twuiRlxYQHE1HA%3D&reserved=0
>
>
> What is your opinion, is it worth having a similar check in Xen?
>
>
I think we can omit adding such a warning for now, if you don't mind. I
will analyze it in more detail and send it as a follow-up patch later if
needed. Would that be okay for you?
>> + /* 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);
>> +
>
>
> [snip]
>
Best regards,
Leonid
