Re: [PATCH v2 4/8] tools/arm: Introduce the "nr_spis" xl config entry

[Jason Andryuk <jason.andryuk@amd.com>]

On 2024-05-16 06:03, Henry Wang wrote:
> Currently, the number of SPIs allocated to the domain is only
> configurable for Dom0less DomUs. Xen domains are supposed to be
> platform agnostics and therefore the numbers of SPIs for libxl
> guests should not be based on the hardware.
> 
> Introduce a new xl config entry for Arm to provide a method for
> user to decide the number of SPIs. This would help to avoid
> bumping the `config->arch.nr_spis` in libxl everytime there is a
> new platform with increased SPI numbers.
> 
> Update the doc and the golang bindings accordingly.
> 
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> ---
> v2:
> - New patch to replace the original patch in v1:
>    "[PATCH 05/15] tools/libs/light: Increase nr_spi to 160"
> ---
>   docs/man/xl.cfg.5.pod.in             | 11 +++++++++++
>   tools/golang/xenlight/helpers.gen.go |  2 ++
>   tools/golang/xenlight/types.gen.go   |  1 +
>   tools/libs/light/libxl_arm.c         |  4 ++--
>   tools/libs/light/libxl_types.idl     |  1 +
>   tools/xl/xl_parse.c                  |  3 +++
>   6 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 8f2b375ce9..6a2d86065e 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -3072,6 +3072,17 @@ raised.
>   
>   =back
>   
> +=over 4
> +
> +=item B<nr_spis="NR_SPIS">
> +
> +A 32-bit optional integer parameter specifying the number of SPIs (Shared

I'd phrase it "An optional 32-but integer"

> +Peripheral Interrupts) to allocate for the domain. If the `nr_spis` parameter
> +is missing, the max number of SPIs calculated by the toolstack based on the
> +devices allocated for the domain will be used.

This text says the maximum only applies if xl.cfg nr_spis is not setup.

> +
> +=back
> +
>   =head3 x86
>   
>   =over 4

> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 1cb89fa584..a4029e3ac8 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -181,8 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>   
>       LOG(DEBUG, "Configure the domain");
>   
> -    config->arch.nr_spis = nr_spis;
> -    LOG(DEBUG, " - Allocate %u 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);

But this is always taking the max.  Should it instead be:

config->arch.nr_spis = d_config->b_info.arch_arm.nr_spis ?: nr_spis;

However, I don't know if that makes sense for ARM.  Does the hardware 
nr_spis need to be a minimum for a domain?

Really, we just want the documentation to match the code.

Thanks,
Jason