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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 24 May 2024, Julien Grall wrote:
> 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.

I change it


> > +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).

I added it


> > +
> > +=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.

I left it as is for now


> > +    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.

I added it


> >                                 ])),
> >       ("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
>