[PATCH 0/2] arm: better handling of nr_spis

[PATCH 0/2] arm: better handling of nr_spis

[Michal Orzel]

Refer:
https://lore.kernel.org/xen-devel/20250306220343.203047-6-jason.andryuk@amd.com/T/#mc15ab00940d5964b18b3d6092869dae6f85af1dc

Michal Orzel (2):
  xen/arm: Improve handling of nr_spis
  tools/arm: Reject configuration with incorrect nr_spis value

 docs/man/xl.cfg.5.pod.in        | 13 +++++--------
 tools/libs/light/libxl_arm.c    |  6 ++++++
 xen/arch/arm/dom0less-build.c   |  2 +-
 xen/arch/arm/domain_build.c     |  9 ++-------
 xen/arch/arm/gic.c              |  3 +++
 xen/arch/arm/include/asm/vgic.h |  3 +++
 6 files changed, 20 insertions(+), 16 deletions(-)

-- 
2.25.1

[PATCH 1/2] xen/arm: Improve handling of nr_spis

[Michal Orzel]

At the moment, we print a warning about max number of IRQs supported by
GIC bigger than vGIC only for hardware domain. This check is not hwdom
special, and should be made common. Also, in case of user not specifying
nr_spis for dom0less domUs, we should take into account max number of
IRQs supported by vGIC if it's smaller than for GIC.

Introduce VGIC_MAX_IRQS macro and use it instead of hardcoded 992 value.
Fix calculation of nr_spis for dom0less domUs and make the GIC/vGIC max
IRQs comparison common.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/arch/arm/dom0less-build.c   | 2 +-
 xen/arch/arm/domain_build.c     | 9 ++-------
 xen/arch/arm/gic.c              | 3 +++
 xen/arch/arm/include/asm/vgic.h | 3 +++
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 31f31c38da3f..9a84fee94119 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -1018,7 +1018,7 @@ void __init create_domUs(void)
         {
             int vpl011_virq = GUEST_VPL011_SPI;
 
-            d_cfg.arch.nr_spis = gic_number_lines() - 32;
+            d_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32;
 
             /*
              * 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 7cc141ef75e9..b99c4e3a69bf 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2371,13 +2371,8 @@ 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;
-    /*
-     * Xen vGIC supports a maximum of 992 interrupt lines.
-     * 32 are substracted to cover local IRQs.
-     */
-    dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
-    if ( gic_number_lines() > 992 )
-        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
+    /* 32 are substracted to cover local IRQs */
+    dom0_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32;
     dom0_cfg.arch.tee_type = tee_get_type();
     dom0_cfg.max_vcpus = dom0_max_vcpus();
 
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index acf61a4de373..e80fe0ca2421 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -251,6 +251,9 @@ void __init gic_init(void)
         panic("Failed to initialize the GIC drivers\n");
     /* Clear LR mask for cpu0 */
     clear_cpu_lr_mask();
+
+    if ( gic_number_lines() > VGIC_MAX_IRQS )
+        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded\n");
 }
 
 void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
