[PATCH] tools/arm: Fix nr_spis handling v2

[PATCH] 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 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>
---
 tools/libs/light/libxl_arm.c     | 7 ++++---
 tools/libs/light/libxl_types.idl | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 2d895408cac3..5bb5bac61def 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,14 @@ 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) {
+    /* We use UINT32_MAX to denote if a user provided a value or not */
+    if ((cfg_nr_spis != UINT32_MAX) && (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 = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
+    config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : 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..129c00ce929c 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': 'UINT32_MAX'}),
                               ])),
     ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
                               ])),
-- 
2.25.1

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

[Luca Fancellu]

Hi Michal,

> On 18 Mar 2025, at 09: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 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>
> ---

I’ve tested this fix using our test that mounts a virtio mmio disk on the domain,
everything works fine.

Tested-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

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

[Alejandro Vallejo]

On Tue Mar 18, 2025 at 9:00 AM GMT, Michal Orzel 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 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>
> ---
>  tools/libs/light/libxl_arm.c     | 7 ++++---
>  tools/libs/light/libxl_types.idl | 2 +-
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 2d895408cac3..5bb5bac61def 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,14 @@ 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) {
> +    /* We use UINT32_MAX to denote if a user provided a value or not */
> +    if ((cfg_nr_spis != UINT32_MAX) && (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 = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
> +    config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : 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..129c00ce929c 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': 'UINT32_MAX'}),
>                                ])),
>      ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>                                ])),

Doesn't this regenerate the golang bindings?

Cheers,
Alejandro

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

[Andrew Cooper]

On 19/03/2025 2:01 pm, Alejandro Vallejo wrote:
> On Tue Mar 18, 2025 at 9:00 AM GMT, Michal Orzel 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 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>
>> ---
>>  tools/libs/light/libxl_arm.c     | 7 ++++---
>>  tools/libs/light/libxl_types.idl | 2 +-
>>  2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index 2d895408cac3..5bb5bac61def 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,14 @@ 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) {
>> +    /* We use UINT32_MAX to denote if a user provided a value or not */
>> +    if ((cfg_nr_spis != UINT32_MAX) && (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 = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
>> +    config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : 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..129c00ce929c 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': 'UINT32_MAX'}),
>>                                ])),
>>      ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>>                                ])),
> Doesn't this regenerate the golang bindings?

You need a build environment with golang for that to happen.  It's easy
to miss.

In this case, best to ask the committer to regen.

~Andrew

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

[Orzel, Michal]

On 19/03/2025 15:05, Andrew Cooper wrote:
> 
> 
> On 19/03/2025 2:01 pm, Alejandro Vallejo wrote:
>> On Tue Mar 18, 2025 at 9:00 AM GMT, Michal Orzel 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 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>
>>> ---
>>>  tools/libs/light/libxl_arm.c     | 7 ++++---
>>>  tools/libs/light/libxl_types.idl | 2 +-
>>>  2 files changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>>> index 2d895408cac3..5bb5bac61def 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,14 @@ 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) {
>>> +    /* We use UINT32_MAX to denote if a user provided a value or not */
>>> +    if ((cfg_nr_spis != UINT32_MAX) && (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 = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
>>> +    config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : 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..129c00ce929c 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': 'UINT32_MAX'}),
>>>                                ])),
>>>      ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>>>                                ])),
>> Doesn't this regenerate the golang bindings?
> 
> You need a build environment with golang for that to happen.  It's easy
> to miss.
> 
> In this case, best to ask the committer to regen.
This patch is already waiting for tools Ab, so maybe @Anthony could do that on
commit?

~Michal

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

[Alejandro Vallejo]

On Wed Mar 19, 2025 at 2:05 PM GMT, Andrew Cooper wrote:
> On 19/03/2025 2:01 pm, Alejandro Vallejo wrote:
> > On Tue Mar 18, 2025 at 9:00 AM GMT, Michal Orzel 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 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>
> >> ---
> >>  tools/libs/light/libxl_arm.c     | 7 ++++---
> >>  tools/libs/light/libxl_types.idl | 2 +-
> >>  2 files changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> >> index 2d895408cac3..5bb5bac61def 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,14 @@ 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) {
> >> +    /* We use UINT32_MAX to denote if a user provided a value or not */
> >> +    if ((cfg_nr_spis != UINT32_MAX) && (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 = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
> >> +    config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : 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..129c00ce929c 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': 'UINT32_MAX'}),
> >>                                ])),
> >>      ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
> >>                                ])),
> > Doesn't this regenerate the golang bindings?
>
> You need a build environment with golang for that to happen.  It's easy
> to miss.
>
> In this case, best to ask the committer to regen.
>
> ~Andrew

One more thing for the list of stuff I'd like to add to CI at some point.

Cheers,
Alejandro

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

[Anthony PERARD]

