Re: [PATCH v4 3/9] tools/arm: Introduce the "nr_spis" xl config entry

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 23/05/2024 08:40, 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@xxxxxxx>
> Reviewed-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> ---
> v4:
> - Add Jason's Reviewed-by tag.
> v3:
> - Reword documentation to avoid ambiguity.
> 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             | 14 ++++++++++++++
>   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, 23 insertions(+), 2 deletions(-)
>
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 8f2b375ce9..416d582844 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -3072,6 +3072,20 @@ raised.
>   
>   =back
>   
> +=over 4
> +
> +=item B<nr_spis="NR_SPIS">
> +
> +An optional 32-bit integer parameter specifying the number of SPIs (Shared
We can't support that much SPIs :). The limit would be 991 SPIs.

> +Peripheral Interrupts) to allocate for the domain. 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.
I think it would be worth mentioning that the number of SPIs should 
match the highest interrupt ID that will be assigned to the domain 
(rather than the number of SPIs planned to be assigned).
> +
> +=back
> +
>   =head3 x86
>   
>   =over 4
> diff --git a/tools/golang/xenlight/helpers.gen.go 
> b/tools/golang/xenlight/helpers.gen.go
> index b9cb5b33c7..fe5110474d 100644
> --- a/tools/golang/xenlight/helpers.gen.go
> +++ b/tools/golang/xenlight/helpers.gen.go
> @@ -1154,6 +1154,7 @@ return fmt.Errorf("invalid union key '%v'", x.Type)}
>   x.ArchArm.GicVersion = GicVersion(xc.arch_arm.gic_version)
>   x.ArchArm.Vuart = VuartType(xc.arch_arm.vuart)
>   x.ArchArm.SveVl = SveType(xc.arch_arm.sve_vl)
> +x.ArchArm.NrSpis = uint32(xc.arch_arm.nr_spis)
>   if err := x.ArchX86.MsrRelaxed.fromC(&xc.arch_x86.msr_relaxed);err != nil {
>   return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
>   }
> @@ -1670,6 +1671,7 @@ return fmt.Errorf("invalid union key '%v'", x.Type)}
>   xc.arch_arm.gic_version = C.libxl_gic_version(x.ArchArm.GicVersion)
>   xc.arch_arm.vuart = C.libxl_vuart_type(x.ArchArm.Vuart)
>   xc.arch_arm.sve_vl = C.libxl_sve_type(x.ArchArm.SveVl)
> +xc.arch_arm.nr_spis = C.uint32_t(x.ArchArm.NrSpis)
>   if err := x.ArchX86.MsrRelaxed.toC(&xc.arch_x86.msr_relaxed); err != nil {
>   return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
>   }
> diff --git a/tools/golang/xenlight/types.gen.go 
> b/tools/golang/xenlight/types.gen.go
> index 5b293755d7..c9e45b306f 100644
> --- a/tools/golang/xenlight/types.gen.go
> +++ b/tools/golang/xenlight/types.gen.go
> @@ -597,6 +597,7 @@ ArchArm struct {
>   GicVersion GicVersion
>   Vuart VuartType
>   SveVl SveType
> +NrSpis uint32
>   }
>   ArchX86 struct {
>   MsrRelaxed Defbool
> 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);
I am not entirely sure about using max(). To me if the user specifies a 
lower limit, then we should throw an error because this is likely an 
indication that the SPIs they will want to assign will clash with the 
emulated ones.
So it would be better to warn at domain creation rather than waiting 
until the IRQs are assigned.
I would like Anthony's opinion on this one. Given he is away this month, 
I guess we could get this patch merged (with other comments addressed) 
and have a follow-up if wanted before 4.19.
> +    LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
>   
>       switch (d_config->b_info.arch_arm.gic_version) {
>       case LIBXL_GIC_VERSION_DEFAULT:
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index 79e9c656cc..4e65e6fda5 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -722,6 +722,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),
From my understandig, any change in the .idl requires a corresponding 
LIBXL_HAVE_... in include/libxl.h. This is in order to allow external 
toolstack (such as libvirt) to be able to select at build time between 
multiple version of libxl.
>                                 ])),
>       ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>                                 ])),
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index c504ab3711..e3a4800f6e 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -2935,6 +2935,9 @@ skip_usbdev:
>           }
>       }
>   
> +    if (!xlu_cfg_get_long (config, "nr_spis", &l, 0))
> +        b_info->arch_arm.nr_spis = l;
> +
>       parse_vkb_list(config, d_config);
>   
>       d_config->virtios = NULL;
Cheers,

--
Julien Grall