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

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

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@xxxxxxx>
> >> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> >> ---
> >>  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