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

["Anthony PERARD" <anthony.perard@vates.tech>]

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