index e309dca1ad01..c549e5840bfa 100644
--- a/xen/arch/arm/include/asm/vgic.h
+++ b/xen/arch/arm/include/asm/vgic.h
@@ -329,6 +329,9 @@ extern void vgic_check_inflight_irqs_pending(struct vcpu *v,
  */
 #define vgic_num_irqs(d)        ((d)->arch.vgic.nr_spis + 32)
 
+/* Maximum number of IRQs supported by vGIC */
+#define VGIC_MAX_IRQS 992U
+
 /*
  * Allocate a guest VIRQ
  *  - spi == 0 => allocate a PPI. It will be the same on every vCPU
-- 
2.25.1

[PATCH 2/2] tools/arm: Reject configuration with incorrect nr_spis value

[Michal Orzel]

If the calculated value for nr_spis by the toolstack is bigger than the
value provided by the user, we silently ignore the latter. This is not
consistent with the approach we have in Xen on Arm when we try to reject
incorrect configuration. Also, the documentation for nr_spis is
incorrect as it mentions 991 as the number of max SPIs, where it should
be 960 i.e. (1020 - 32) rounded down to the nearest multiple of 32.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 docs/man/xl.cfg.5.pod.in     | 13 +++++--------
 tools/libs/light/libxl_arm.c |  6 ++++++
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 8e1422104e50..7339c44efd54 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -3083,14 +3083,11 @@ interval of colors (such as "0-4").
 =item B<nr_spis="NR_SPIS">
 
 An optional integer parameter specifying the number of SPIs (Shared
-Peripheral Interrupts) to allocate for the domain. Max is 991 SPIs. If
-the value specified by the `nr_spis` parameter is smaller than the
-number of SPIs calculated by the toolstack based on the devices
-allocated for the domain, or the `nr_spis` parameter is not specified,
-the value calculated by the toolstack will be used for the domain.
-Otherwise, the value specified by the `nr_spis` parameter will be used.
-The number of SPIs should match the highest interrupt ID that will be
-assigned to the domain.
+Peripheral Interrupts) to allocate for the domain. Max is 960 SPIs. If
+the `nr_spis` parameter is not specified, the value calculated by the toolstack
+will be used for the domain. Otherwise, the value specified by the `nr_spis`
+parameter will be used. The number of SPIs should match the highest interrupt
+ID that will be assigned to the domain.
 
 =back
 
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 5a9db5e85f6f..ee9154298f2a 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -181,6 +181,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
 
     LOG(DEBUG, "Configure the domain");
 
+    if (nr_spis > d_config->b_info.arch_arm.nr_spis) {
+        LOG(ERROR, "Provided nr_spis value is too small (required %u)\n",
+            nr_spis);
+        return ERROR_FAIL;
+    }
+
     config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
     LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
 
-- 
2.25.1

Re: [PATCH 2/2] tools/arm: Reject configuration with incorrect nr_spis value

[Bertrand Marquis]

Hi Michal,

> On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> If the calculated value for nr_spis by the toolstack is bigger than the
> value provided by the user, we silently ignore the latter. This is not
> consistent with the approach we have in Xen on Arm when we try to reject
> incorrect configuration. Also, the documentation for nr_spis is
> incorrect as it mentions 991 as the number of max SPIs, where it should
> be 960 i.e. (1020 - 32) rounded down to the nearest multiple of 32.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

With a small NIT...

> ---
> docs/man/xl.cfg.5.pod.in     | 13 +++++--------
> tools/libs/light/libxl_arm.c |  6 ++++++
> 2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 8e1422104e50..7339c44efd54 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -3083,14 +3083,11 @@ interval of colors (such as "0-4").
> =item B<nr_spis="NR_SPIS">
> 
> An optional integer parameter specifying the number of SPIs (Shared
> -Peripheral Interrupts) to allocate for the domain. Max is 991 SPIs. If
> -the value specified by the `nr_spis` parameter is smaller than the
> -number of SPIs calculated by the toolstack based on the devices
> -allocated for the domain, or the `nr_spis` parameter is not specified,
> -the value calculated by the toolstack will be used for the domain.
> -Otherwise, the value specified by the `nr_spis` parameter will be used.
> -The number of SPIs should match the highest interrupt ID that will be
> -assigned to the domain.
> +Peripheral Interrupts) to allocate for the domain. Max is 960 SPIs. If
> +the `nr_spis` parameter is not specified, the value calculated by the toolstack
> +will be used for the domain. Otherwise, the value specified by the `nr_spis`
> +parameter will be used. The number of SPIs should match the highest interrupt
> +ID that will be assigned to the domain.
> 
> =back
> 
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 5a9db5e85f6f..ee9154298f2a 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -181,6 +181,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> 
>     LOG(DEBUG, "Configure the domain");
> 
> +    if (nr_spis > d_config->b_info.arch_arm.nr_spis) {
> +        LOG(ERROR, "Provided nr_spis value is too small (required %u)\n",

I would say "minimum required" here.

Cheers
Bertrand

> +            nr_spis);
> +        return ERROR_FAIL;
> +    }
> +
>     config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
>     LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
> 
> -- 
> 2.25.1
> 

Re: [PATCH 1/2] xen/arm: Improve handling of nr_spis

[Bertrand Marquis]

Hi Michal,

> On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> At the moment, we print a warning about max number of IRQs supported by
> GIC bigger than vGIC only for hardware domain. This check is not hwdom
> special, and should be made common. Also, in case of user not specifying
> nr_spis for dom0less domUs, we should take into account max number of
> IRQs supported by vGIC if it's smaller than for GIC.
> 
> Introduce VGIC_MAX_IRQS macro and use it instead of hardcoded 992 value.
> Fix calculation of nr_spis for dom0less domUs and make the GIC/vGIC max
> IRQs comparison common.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
> xen/arch/arm/dom0less-build.c   | 2 +-
> xen/arch/arm/domain_build.c     | 9 ++-------
> xen/arch/arm/gic.c              | 3 +++
> xen/arch/arm/include/asm/vgic.h | 3 +++
> 4 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 31f31c38da3f..9a84fee94119 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -1018,7 +1018,7 @@ void __init create_domUs(void)
>         {
>             int vpl011_virq = GUEST_VPL011_SPI;
> 
> -            d_cfg.arch.nr_spis = gic_number_lines() - 32;
> +            d_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32;

I would suggest to introduce a static inline gic_nr_spis in a gic header ...

> 
>             /*
>              * 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 7cc141ef75e9..b99c4e3a69bf 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2371,13 +2371,8 @@ 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;
> -    /*
> -     * Xen vGIC supports a maximum of 992 interrupt lines.
> -     * 32 are substracted to cover local IRQs.
> -     */
> -    dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
> -    if ( gic_number_lines() > 992 )
> -        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
> +    /* 32 are substracted to cover local IRQs */
> +    dom0_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32;

and reuse it here to make sure the value used is always the same.

>     dom0_cfg.arch.tee_type = tee_get_type();
>     dom0_cfg.max_vcpus = dom0_max_vcpus();
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index acf61a4de373..e80fe0ca2421 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -251,6 +251,9 @@ void __init gic_init(void)
>         panic("Failed to initialize the GIC drivers\n");
>     /* Clear LR mask for cpu0 */
>     clear_cpu_lr_mask();
> +
> +    if ( gic_number_lines() > VGIC_MAX_IRQS )
> +        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded\n");

I am a bit unsure with this one.
If this is the case, it means any gicv2 or gicv3 init detected an impossible value and
any usage of gic_number_lines would be using an impossible value.

Shouldn't this somehow result in a panic as the gic detection was wrong ?
Do you think we can continue to work safely if this value is over VGIC_MAX_IRQS.
There are other places using gic_number_lines like in irq.c.

Cheers
Bertrand


> }
> 
> void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
> index e309dca1ad01..c549e5840bfa 100644
> --- a/xen/arch/arm/include/asm/vgic.h
> +++ b/xen/arch/arm/include/asm/vgic.h
> @@ -329,6 +329,9 @@ extern void vgic_check_inflight_irqs_pending(struct vcpu *v,
>  */
> #define vgic_num_irqs(d)        ((d)->arch.vgic.nr_spis + 32)
> 
> +/* Maximum number of IRQs supported by vGIC */
> +#define VGIC_MAX_IRQS 992U
> +
> /*
>  * Allocate a guest VIRQ
>  *  - spi == 0 => allocate a PPI. It will be the same on every vCPU
> -- 
> 2.25.1
> 

Re: [PATCH 1/2] xen/arm: Improve handling of nr_spis

[Orzel, Michal]

On 11/03/2025 10:30, Bertrand Marquis wrote:
> 
> 
> Hi Michal,
> 
>> On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@amd.com> wrote:
>>
>> At the moment, we print a warning about max number of IRQs supported by
>> GIC bigger than vGIC only for hardware domain. This check is not hwdom
>> special, and should be made common. Also, in case of user not specifying
>> nr_spis for dom0less domUs, we should take into account max number of
>> IRQs supported by vGIC if it's smaller than for GIC.
>>
>> Introduce VGIC_MAX_IRQS macro and use it instead of hardcoded 992 value.
>> Fix calculation of nr_spis for dom0less domUs and make the GIC/vGIC max
>> IRQs comparison common.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>> xen/arch/arm/dom0less-build.c   | 2 +-
>> xen/arch/arm/domain_build.c     | 9 ++-------
>> xen/arch/arm/gic.c              | 3 +++
>> xen/arch/arm/include/asm/vgic.h | 3 +++
>> 4 files changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
>> index 31f31c38da3f..9a84fee94119 100644
>> --- a/xen/arch/arm/dom0less-build.c
>> +++ b/xen/arch/arm/dom0less-build.c
>> @@ -1018,7 +1018,7 @@ void __init create_domUs(void)
>>         {
>>             int vpl011_virq = GUEST_VPL011_SPI;
>>
>> -            d_cfg.arch.nr_spis = gic_number_lines() - 32;
>> +            d_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32;
> 
> I would suggest to introduce a static inline gic_nr_spis in a gic header ...
Why GIC and not vGIC? This is domain's nr_spis, so vGIC.
But then, why static inline if the value does not change and is domain agnostic?
I struggle to find a good name for this macro. Maybe (in vgic.h):
#define vgic_def_nr_spis (min(gic_number_lines(), VGIC_MAX_IRQS) - 32)
to denote default nr_spis if not set by the user?

> 
>>
>>             /*
>>              * 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 7cc141ef75e9..b99c4e3a69bf 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -2371,13 +2371,8 @@ 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;
>> -    /*
>> -     * Xen vGIC supports a maximum of 992 interrupt lines.
>> -     * 32 are substracted to cover local IRQs.
>> -     */
>> -    dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
>> -    if ( gic_number_lines() > 992 )
>> -        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
>> +    /* 32 are substracted to cover local IRQs */
>> +    dom0_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32;
> 
> and reuse it here to make sure the value used is always the same.
> 
>>     dom0_cfg.arch.tee_type = tee_get_type();
>>     dom0_cfg.max_vcpus = dom0_max_vcpus();
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index acf61a4de373..e80fe0ca2421 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -251,6 +251,9 @@ void __init gic_init(void)
>>         panic("Failed to initialize the GIC drivers\n");
>>     /* Clear LR mask for cpu0 */
>>     clear_cpu_lr_mask();
>> +
>> +    if ( gic_number_lines() > VGIC_MAX_IRQS )
>> +        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded\n");
> 
> I am a bit unsure with this one.
> If this is the case, it means any gicv2 or gicv3 init detected an impossible value and
> any usage of gic_number_lines would be using an impossible value.
Why impossible? GIC can support up to 1020 IRQs. Our vGIC can support up to 992
IRQs.

> 
> Shouldn't this somehow result in a panic as the gic detection was wrong ?
> Do you think we can continue to work safely if this value is over VGIC_MAX_IRQS.
> There are other places using gic_number_lines like in irq.c.
I can add a panic, sure. This would be a change in behavior because we used this
check for hwdom unconditionally. I'd need others opinion for this one.

~Michal

Re: [PATCH 1/2] xen/arm: Improve handling of nr_spis

[Bertrand Marquis]

> On 11 Mar 2025, at 10:59, Orzel, Michal <Michal.Orzel@amd.com> wrote:
> 
> 
> 
> On 11/03/2025 10:30, Bertrand Marquis wrote:
>> 
>> 
>> Hi Michal,
>> 
>>> On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@amd.com> wrote:
>>> 
>>> At the moment, we print a warning about max number of IRQs supported by
>>> GIC bigger than vGIC only for hardware domain. This check is not hwdom
>>> special, and should be made common. Also, in case of user not specifying
>>> nr_spis for dom0less domUs, we should take into account max number of
>>> IRQs supported by vGIC if it's smaller than for GIC.
>>> 
>>> Introduce VGIC_MAX_IRQS macro and use it instead of hardcoded 992 value.
>>> Fix calculation of nr_spis for dom0less domUs and make the GIC/vGIC max
>>> IRQs comparison common.
>>> 
>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>> ---
>>> xen/arch/arm/dom0less-build.c   | 2 +-
>>> xen/arch/arm/domain_build.c     | 9 ++-------
>>> xen/arch/arm/gic.c              | 3 +++
>>> xen/arch/arm/include/asm/vgic.h | 3 +++
>>> 4 files changed, 9 insertions(+), 8 deletions(-)
>>> 
>>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
>>> index 31f31c38da3f..9a84fee94119 100644
>>> --- a/xen/arch/arm/dom0less-build.c
>>> +++ b/xen/arch/arm/dom0less-build.c
>>> @@ -1018,7 +1018,7 @@ void __init create_domUs(void)
>>>        {
>>>            int vpl011_virq = GUEST_VPL011_SPI;
>>> 
>>> -            d_cfg.arch.nr_spis = gic_number_lines() - 32;
>>> +            d_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32;
>> 
>> I would suggest to introduce a static inline gic_nr_spis in a gic header ...
> Why GIC and not vGIC? This is domain's nr_spis, so vGIC.

yes vGIC sorry.

> But then, why static inline if the value does not change and is domain agnostic?
> I struggle to find a good name for this macro. Maybe (in vgic.h):
> #define vgic_def_nr_spis (min(gic_number_lines(), VGIC_MAX_IRQS) - 32)
> to denote default nr_spis if not set by the user?

Yes that would work. My point is to prevent to have 2 definitions in 2 different
source file and a risk to forget to update one and not the other (let say if some
day we change 32 in 64).

> 
>> 
>>> 
>>>            /*
>>>             * 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 7cc141ef75e9..b99c4e3a69bf 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -2371,13 +2371,8 @@ 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;
>>> -    /*
>>> -     * Xen vGIC supports a maximum of 992 interrupt lines.
>>> -     * 32 are substracted to cover local IRQs.
>>> -     */
>>> -    dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
>>> -    if ( gic_number_lines() > 992 )
>>> -        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
>>> +    /* 32 are substracted to cover local IRQs */
>>> +    dom0_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32;
>> 
>> and reuse it here to make sure the value used is always the same.
>> 
>>>    dom0_cfg.arch.tee_type = tee_get_type();
>>>    dom0_cfg.max_vcpus = dom0_max_vcpus();
>>> 
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index acf61a4de373..e80fe0ca2421 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -251,6 +251,9 @@ void __init gic_init(void)
>>>        panic("Failed to initialize the GIC drivers\n");
>>>    /* Clear LR mask for cpu0 */
>>>    clear_cpu_lr_mask();
>>> +
>>> +    if ( gic_number_lines() > VGIC_MAX_IRQS )
>>> +        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded\n");
>> 
>> I am a bit unsure with this one.
>> If this is the case, it means any gicv2 or gicv3 init detected an impossible value and
>> any usage of gic_number_lines would be using an impossible value.
> Why impossible? GIC can support up to 1020 IRQs. Our vGIC can support up to 992
> IRQs.

Maybe unsupported is a better wording, my point is that it could lead to non working system
if say something uses irq 1000.

> 
>> 
>> Shouldn't this somehow result in a panic as the gic detection was wrong ?
>> Do you think we can continue to work safely if this value is over VGIC_MAX_IRQS.
>> There are other places using gic_number_lines like in irq.c.
> I can add a panic, sure. This would be a change in behavior because we used this
> check for hwdom unconditionally. I'd need others opinion for this one.

ok.

Cheers
Bertrand

> 
> ~Michal

Re: [PATCH 1/2] xen/arm: Improve handling of nr_spis

[Orzel, Michal]

On 11/03/2025 11:12, Bertrand Marquis wrote:
> 
> 
>> On 11 Mar 2025, at 10:59, Orzel, Michal <Michal.Orzel@amd.com> wrote:
>>
>>
>>
>> On 11/03/2025 10:30, Bertrand Marquis wrote:
>>>
>>>
>>> Hi Michal,
>>>
>>>> On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>
>>>> At the moment, we print a warning about max number of IRQs supported by
>>>> GIC bigger than vGIC only for hardware domain. This check is not hwdom
>>>> special, and should be made common. Also, in case of user not specifying
>>>> nr_spis for dom0less domUs, we should take into account max number of
>>>> IRQs supported by vGIC if it's smaller than for GIC.
>>>>
>>>> Introduce VGIC_MAX_IRQS macro and use it instead of hardcoded 992 value.
>>>> Fix calculation of nr_spis for dom0less domUs and make the GIC/vGIC max
>>>> IRQs comparison common.
>>>>
>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>> ---
>>>> xen/arch/arm/dom0less-build.c   | 2 +-
>>>> xen/arch/arm/domain_build.c     | 9 ++-------
>>>> xen/arch/arm/gic.c              | 3 +++
>>>> xen/arch/arm/include/asm/vgic.h | 3 +++
>>>> 4 files changed, 9 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
>>>> index 31f31c38da3f..9a84fee94119 100644
>>>> --- a/xen/arch/arm/dom0less-build.c
>>>> +++ b/xen/arch/arm/dom0less-build.c
>>>> @@ -1018,7 +1018,7 @@ void __init create_domUs(void)
>>>>        {
>>>>            int vpl011_virq = GUEST_VPL011_SPI;
>>>>
>>>> -            d_cfg.arch.nr_spis = gic_number_lines() - 32;
>>>> +            d_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32;
>>>
>>> I would suggest to introduce a static inline gic_nr_spis in a gic header ...
>> Why GIC and not vGIC? This is domain's nr_spis, so vGIC.
> 
> yes vGIC sorry.
> 
>> But then, why static inline if the value does not change and is domain agnostic?
>> I struggle to find a good name for this macro. Maybe (in vgic.h):
>> #define vgic_def_nr_spis (min(gic_number_lines(), VGIC_MAX_IRQS) - 32)
>> to denote default nr_spis if not set by the user?
> 
> Yes that would work. My point is to prevent to have 2 definitions in 2 different
> source file and a risk to forget to update one and not the other (let say if some
> day we change 32 in 64).
> 
>>
>>>
>>>>
>>>>            /*
>>>>             * 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 7cc141ef75e9..b99c4e3a69bf 100644
>>>> --- a/xen/arch/arm/domain_build.c
>>>> +++ b/xen/arch/arm/domain_build.c
>>>> @@ -2371,13 +2371,8 @@ 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;
>>>> -    /*
>>>> -     * Xen vGIC supports a maximum of 992 interrupt lines.
>>>> -     * 32 are substracted to cover local IRQs.
>>>> -     */
>>>> -    dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
>>>> -    if ( gic_number_lines() > 992 )
>>>> -        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
>>>> +    /* 32 are substracted to cover local IRQs */
>>>> +    dom0_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32;
>>>
>>> and reuse it here to make sure the value used is always the same.
>>>
>>>>    dom0_cfg.arch.tee_type = tee_get_type();
>>>>    dom0_cfg.max_vcpus = dom0_max_vcpus();
>>>>
>>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>>> index acf61a4de373..e80fe0ca2421 100644
>>>> --- a/xen/arch/arm/gic.c
>>>> +++ b/xen/arch/arm/gic.c
>>>> @@ -251,6 +251,9 @@ void __init gic_init(void)
>>>>        panic("Failed to initialize the GIC drivers\n");
>>>>    /* Clear LR mask for cpu0 */
>>>>    clear_cpu_lr_mask();
>>>> +
>>>> +    if ( gic_number_lines() > VGIC_MAX_IRQS )
>>>> +        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded\n");
>>>
>>> I am a bit unsure with this one.
>>> If this is the case, it means any gicv2 or gicv3 init detected an impossible value and
>>> any usage of gic_number_lines would be using an impossible value.
>> Why impossible? GIC can support up to 1020 IRQs. Our vGIC can support up to 992
>> IRQs.
> 
> Maybe unsupported is a better wording, my point is that it could lead to non working system
> if say something uses irq 1000.
Actually, I took a look at the code and I don't think we should panic (i.e. we
should keep things as they are today). In your example, if something uses IRQ >
VGIC_MAX_IRQS that is bigger than gic_number_lines(), then we will receive error
when mapping this IRQ to guest (but you don't have to use such device and in the
future we may enable IRQ re-mapping). That's why in all the places related to
domains, we use vgic_num_irqs() and not gic_number_lines(). The latter is only
used for IRQs routed to Xen.

~Michal

Re: [PATCH 1/2] xen/arm: Improve handling of nr_spis

[Bertrand Marquis]

Hi Michal,

> On 11 Mar 2025, at 12:06, Orzel, Michal <michal.orzel@amd.com> wrote:
> 
> 
> 
> On 11/03/2025 11:12, Bertrand Marquis wrote:
>> 
>> 
>>> On 11 Mar 2025, at 10:59, Orzel, Michal <Michal.Orzel@amd.com> wrote:
>>> 
>>> 
>>> 
>>> On 11/03/2025 10:30, Bertrand Marquis wrote:
>>>> 
>>>> 
>>>> Hi Michal,
>>>> 
>>>>> On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>> 
>>>>> At the moment, we print a warning about max number of IRQs supported by
>>>>> GIC bigger than vGIC only for hardware domain. This check is not hwdom
>>>>> special, and should be made common. Also, in case of user not specifying
>>>>> nr_spis for dom0less domUs, we should take into account max number of
>>>>> IRQs supported by vGIC if it's smaller than for GIC.
>>>>> 
>>>>> Introduce VGIC_MAX_IRQS macro and use it instead of hardcoded 992 value.
>>>>> Fix calculation of nr_spis for dom0less domUs and make the GIC/vGIC max
>>>>> IRQs comparison common.
>>>>> 
>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>>> ---
>>>>> xen/arch/arm/dom0less-build.c   | 2 +-
>>>>> xen/arch/arm/domain_build.c     | 9 ++-------
>>>>> xen/arch/arm/gic.c              | 3 +++
>>>>> xen/arch/arm/include/asm/vgic.h | 3 +++
>>>>> 4 files changed, 9 insertions(+), 8 deletions(-)
>>>>> 
>>>>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
>>>>> index 31f31c38da3f..9a84fee94119 100644
>>>>> --- a/xen/arch/arm/dom0less-build.c
>>>>> +++ b/xen/arch/arm/dom0less-build.c
>>>>> @@ -1018,7 +1018,7 @@ void __init create_domUs(void)
>>>>>       {
>>>>>           int vpl011_virq = GUEST_VPL011_SPI;
>>>>> 
>>>>> -            d_cfg.arch.nr_spis = gic_number_lines() - 32;
>>>>> +            d_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32;
>>>> 
>>>> I would suggest to introduce a static inline gic_nr_spis in a gic header ...
>>> Why GIC and not vGIC? This is domain's nr_spis, so vGIC.
>> 
>> yes vGIC sorry.
>> 
>>> But then, why static inline if the value does not change and is domain agnostic?
>>> I struggle to find a good name for this macro. Maybe (in vgic.h):
>>> #define vgic_def_nr_spis (min(gic_number_lines(), VGIC_MAX_IRQS) - 32)
>>> to denote default nr_spis if not set by the user?
>> 
>> Yes that would work. My point is to prevent to have 2 definitions in 2 different
>> source file and a risk to forget to update one and not the other (let say if some
>> day we change 32 in 64).
>> 
>>> 
>>>> 
>>>>> 
>>>>>           /*
>>>>>            * 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 7cc141ef75e9..b99c4e3a69bf 100644
>>>>> --- a/xen/arch/arm/domain_build.c
>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>> @@ -2371,13 +2371,8 @@ 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;
>>>>> -    /*
>>>>> -     * Xen vGIC supports a maximum of 992 interrupt lines.
>>>>> -     * 32 are substracted to cover local IRQs.
>>>>> -     */
>>>>> -    dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
>>>>> -    if ( gic_number_lines() > 992 )
>>>>> -        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
>>>>> +    /* 32 are substracted to cover local IRQs */
>>>>> +    dom0_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32;
>>>> 
>>>> and reuse it here to make sure the value used is always the same.
>>>> 
>>>>>   dom0_cfg.arch.tee_type = tee_get_type();
>>>>>   dom0_cfg.max_vcpus = dom0_max_vcpus();
>>>>> 
>>>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>>>> index acf61a4de373..e80fe0ca2421 100644
>>>>> --- a/xen/arch/arm/gic.c
>>>>> +++ b/xen/arch/arm/gic.c
>>>>> @@ -251,6 +251,9 @@ void __init gic_init(void)
>>>>>       panic("Failed to initialize the GIC drivers\n");
>>>>>   /* Clear LR mask for cpu0 */
>>>>>   clear_cpu_lr_mask();
>>>>> +
>>>>> +    if ( gic_number_lines() > VGIC_MAX_IRQS )
>>>>> +        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded\n");
>>>> 
>>>> I am a bit unsure with this one.
>>>> If this is the case, it means any gicv2 or gicv3 init detected an impossible value and
>>>> any usage of gic_number_lines would be using an impossible value.
>>> Why impossible? GIC can support up to 1020 IRQs. Our vGIC can support up to 992
>>> IRQs.
>> 
>> Maybe unsupported is a better wording, my point is that it could lead to non working system
>> if say something uses irq 1000.
> Actually, I took a look at the code and I don't think we should panic (i.e. we
> should keep things as they are today). In your example, if something uses IRQ >
> VGIC_MAX_IRQS that is bigger than gic_number_lines(), then we will receive error
> when mapping this IRQ to guest (but you don't have to use such device and in the
> future we may enable IRQ re-mapping). That's why in all the places related to
> domains, we use vgic_num_irqs() and not gic_number_lines(). The latter is only
> used for IRQs routed to Xen.

So you will get an error later such an IRQ is mapped to a guest. Tracking why this is might not
be easy.
Anyway this is a hardware value that the user has no power on so if we panic it would mean
Xen would never run on the given hardware without any chance for the user to workaround the
problem so maybe the Warning is the best we can do here.

Maybe others have an other idea here but i will not object to this.

Cheers
Bertrand

> 
> ~Michal
> 

Re: [PATCH 1/2] xen/arm: Improve handling of nr_spis

[Orzel, Michal]

On 11/03/2025 14:26, Bertrand Marquis wrote:
> 
> 
> Hi Michal,
> 
>> On 11 Mar 2025, at 12:06, Orzel, Michal <michal.orzel@amd.com> wrote:
>>
>>
>>
>> On 11/03/2025 11:12, Bertrand Marquis wrote:
>>>
>>>
>>>> On 11 Mar 2025, at 10:59, Orzel, Michal <Michal.Orzel@amd.com> wrote:
>>>>
>>>>
>>>>
>>>> On 11/03/2025 10:30, Bertrand Marquis wrote:
>>>>>
>>>>>
>>>>> Hi Michal,
>>>>>
>>>>>> On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>>>
>>>>>> At the moment, we print a warning about max number of IRQs supported by
>>>>>> GIC bigger than vGIC only for hardware domain. This check is not hwdom
>>>>>> special, and should be made common. Also, in case of user not specifying
>>>>>> nr_spis for dom0less domUs, we should take into account max number of
>>>>>> IRQs supported by vGIC if it's smaller than for GIC.
>>>>>>
>>>>>> Introduce VGIC_MAX_IRQS macro and use it instead of hardcoded 992 value.
>>>>>> Fix calculation of nr_spis for dom0less domUs and make the GIC/vGIC max
>>>>>> IRQs comparison common.
>>>>>>
>>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>>>> ---
>>>>>> xen/arch/arm/dom0less-build.c   | 2 +-
>>>>>> xen/arch/arm/domain_build.c     | 9 ++-------
>>>>>> xen/arch/arm/gic.c              | 3 +++
>>>>>> xen/arch/arm/include/asm/vgic.h | 3 +++
>>>>>> 4 files changed, 9 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
>>>>>> index 31f31c38da3f..9a84fee94119 100644
>>>>>> --- a/xen/arch/arm/dom0less-build.c
>>>>>> +++ b/xen/arch/arm/dom0less-build.c
>>>>>> @@ -1018,7 +1018,7 @@ void __init create_domUs(void)
>>>>>>       {
>>>>>>           int vpl011_virq = GUEST_VPL011_SPI;
>>>>>>
>>>>>> -            d_cfg.arch.nr_spis = gic_number_lines() - 32;
>>>>>> +            d_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32;
>>>>>
>>>>> I would suggest to introduce a static inline gic_nr_spis in a gic header ...
>>>> Why GIC and not vGIC? This is domain's nr_spis, so vGIC.
>>>
>>> yes vGIC sorry.
>>>
>>>> But then, why static inline if the value does not change and is domain agnostic?
>>>> I struggle to find a good name for this macro. Maybe (in vgic.h):
>>>> #define vgic_def_nr_spis (min(gic_number_lines(), VGIC_MAX_IRQS) - 32)
>>>> to denote default nr_spis if not set by the user?
>>>
>>> Yes that would work. My point is to prevent to have 2 definitions in 2 different
>>> source file and a risk to forget to update one and not the other (let say if some
>>> day we change 32 in 64).
>>>
>>>>
>>>>>
>>>>>>
>>>>>>           /*
>>>>>>            * 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 7cc141ef75e9..b99c4e3a69bf 100644
>>>>>> --- a/xen/arch/arm/domain_build.c
>>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>>> @@ -2371,13 +2371,8 @@ 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;
>>>>>> -    /*
>>>>>> -     * Xen vGIC supports a maximum of 992 interrupt lines.
>>>>>> -     * 32 are substracted to cover local IRQs.
>>>>>> -     */
>>>>>> -    dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
>>>>>> -    if ( gic_number_lines() > 992 )
>>>>>> -        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
>>>>>> +    /* 32 are substracted to cover local IRQs */
>>>>>> +    dom0_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32;
>>>>>
>>>>> and reuse it here to make sure the value used is always the same.
>>>>>
>>>>>>   dom0_cfg.arch.tee_type = tee_get_type();
>>>>>>   dom0_cfg.max_vcpus = dom0_max_vcpus();
>>>>>>
>>>>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>>>>> index acf61a4de373..e80fe0ca2421 100644
>>>>>> --- a/xen/arch/arm/gic.c
>>>>>> +++ b/xen/arch/arm/gic.c
>>>>>> @@ -251,6 +251,9 @@ void __init gic_init(void)
>>>>>>       panic("Failed to initialize the GIC drivers\n");
>>>>>>   /* Clear LR mask for cpu0 */
>>>>>>   clear_cpu_lr_mask();
>>>>>> +
>>>>>> +    if ( gic_number_lines() > VGIC_MAX_IRQS )
>>>>>> +        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded\n");
>>>>>
>>>>> I am a bit unsure with this one.
>>>>> If this is the case, it means any gicv2 or gicv3 init detected an impossible value and
>>>>> any usage of gic_number_lines would be using an impossible value.
>>>> Why impossible? GIC can support up to 1020 IRQs. Our vGIC can support up to 992
>>>> IRQs.
>>>
>>> Maybe unsupported is a better wording, my point is that it could lead to non working system
>>> if say something uses irq 1000.
>> Actually, I took a look at the code and I don't think we should panic (i.e. we
>> should keep things as they are today). In your example, if something uses IRQ >
>> VGIC_MAX_IRQS that is bigger than gic_number_lines(), then we will receive error
>> when mapping this IRQ to guest (but you don't have to use such device and in the
>> future we may enable IRQ re-mapping). That's why in all the places related to
>> domains, we use vgic_num_irqs() and not gic_number_lines(). The latter is only
>> used for IRQs routed to Xen.
> 
> So you will get an error later such an IRQ is mapped to a guest. Tracking why this is might not
> be easy.
I did check and we get a nice error that I find good enough, e.g.:
(XEN) the vIRQ number 260 is too high for domain 0 (max = 256)
(XEN) Unable to map IRQ260 to d0

~Michal

Re: [PATCH 1/2] xen/arm: Improve handling of nr_spis

[Bertrand Marquis]

> On 11 Mar 2025, at 14:33, Orzel, Michal <michal.orzel@amd.com> wrote:
> 
> 
> 
> On 11/03/2025 14:26, Bertrand Marquis wrote:
>> 
>> 
>> Hi Michal,
>> 
>>> On 11 Mar 2025, at 12:06, Orzel, Michal <michal.orzel@amd.com> wrote:
>>> 
>>> 
>>> 
>>> On 11/03/2025 11:12, Bertrand Marquis wrote:
>>>> 
>>>> 
>>>>> On 11 Mar 2025, at 10:59, Orzel, Michal <Michal.Orzel@amd.com> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 11/03/2025 10:30, Bertrand Marquis wrote:
>>>>>> 
>>>>>> 
>>>>>> Hi Michal,
>>>>>> 
>>>>>>> On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>>>> 
>>>>>>> At the moment, we print a warning about max number of IRQs supported by
>>>>>>> GIC bigger than vGIC only for hardware domain. This check is not hwdom
>>>>>>> special, and should be made common. Also, in case of user not specifying
>>>>>>> nr_spis for dom0less domUs, we should take into account max number of
>>>>>>> IRQs supported by vGIC if it's smaller than for GIC.
>>>>>>> 
>>>>>>> Introduce VGIC_MAX_IRQS macro and use it instead of hardcoded 992 value.
>>>>>>> Fix calculation of nr_spis for dom0less domUs and make the GIC/vGIC max
>>>>>>> IRQs comparison common.
>>>>>>> 
>>>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>>>>> ---
>>>>>>> xen/arch/arm/dom0less-build.c   | 2 +-
>>>>>>> xen/arch/arm/domain_build.c     | 9 ++-------
>>>>>>> xen/arch/arm/gic.c              | 3 +++
>>>>>>> xen/arch/arm/include/asm/vgic.h | 3 +++
>>>>>>> 4 files changed, 9 insertions(+), 8 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
>>>>>>> index 31f31c38da3f..9a84fee94119 100644
>>>>>>> --- a/xen/arch/arm/dom0less-build.c
>>>>>>> +++ b/xen/arch/arm/dom0less-build.c
>>>>>>> @@ -1018,7 +1018,7 @@ void __init create_domUs(void)
>>>>>>>      {
>>>>>>>          int vpl011_virq = GUEST_VPL011_SPI;
>>>>>>> 
>>>>>>> -            d_cfg.arch.nr_spis = gic_number_lines() - 32;
>>>>>>> +            d_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32;
>>>>>> 
>>>>>> I would suggest to introduce a static inline gic_nr_spis in a gic header ...
>>>>> Why GIC and not vGIC? This is domain's nr_spis, so vGIC.
>>>> 
>>>> yes vGIC sorry.
>>>> 
>>>>> But then, why static inline if the value does not change and is domain agnostic?
>>>>> I struggle to find a good name for this macro. Maybe (in vgic.h):
>>>>> #define vgic_def_nr_spis (min(gic_number_lines(), VGIC_MAX_IRQS) - 32)
>>>>> to denote default nr_spis if not set by the user?
>>>> 
>>>> Yes that would work. My point is to prevent to have 2 definitions in 2 different
>>>> source file and a risk to forget to update one and not the other (let say if some
>>>> day we change 32 in 64).
>>>> 
>>>>> 
>>>>>> 
>>>>>>> 
>>>>>>>          /*
>>>>>>>           * 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 7cc141ef75e9..b99c4e3a69bf 100644
>>>>>>> --- a/xen/arch/arm/domain_build.c
>>>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>>>> @@ -2371,13 +2371,8 @@ 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;
>>>>>>> -    /*
>>>>>>> -     * Xen vGIC supports a maximum of 992 interrupt lines.
>>>>>>> -     * 32 are substracted to cover local IRQs.
>>>>>>> -     */
>>>>>>> -    dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
>>>>>>> -    if ( gic_number_lines() > 992 )
>>>>>>> -        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
>>>>>>> +    /* 32 are substracted to cover local IRQs */
>>>>>>> +    dom0_cfg.arch.nr_spis = min(gic_number_lines(), VGIC_MAX_IRQS) - 32;
>>>>>> 
>>>>>> and reuse it here to make sure the value used is always the same.
>>>>>> 
>>>>>>>  dom0_cfg.arch.tee_type = tee_get_type();
>>>>>>>  dom0_cfg.max_vcpus = dom0_max_vcpus();
>>>>>>> 
>>>>>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>>>>>> index acf61a4de373..e80fe0ca2421 100644
>>>>>>> --- a/xen/arch/arm/gic.c
>>>>>>> +++ b/xen/arch/arm/gic.c
>>>>>>> @@ -251,6 +251,9 @@ void __init gic_init(void)
>>>>>>>      panic("Failed to initialize the GIC drivers\n");
>>>>>>>  /* Clear LR mask for cpu0 */
>>>>>>>  clear_cpu_lr_mask();
>>>>>>> +
>>>>>>> +    if ( gic_number_lines() > VGIC_MAX_IRQS )
>>>>>>> +        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded\n");
>>>>>> 
>>>>>> I am a bit unsure with this one.
>>>>>> If this is the case, it means any gicv2 or gicv3 init detected an impossible value and
>>>>>> any usage of gic_number_lines would be using an impossible value.
>>>>> Why impossible? GIC can support up to 1020 IRQs. Our vGIC can support up to 992
>>>>> IRQs.
>>>> 
>>>> Maybe unsupported is a better wording, my point is that it could lead to non working system
>>>> if say something uses irq 1000.
>>> Actually, I took a look at the code and I don't think we should panic (i.e. we
>>> should keep things as they are today). In your example, if something uses IRQ >
>>> VGIC_MAX_IRQS that is bigger than gic_number_lines(), then we will receive error
>>> when mapping this IRQ to guest (but you don't have to use such device and in the
>>> future we may enable IRQ re-mapping). That's why in all the places related to
>>> domains, we use vgic_num_irqs() and not gic_number_lines(). The latter is only
>>> used for IRQs routed to Xen.
>> 
>> So you will get an error later such an IRQ is mapped to a guest. Tracking why this is might not
>> be easy.
> I did check and we get a nice error that I find good enough, e.g.:
> (XEN) the vIRQ number 260 is too high for domain 0 (max = 256)
> (XEN) Unable to map IRQ260 to d0

Agree this is enough. If the user does not get why it says 256 it will
be able to find the warning earlier in the logs.

Thanks for digging

Cheers
Bertrand

> 
> ~Michal