On 04.09.25 18:10, Leonid Komarianskyi wrote:
> Hello Oleksandr,
Hello Leonid
>
> 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?
Sure, I do not mind, it was a question, but not a request for immediate
change.
>
>>> + /* 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
