| Message ID | 20240516100330.1433265-5-xin.wang2@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remaining patches for dynamic node programming using overlay dtbo | expand |
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
Hi Jason, On 5/21/2024 3:13 AM, Jason Andryuk wrote: >> + >> +=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. Before you pointed this out, I didn't realize the ambiguity in the doc about the "max". The "max" in the doc have different meanings compared to the "max()" in the code. I will drop the "max" in the doc and reword the doc to "If the `nr_spis` parameter is missing, the number of SPIs calculated by the toolstack based on the devices allocated for the domain will be used.". Thanks for pointing it out. Kind regards, Henry > > Thanks, > Jason
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 +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. + +=back + =head3 x86 =over 4 diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go index 78bdb08b15..757ccaf035 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 ccfe18019e..b7b4ba88af 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); + 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 470122e768..3f143f405d 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), ])), ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool), ])), diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index ab09d0288b..4aa99029b5 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -2933,6 +2933,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;
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(-)