[PATCH v2] tools/arm: Fix nr_spis handling v2

[PATCH v2] tools/arm: Fix nr_spis handling v2

[Michal Orzel]

We are missing a way to detect whether a user provided a value for
nr_spis equal to 0 or did not provide any value (default is also 0) which
can cause issues when calculated nr_spis is > 0 and the value from domain
config is 0. Fix it by setting default value for nr_spis to newly added
LIBXL_NR_SPIS_DEFAULT i.e. UINT32_MAX (max supported nr of SPIs is 960
anyway).

Fixes: 55d62b8d4636 ("tools/arm: Reject configuration with incorrect nr_spis value")
Reported-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
Changes in v2:
 - add LIBXL_NR_SPIS_DEFAULT, rearrange checks
---
 tools/include/libxl.h            |  1 +
 tools/libs/light/libxl_arm.c     | 17 +++++++++++------
 tools/libs/light/libxl_types.idl |  2 +-
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index f8fe4afd7dca..b7ad7735ca4c 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -1579,6 +1579,7 @@ bool libxl_defbool_val(libxl_defbool db);
 
 const char *libxl_defbool_to_string(libxl_defbool b);
 
+#define LIBXL_NR_SPIS_DEFAULT (~(uint32_t)0)
 #define LIBXL_TIMER_MODE_DEFAULT -1
 #define LIBXL_MEMKB_DEFAULT ~0ULL
 
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 2d895408cac3..0adcaa373b54 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -84,7 +84,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
                                       libxl_domain_config *d_config,
                                       struct xen_domctl_createdomain *config)
 {
-    uint32_t nr_spis = 0;
+    uint32_t nr_spis = 0, cfg_nr_spis = d_config->b_info.arch_arm.nr_spis;
     unsigned int i;
     uint32_t vuart_irq, virtio_irq = 0;
     bool vuart_enabled = false, virtio_enabled = false;
@@ -181,13 +181,18 @@ 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 (minimum required %u)\n",
-            nr_spis);
-        return ERROR_FAIL;
+    /* Check if a user provided a value or not */
+    if (cfg_nr_spis != LIBXL_NR_SPIS_DEFAULT) {
+        if (nr_spis > cfg_nr_spis) {
+            LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n",
+                nr_spis);
+            return ERROR_FAIL;
+        }
+        config->arch.nr_spis = cfg_nr_spis;
     }
+    else
+        config->arch.nr_spis = nr_spis;
 
-    config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
     LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
 
     switch (d_config->b_info.arch_arm.gic_version) {
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index bd4b8721ff19..9bb296993199 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -723,7 +723,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
                                ("vuart", libxl_vuart_type),
                                ("sve_vl", libxl_sve_type),
-                               ("nr_spis", uint32),
+                               ("nr_spis", uint32, {'init_val': 'LIBXL_NR_SPIS_DEFAULT'}),
                               ])),
     ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
                               ])),
-- 
2.25.1

Re: [PATCH v2] tools/arm: Fix nr_spis handling v2

[Anthony PERARD]

On Tue, Mar 25, 2025 at 12:00:29PM +0100, Michal Orzel wrote:
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 2d895408cac3..0adcaa373b54 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -181,13 +181,18 @@ 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 (minimum required %u)\n",
> -            nr_spis);
> -        return ERROR_FAIL;
> +    /* Check if a user provided a value or not */
> +    if (cfg_nr_spis != LIBXL_NR_SPIS_DEFAULT) {
> +        if (nr_spis > cfg_nr_spis) {
> +            LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n",
> +                nr_spis);
> +            return ERROR_FAIL;
> +        }
> +        config->arch.nr_spis = cfg_nr_spis;
>      }
> +    else
> +        config->arch.nr_spis = nr_spis;

Just one small coding style issue: to avoid confusion, whenever one side
a of an if..else is using a block, both side should use a block. But
that can be fixed on commit:
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v2] tools/arm: Fix nr_spis handling v2

[Orzel, Michal]

