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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Michal,

> On 11 Mar 2025, at 10:04, Michal Orzel <michal.orzel@xxxxxxx> 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@xxxxxxx>

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

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
>