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

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hello Oleksandr,

Thank you for your review. It was really helpful.

On 27.08.25 15:01, Oleksandr Tyshchenko wrote:
> 
> 
> On 27.08.25 13:25, Leonid Komarianskyi wrote:
> 
> Hello Leonid
> 
>> Hello Volodymyr,
>>
>> Thank you for your suggestion.
>>
>> On 27.08.25 02:08, Volodymyr Babchuk wrote:
>>> Hi Leonid,
>>>
>>> Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx> writes:
>>>
>>>> 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 DomUs (Dom0 setups) domains,
>>>> where 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
>>>> 960 interrupt lines, as it works currently.
>>>>
>>>> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@xxxxxxxx>
>>>>
>>>> ---
>>>> Changes in V2:
>>>> - no changes
>>>>
>>>> Changes in V3:
>>>> - renamed macro VGIC_DEF_NR_ESPIS to more appropriate VGIC_DEF_MAX_SPI
>>>
>>> Will VGIC_DEF_MAX_ESPI be better? When other code refer to "SPI" it mean
>>> "common SPI" (less than 1022), while ESPI is used for extended SPI. So,
>>> naturally it feels that VGIC_DEF_MAX_SPI should be equal to 1022...
>>>
>>
>> Yes, I agree with that - VGIC_DEF_MAX_ESPI sounds more appropriate in
>> this case. I will rename it in V4.
>>
>>>> - 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
>>>> ---
>>>>    xen/arch/arm/dom0less-build.c   | 12 ++++++++++++
>>>>    xen/arch/arm/domain_build.c     | 11 +++++++++++
>>>>    xen/arch/arm/include/asm/vgic.h | 14 ++++++++++++++
>>>>    3 files changed, 37 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less- 
>>>> build.c
>>>> index 69b9ea22ce..f4f9077db5 100644
>>>> --- a/xen/arch/arm/dom0less-build.c
>>>> +++ b/xen/arch/arm/dom0less-build.c
>>>> @@ -286,6 +286,18 @@ 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;
>>>> +#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 overwrite the nr_spis with the 
>>>> maximum
>>>> +         * available INTID from eSPI range. In that case, the 
>>>> number of
>>>> +         * regular SPIs will be adjusted to the maximum value 
>>>> during vGIC
>>>> +         * initialization.
>>>> +         */
>>>> +        if ( gic_number_espis() > 0 )
>>>> +            d_cfg->arch.nr_spis = VGIC_DEF_MAX_SPI;
>>>> +#endif
>>>>            /*
>>>>             * 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..148a8bdb60 100644
>>>> --- a/xen/arch/arm/domain_build.c
>>>> +++ b/xen/arch/arm/domain_build.c
>>>> @@ -2055,6 +2055,17 @@ 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;
>>>> +#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
>>>> +     * overwrite the nr_spis with the maximum available INTID from 
>>>> eSPI range.
>>>> +     * In that case, the number of regular SPIs will be adjusted to 
>>>> the maximum
>>>> +     * value during vGIC initialization.
>>>> +     */
>>>> +    if ( gic_number_espis() > 0 )
>>>> +        dom0_cfg.arch.nr_spis = VGIC_DEF_MAX_SPI;
>>>> +#endif
> 
> So we have almost (?) identical code and big comments in both 
> create_dom0() and arch_create_domUs().
> 
> I was thinking, wouldn't it be better if we put this into a helper? This 
> way, I think, we will reduce the number of GICv3-specific #ifdef-s in 
> common domain construction code and will not need to worry about keeping 
> these places in sync when updating the check/comment.
> 
> Something like below to be applied on top of your patch (not build-tested):
> 
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index f4f9077db5..02d5559102 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -285,19 +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;
> -#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 overwrite the nr_spis with the maximum
> -         * available INTID from eSPI range. In that case, the number of
> -         * regular SPIs will be adjusted to the maximum value during vGIC
> -         * initialization.
> -         */
> -        if ( gic_number_espis() > 0 )
> -            d_cfg->arch.nr_spis = VGIC_DEF_MAX_SPI;
> -#endif
> +        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 148a8bdb60..39eea0be00 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2054,18 +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;
> -#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
> -     * overwrite the nr_spis with the maximum available INTID from eSPI 
> range.
> -     * In that case, the number of regular SPIs will be adjusted to the 
> maximum
> -     * value during vGIC initialization.
> -     */
> -    if ( gic_number_espis() > 0 )
> -        dom0_cfg.arch.nr_spis = VGIC_DEF_MAX_SPI;
> -#endif
> +    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 0bb025f5d5..0350122a74 100644
> --- a/xen/arch/arm/include/asm/vgic.h
> +++ b/xen/arch/arm/include/asm/vgic.h
> @@ -367,6 +367,24 @@ extern void vgic_check_inflight_irqs_pending(struct 
> vcpu *v,
>                             min(gic_number_espis(), 1024U) - 32)
>   #endif
> 
> +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 overwrite the nr_spis with the maximum
> +     * available INTID from eSPI range. In that case, the number of
> +     * regular SPIs will be adjusted to the maximum value during vGIC
> +     * initialization.
> +     */
> +    if ( gic_number_espis() > 0 )
> +        return VGIC_DEF_MAX_SPI;
> +#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)
> 
> ******
> 
> Also, if VGIC_DEF_MAX_SPI (or whatever name you agreed on) is not 
> supposed to be used outside of vgic.h, I would even consider dropping it
> and using "ESPI_BASE_INTID + min(gic_number_espis(), 1024U) - 32" (to 
> reduce one more #ifdef in the header).

Yes, I agree with you - it looks much better. I will check your changes, 
and if everything is okay, I will apply them to V4.

> 
> 
> P.S. I might be wrong, but it feels to me, that description for 
> "nr_spis" in docs/man/xl.cfg.5.pod.in needs a update within your series 
> (it mentions that "Max is 960 SPIs").


Best regards,
Leonid