On 26/03/2025 15:47, Anthony PERARD wrote:
> 
> 
> On Tue, Mar 25, 2025 at 12:00:29PM +0100, Michal Orzel wrote:
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index 2d895408cac3..0adcaa373b54 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -181,13 +181,18 @@ 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 (minimum required %u)\n",
>> -            nr_spis);
>> -        return ERROR_FAIL;
>> +    /* Check if a user provided a value or not */
>> +    if (cfg_nr_spis != LIBXL_NR_SPIS_DEFAULT) {
>> +        if (nr_spis > cfg_nr_spis) {
>> +            LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n",
>> +                nr_spis);
>> +            return ERROR_FAIL;
>> +        }
>> +        config->arch.nr_spis = cfg_nr_spis;
>>      }
>> +    else
>> +        config->arch.nr_spis = nr_spis;
> 
> Just one small coding style issue: to avoid confusion, whenever one side
> a of an if..else is using a block, both side should use a block. But
Oh, I should have checked with tools/libs/light/CODING_STYLE.

> that can be fixed on commit:
> Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
Fixing on commit would be great.

~Michal

Re: [PATCH v2] tools/arm: Fix nr_spis handling v2

[Bertrand Marquis]

Hi Michal,

> On 25 Mar 2025, at 12:00, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> We are missing a way to detect whether a user provided a value for
> nr_spis equal to 0 or did not provide any value (default is also 0) which
> can cause issues when calculated nr_spis is > 0 and the value from domain
> config is 0. Fix it by setting default value for nr_spis to newly added
> LIBXL_NR_SPIS_DEFAULT i.e. UINT32_MAX (max supported nr of SPIs is 960
> anyway).
> 
> Fixes: 55d62b8d4636 ("tools/arm: Reject configuration with incorrect nr_spis value")
> Reported-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Sounds good to me, so for Arm side of things:

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

Cheers
Bertrand

> ---
> Changes in v2:
> - add LIBXL_NR_SPIS_DEFAULT, rearrange checks
> ---
> tools/include/libxl.h            |  1 +
> tools/libs/light/libxl_arm.c     | 17 +++++++++++------
> tools/libs/light/libxl_types.idl |  2 +-
> 3 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index f8fe4afd7dca..b7ad7735ca4c 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -1579,6 +1579,7 @@ bool libxl_defbool_val(libxl_defbool db);
> 
> const char *libxl_defbool_to_string(libxl_defbool b);
> 
> +#define LIBXL_NR_SPIS_DEFAULT (~(uint32_t)0)
> #define LIBXL_TIMER_MODE_DEFAULT -1
> #define LIBXL_MEMKB_DEFAULT ~0ULL
> 
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 2d895408cac3..0adcaa373b54 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -84,7 +84,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>                                       libxl_domain_config *d_config,
>                                       struct xen_domctl_createdomain *config)
> {
> -    uint32_t nr_spis = 0;
> +    uint32_t nr_spis = 0, cfg_nr_spis = d_config->b_info.arch_arm.nr_spis;
>     unsigned int i;
>     uint32_t vuart_irq, virtio_irq = 0;
>     bool vuart_enabled = false, virtio_enabled = false;
> @@ -181,13 +181,18 @@ 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 (minimum required %u)\n",
> -            nr_spis);
> -        return ERROR_FAIL;
> +    /* Check if a user provided a value or not */
> +    if (cfg_nr_spis != LIBXL_NR_SPIS_DEFAULT) {
> +        if (nr_spis > cfg_nr_spis) {
> +            LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n",
> +                nr_spis);
> +            return ERROR_FAIL;
> +        }
> +        config->arch.nr_spis = cfg_nr_spis;
>     }
> +    else
> +        config->arch.nr_spis = nr_spis;
> 
> -    config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
>     LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
> 
>     switch (d_config->b_info.arch_arm.gic_version) {
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index bd4b8721ff19..9bb296993199 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -723,7 +723,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>     ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>                                ("vuart", libxl_vuart_type),
>                                ("sve_vl", libxl_sve_type),
> -                               ("nr_spis", uint32),
> +                               ("nr_spis", uint32, {'init_val': 'LIBXL_NR_SPIS_DEFAULT'}),
>                               ])),
>     ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>                               ])),
> -- 
> 2.25.1
>