Re: [PATCH v4 09/12] xen/arm: domain_build/dom0less-build: adjust domains config to support eSPIs

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hi Oleksandr,

Thank you for your review.

On 28.08.25 16:19, Oleksandr Tyshchenko wrote:
> 
> 
> On 27.08.25 21:24, Leonid Komarianskyi wrote:
> 
> Hello Leonid
> 
>> The Dom0 and DomUs logic for the dom0less configuration in 
>> create_dom0() and
>> arch_create_domUs() has been updated to account for extended SPIs when
>> supported by the hardware and enabled with CONFIG_GICV3_ESPI. These 
>> changes
>> ensure the proper calculation of the maximum number of SPIs and eSPIs 
>> available
>> to Dom0 and DomUs in dom0less setups.
>>
>> When eSPIs are supported by the hardware and CONFIG_GICV3_ESPI is 
>> enabled, the
>> maximum number of eSPI interrupts is calculated using the ESPI_BASE_INTID
>> offset (4096) and is limited to 1024, with 32 IRQs subtracted. To ensure
>> compatibility with non-Dom0 domains, this adjustment is applied by the
>> toolstack during domain creation, while for Dom0 or DomUs in Dom0, it is
>> handled directly during VGIC initialization. If eSPIs are not 
>> supported, the
>> calculation defaults to using the standard SPI range, with a maximum 
>> value of
>> 992 interrupt lines, as it works currently.
>>
>> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@xxxxxxxx>
>>
>> ---
>> Changes in V4:
>> - consolidated the eSPI and SPI logic into a new inline function,
>>    vgic_def_nr_spis. Without eSPI support (either due to config being
>>    disabled or hardware not supporting it), it will return the regular 
>> SPI
>>    range, as it works currently. There are no functional changes compared
>>    with the previous patch version
>> - removed VGIC_DEF_MAX_SPI macro, to reduce the number of ifdefs
> 
> very good, thanks, only NIT below
> 
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> 
> 
> 
>>
>> Changes in V3:
>> - renamed macro VGIC_DEF_NR_ESPIS to more appropriate VGIC_DEF_MAX_SPI
>> - added eSPI initialization for dom0less setups
>> - fixed comment with mentions about dom0less builds
>> - fixed formatting for lines with more than 80 symbols
>> - updated commit message
>>
>> Changes in V2:
>> - no changes
>> ---
>>   xen/arch/arm/dom0less-build.c   |  2 +-
>>   xen/arch/arm/domain_build.c     |  2 +-
>>   xen/arch/arm/include/asm/vgic.h | 21 +++++++++++++++++++++
>>   3 files changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less- 
>> build.c
>> index 69b9ea22ce..02d5559102 100644
>> --- a/xen/arch/arm/dom0less-build.c
>> +++ b/xen/arch/arm/dom0less-build.c
>> @@ -285,7 +285,7 @@ void __init arch_create_domUs(struct 
>> dt_device_node *node,
>>       {
>>           int vpl011_virq = GUEST_VPL011_SPI;
>> -        d_cfg->arch.nr_spis = VGIC_DEF_NR_SPIS;
>> +        d_cfg->arch.nr_spis = vgic_def_nr_spis();
>>           /*
>>            * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map is
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index d91a71acfd..39eea0be00 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -2054,7 +2054,7 @@ void __init create_dom0(void)
>>       /* The vGIC for DOM0 is exactly emulating the hardware GIC */
>>       dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
>> -    dom0_cfg.arch.nr_spis = VGIC_DEF_NR_SPIS;
>> +    dom0_cfg.arch.nr_spis = vgic_def_nr_spis();
>>       dom0_cfg.arch.tee_type = tee_get_type();
>>       dom0_cfg.max_vcpus = dom0_max_vcpus();
>> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/ 
>> asm/vgic.h
>> index fb4cea73eb..11f9d216eb 100644
>> --- a/xen/arch/arm/include/asm/vgic.h
>> +++ b/xen/arch/arm/include/asm/vgic.h
>> @@ -355,6 +355,27 @@ extern void 
>> vgic_check_inflight_irqs_pending(struct vcpu *v,
>>   /* Default number of vGIC SPIs. 32 are substracted to cover local 
>> IRQs. */
>>   #define VGIC_DEF_NR_SPIS (min(gic_number_lines(), VGIC_MAX_IRQS) - 32)
>> +static inline unsigned int vgic_def_nr_spis(void)
>> +{
>> +#ifdef CONFIG_GICV3_ESPI
>> +    /*
>> +     * Check if the hardware supports extended SPIs (even if the 
>> appropriate
>> +     * config is set). If not, the common SPI range will be used. 
>> Otherwise
>> +     * returns the maximum eSPI INTID, supported by HW GIC, 
>> subtracted by 32.
> 
> NIT: s/returns/return ...
> 

I will fix this in V5.

> 
>> +     * For non-Dom0 domains, the toolstack or arch_create_domUs function
>> +     * applies the same adjustment to cover local IRQs (please, see 
>> comment
>> +     * for macro that is used for regular SPIs - VGIC_DEF_NR_SPIS).
> 
>   ... I am not 100% sure, that I follow the wording "For non-Dom0 
> domains, the toolstack or arch_create_domUs ...". Did you perhaps mean:
> 
> For Dom0 and started at boot time DomUs ...?
> 
>   We will

Yes, I agree that 'For Dom0 and started at boot time DomUs, we will add 
back...' sounds better :)

>> +     * add back this value during VGIC initialization. This ensures 
>> consistent
>> +     * handling for Dom0 and other domains. For the regular SPI range 
>> interrupts
>> +     * in this case, the maximum value of VGIC_DEF_NR_SPIS will be used.
>> +     */
>> +    if ( gic_number_espis() > 0 )
>> +        return ESPI_BASE_INTID + min(gic_number_espis(), 1024U) - 32;
>> +#endif
>> +
>> +    return VGIC_DEF_NR_SPIS;
>> +}
>> +
>>   extern bool vgic_is_valid_line(struct domain *d, unsigned int virq);
>>   static inline bool vgic_is_spi(struct domain *d, unsigned int virq)
> 

Best regards,
Leonid