On Tue, Mar 18, 2025 at 10:00:13AM +0100, Michal Orzel 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 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>
> ---
>  tools/libs/light/libxl_arm.c     | 7 ++++---
>  tools/libs/light/libxl_types.idl | 2 +-
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 2d895408cac3..5bb5bac61def 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,14 @@ 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) {
> +    /* We use UINT32_MAX to denote if a user provided a value or not */
> +    if ((cfg_nr_spis != UINT32_MAX) && (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 = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
> +    config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : nr_spis;

Could you try to check only once if there is a user provided value for
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..129c00ce929c 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': 'UINT32_MAX'}),

Could you introduce something like LIBXL_NR_SPIS_DEFAULT in libxl.h
instead? (Like we have LIBXL_MAX_GRANT_DEFAULT or LIBXL_MEMKB_DEFAULT)

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

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

[Orzel, Michal]

On 20/03/2025 17:46, Anthony PERARD wrote:
> 
> 
> On Tue, Mar 18, 2025 at 10:00:13AM +0100, Michal Orzel 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 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>
>> ---
>>  tools/libs/light/libxl_arm.c     | 7 ++++---
>>  tools/libs/light/libxl_types.idl | 2 +-
>>  2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index 2d895408cac3..5bb5bac61def 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,14 @@ 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) {
>> +    /* We use UINT32_MAX to denote if a user provided a value or not */
>> +    if ((cfg_nr_spis != UINT32_MAX) && (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 = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
>> +    config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : nr_spis;
> 
> Could you try to check only once if there is a user provided value for
> nr_spis?
Sure, but that's one extra variable or more if/else blocks.

> 
>>      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..129c00ce929c 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': 'UINT32_MAX'}),
> 
> Could you introduce something like LIBXL_NR_SPIS_DEFAULT in libxl.h
> instead? (Like we have LIBXL_MAX_GRANT_DEFAULT or LIBXL_MEMKB_DEFAULT)
Sure, no problem.

~Michal

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

[Orzel, Michal]

On 19/03/2025 15:01, Alejandro Vallejo wrote:
> 
> 
> On Tue Mar 18, 2025 at 9:00 AM GMT, Michal Orzel 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 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>
>> ---
>>  tools/libs/light/libxl_arm.c     | 7 ++++---
>>  tools/libs/light/libxl_types.idl | 2 +-
>>  2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index 2d895408cac3..5bb5bac61def 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,14 @@ 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) {
>> +    /* We use UINT32_MAX to denote if a user provided a value or not */
>> +    if ((cfg_nr_spis != UINT32_MAX) && (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 = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
>> +    config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : 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..129c00ce929c 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': 'UINT32_MAX'}),
>>                                ])),
>>      ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>>                                ])),
> 
> Doesn't this regenerate the golang bindings?
FYI, it does not. The bindings are already there for NrSpis and default value is
does not result in a change (for verification I checked max_grant_frames that
uses LIBXL_MAX_GRANT_DEFAULT).

~Michal

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

[Alejandro Vallejo]

On Mon Mar 24, 2025 at 1:08 PM GMT, Michal Orzel wrote:
>
>
> On 19/03/2025 15:01, Alejandro Vallejo wrote:
> > 
> > Doesn't this regenerate the golang bindings?
> FYI, it does not. The bindings are already there for NrSpis and default value is
> does not result in a change (for verification I checked max_grant_frames that
> uses LIBXL_MAX_GRANT_DEFAULT).
>
> ~Michal

Oh, good then. Though it does raise the (completely separate) question of how
correct those bindings might be if they ignore the IDL's default values... :/

Cheers,
Alejandro

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

[Orzel, Michal]

On 24/03/2025 16:22, Alejandro Vallejo wrote:
> 
> 
> On Mon Mar 24, 2025 at 1:08 PM GMT, Michal Orzel wrote:
>>
>>
>> On 19/03/2025 15:01, Alejandro Vallejo wrote:
>>>
>>> Doesn't this regenerate the golang bindings?
>> FYI, it does not. The bindings are already there for NrSpis and default value is
>> does not result in a change (for verification I checked max_grant_frames that
>> uses LIBXL_MAX_GRANT_DEFAULT).
>>
>> ~Michal
> 
> Oh, good then. Though it does raise the (completely separate) question of how
> correct those bindings might be if they ignore the IDL's default values... :/
Why ignore. AFAICS libxl_domain_build_info_init is called there to grab default
values.

~Michal

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

[Alejandro Vallejo]

On Mon Mar 24, 2025 at 3:31 PM GMT, Michal Orzel wrote:
>
>
> On 24/03/2025 16:22, Alejandro Vallejo wrote:
> > 
> > 
> > On Mon Mar 24, 2025 at 1:08 PM GMT, Michal Orzel wrote:
> >>
> >>
> >> On 19/03/2025 15:01, Alejandro Vallejo wrote:
> >>>
> >>> Doesn't this regenerate the golang bindings?
> >> FYI, it does not. The bindings are already there for NrSpis and default value is
> >> does not result in a change (for verification I checked max_grant_frames that
> >> uses LIBXL_MAX_GRANT_DEFAULT).
> >>
> >> ~Michal
> > 
> > Oh, good then. Though it does raise the (completely separate) question of how
> > correct those bindings might be if they ignore the IDL's default values... :/
> Why ignore. AFAICS libxl_domain_build_info_init is called there to grab default
> values.
>
> ~Michal

Oh, so the default populator is itself autogenerated C with a golang binding
rather than native golang. Fair enough. The more you learn.

Cheers,
Alejandro