| Message ID | 20250318090013.100823-1-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tools/arm: Fix nr_spis handling v2 | expand |
Hi Michal, > On 18 Mar 2025, at 09:00, Michal Orzel <michal.orzel@amd.com> wrote: > > We are missing a way to detect whether a user provided a value for > nr_spis equal to 0 or did not provide any value (default is also 0) which > can cause issues when calculated nr_spis is > 0 and the value from domain > config is 0. Fix it by setting default value for nr_spis to UINT32_MAX > (max supported nr of SPIs is 960 anyway). > > Fixes: 55d62b8d4636 ("tools/arm: Reject configuration with incorrect nr_spis value") > Reported-by: Luca Fancellu <luca.fancellu@arm.com> > Signed-off-by: Michal Orzel <michal.orzel@amd.com> > --- I’ve tested this fix using our test that mounts a virtio mmio disk on the domain, everything works fine. Tested-by: Luca Fancellu <luca.fancellu@arm.com> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
On Tue Mar 18, 2025 at 9:00 AM GMT, Michal Orzel wrote: > We are missing a way to detect whether a user provided a value for > nr_spis equal to 0 or did not provide any value (default is also 0) which > can cause issues when calculated nr_spis is > 0 and the value from domain > config is 0. Fix it by setting default value for nr_spis to UINT32_MAX > (max supported nr of SPIs is 960 anyway). > > Fixes: 55d62b8d4636 ("tools/arm: Reject configuration with incorrect nr_spis value") > Reported-by: Luca Fancellu <luca.fancellu@arm.com> > Signed-off-by: Michal Orzel <michal.orzel@amd.com> > --- > tools/libs/light/libxl_arm.c | 7 ++++--- > tools/libs/light/libxl_types.idl | 2 +- > 2 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c > index 2d895408cac3..5bb5bac61def 100644 > --- a/tools/libs/light/libxl_arm.c > +++ b/tools/libs/light/libxl_arm.c > @@ -84,7 +84,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, > libxl_domain_config *d_config, > struct xen_domctl_createdomain *config) > { > - uint32_t nr_spis = 0; > + uint32_t nr_spis = 0, cfg_nr_spis = d_config->b_info.arch_arm.nr_spis; > unsigned int i; > uint32_t vuart_irq, virtio_irq = 0; > bool vuart_enabled = false, virtio_enabled = false; > @@ -181,13 +181,14 @@ 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) { > + /* We use UINT32_MAX to denote if a user provided a value or not */ > + if ((cfg_nr_spis != UINT32_MAX) && (nr_spis > cfg_nr_spis)) { > LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n", > nr_spis); > return ERROR_FAIL; > } > > - config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis); > + config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : nr_spis; > LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis); > > switch (d_config->b_info.arch_arm.gic_version) { > diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl > index bd4b8721ff19..129c00ce929c 100644 > --- a/tools/libs/light/libxl_types.idl > +++ b/tools/libs/light/libxl_types.idl > @@ -723,7 +723,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), > + ("nr_spis", uint32, {'init_val': 'UINT32_MAX'}), > ])), > ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool), > ])), Doesn't this regenerate the golang bindings? Cheers, Alejandro
On 19/03/2025 2:01 pm, Alejandro Vallejo wrote: > On Tue Mar 18, 2025 at 9:00 AM GMT, Michal Orzel wrote: >> We are missing a way to detect whether a user provided a value for >> nr_spis equal to 0 or did not provide any value (default is also 0) which >> can cause issues when calculated nr_spis is > 0 and the value from domain >> config is 0. Fix it by setting default value for nr_spis to UINT32_MAX >> (max supported nr of SPIs is 960 anyway). >> >> Fixes: 55d62b8d4636 ("tools/arm: Reject configuration with incorrect nr_spis value") >> Reported-by: Luca Fancellu <luca.fancellu@arm.com> >> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >> --- >> tools/libs/light/libxl_arm.c | 7 ++++--- >> tools/libs/light/libxl_types.idl | 2 +- >> 2 files changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c >> index 2d895408cac3..5bb5bac61def 100644 >> --- a/tools/libs/light/libxl_arm.c >> +++ b/tools/libs/light/libxl_arm.c >> @@ -84,7 +84,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, >> libxl_domain_config *d_config, >> struct xen_domctl_createdomain *config) >> { >> - uint32_t nr_spis = 0; >> + uint32_t nr_spis = 0, cfg_nr_spis = d_config->b_info.arch_arm.nr_spis; >> unsigned int i; >> uint32_t vuart_irq, virtio_irq = 0; >> bool vuart_enabled = false, virtio_enabled = false; >> @@ -181,13 +181,14 @@ 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) { >> + /* We use UINT32_MAX to denote if a user provided a value or not */ >> + if ((cfg_nr_spis != UINT32_MAX) && (nr_spis > cfg_nr_spis)) { >> LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n", >> nr_spis); >> return ERROR_FAIL; >> } >> >> - config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis); >> + config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : nr_spis; >> LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis); >> >> switch (d_config->b_info.arch_arm.gic_version) { >> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl >> index bd4b8721ff19..129c00ce929c 100644 >> --- a/tools/libs/light/libxl_types.idl >> +++ b/tools/libs/light/libxl_types.idl >> @@ -723,7 +723,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), >> + ("nr_spis", uint32, {'init_val': 'UINT32_MAX'}), >> ])), >> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool), >> ])), > Doesn't this regenerate the golang bindings? You need a build environment with golang for that to happen. It's easy to miss. In this case, best to ask the committer to regen. ~Andrew
On 19/03/2025 15:05, Andrew Cooper wrote: > > > On 19/03/2025 2:01 pm, Alejandro Vallejo wrote: >> On Tue Mar 18, 2025 at 9:00 AM GMT, Michal Orzel wrote: >>> We are missing a way to detect whether a user provided a value for >>> nr_spis equal to 0 or did not provide any value (default is also 0) which >>> can cause issues when calculated nr_spis is > 0 and the value from domain >>> config is 0. Fix it by setting default value for nr_spis to UINT32_MAX >>> (max supported nr of SPIs is 960 anyway). >>> >>> Fixes: 55d62b8d4636 ("tools/arm: Reject configuration with incorrect nr_spis value") >>> Reported-by: Luca Fancellu <luca.fancellu@arm.com> >>> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >>> --- >>> tools/libs/light/libxl_arm.c | 7 ++++--- >>> tools/libs/light/libxl_types.idl | 2 +- >>> 2 files changed, 5 insertions(+), 4 deletions(-) >>> >>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c >>> index 2d895408cac3..5bb5bac61def 100644 >>> --- a/tools/libs/light/libxl_arm.c >>> +++ b/tools/libs/light/libxl_arm.c >>> @@ -84,7 +84,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, >>> libxl_domain_config *d_config, >>> struct xen_domctl_createdomain *config) >>> { >>> - uint32_t nr_spis = 0; >>> + uint32_t nr_spis = 0, cfg_nr_spis = d_config->b_info.arch_arm.nr_spis; >>> unsigned int i; >>> uint32_t vuart_irq, virtio_irq = 0; >>> bool vuart_enabled = false, virtio_enabled = false; >>> @@ -181,13 +181,14 @@ 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) { >>> + /* We use UINT32_MAX to denote if a user provided a value or not */ >>> + if ((cfg_nr_spis != UINT32_MAX) && (nr_spis > cfg_nr_spis)) { >>> LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n", >>> nr_spis); >>> return ERROR_FAIL; >>> } >>> >>> - config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis); >>> + config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : nr_spis; >>> LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis); >>> >>> switch (d_config->b_info.arch_arm.gic_version) { >>> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl >>> index bd4b8721ff19..129c00ce929c 100644 >>> --- a/tools/libs/light/libxl_types.idl >>> +++ b/tools/libs/light/libxl_types.idl >>> @@ -723,7 +723,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), >>> + ("nr_spis", uint32, {'init_val': 'UINT32_MAX'}), >>> ])), >>> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool), >>> ])), >> Doesn't this regenerate the golang bindings? > > You need a build environment with golang for that to happen. It's easy > to miss. > > In this case, best to ask the committer to regen. This patch is already waiting for tools Ab, so maybe @Anthony could do that on commit? ~Michal
On Wed Mar 19, 2025 at 2:05 PM GMT, Andrew Cooper wrote: > On 19/03/2025 2:01 pm, Alejandro Vallejo wrote: > > On Tue Mar 18, 2025 at 9:00 AM GMT, Michal Orzel wrote: > >> We are missing a way to detect whether a user provided a value for > >> nr_spis equal to 0 or did not provide any value (default is also 0) which > >> can cause issues when calculated nr_spis is > 0 and the value from domain > >> config is 0. Fix it by setting default value for nr_spis to UINT32_MAX > >> (max supported nr of SPIs is 960 anyway). > >> > >> Fixes: 55d62b8d4636 ("tools/arm: Reject configuration with incorrect nr_spis value") > >> Reported-by: Luca Fancellu <luca.fancellu@arm.com> > >> Signed-off-by: Michal Orzel <michal.orzel@amd.com> > >> --- > >> tools/libs/light/libxl_arm.c | 7 ++++--- > >> tools/libs/light/libxl_types.idl | 2 +- > >> 2 files changed, 5 insertions(+), 4 deletions(-) > >> > >> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c > >> index 2d895408cac3..5bb5bac61def 100644 > >> --- a/tools/libs/light/libxl_arm.c > >> +++ b/tools/libs/light/libxl_arm.c > >> @@ -84,7 +84,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, > >> libxl_domain_config *d_config, > >> struct xen_domctl_createdomain *config) > >> { > >> - uint32_t nr_spis = 0; > >> + uint32_t nr_spis = 0, cfg_nr_spis = d_config->b_info.arch_arm.nr_spis; > >> unsigned int i; > >> uint32_t vuart_irq, virtio_irq = 0; > >> bool vuart_enabled = false, virtio_enabled = false; > >> @@ -181,13 +181,14 @@ 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) { > >> + /* We use UINT32_MAX to denote if a user provided a value or not */ > >> + if ((cfg_nr_spis != UINT32_MAX) && (nr_spis > cfg_nr_spis)) { > >> LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n", > >> nr_spis); > >> return ERROR_FAIL; > >> } > >> > >> - config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis); > >> + config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : nr_spis; > >> LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis); > >> > >> switch (d_config->b_info.arch_arm.gic_version) { > >> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl > >> index bd4b8721ff19..129c00ce929c 100644 > >> --- a/tools/libs/light/libxl_types.idl > >> +++ b/tools/libs/light/libxl_types.idl > >> @@ -723,7 +723,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), > >> + ("nr_spis", uint32, {'init_val': 'UINT32_MAX'}), > >> ])), > >> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool), > >> ])), > > Doesn't this regenerate the golang bindings? > > You need a build environment with golang for that to happen. It's easy > to miss. > > In this case, best to ask the committer to regen. > > ~Andrew One more thing for the list of stuff I'd like to add to CI at some point. Cheers, Alejandro
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index 2d895408cac3..5bb5bac61def 100644 --- a/tools/libs/light/libxl_arm.c +++ b/tools/libs/light/libxl_arm.c @@ -84,7 +84,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, libxl_domain_config *d_config, struct xen_domctl_createdomain *config) { - uint32_t nr_spis = 0; + uint32_t nr_spis = 0, cfg_nr_spis = d_config->b_info.arch_arm.nr_spis; unsigned int i; uint32_t vuart_irq, virtio_irq = 0; bool vuart_enabled = false, virtio_enabled = false; @@ -181,13 +181,14 @@ 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) { + /* We use UINT32_MAX to denote if a user provided a value or not */ + if ((cfg_nr_spis != UINT32_MAX) && (nr_spis > cfg_nr_spis)) { LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n", nr_spis); return ERROR_FAIL; } - config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis); + config->arch.nr_spis = (cfg_nr_spis != UINT32_MAX) ? cfg_nr_spis : nr_spis; LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis); switch (d_config->b_info.arch_arm.gic_version) { diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl index bd4b8721ff19..129c00ce929c 100644 --- a/tools/libs/light/libxl_types.idl +++ b/tools/libs/light/libxl_types.idl @@ -723,7 +723,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), + ("nr_spis", uint32, {'init_val': 'UINT32_MAX'}), ])), ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool), ])),
We are missing a way to detect whether a user provided a value for nr_spis equal to 0 or did not provide any value (default is also 0) which can cause issues when calculated nr_spis is > 0 and the value from domain config is 0. Fix it by setting default value for nr_spis to UINT32_MAX (max supported nr of SPIs is 960 anyway). Fixes: 55d62b8d4636 ("tools/arm: Reject configuration with incorrect nr_spis value") Reported-by: Luca Fancellu <luca.fancellu@arm.com> Signed-off-by: Michal Orzel <michal.orzel@amd.com> --- tools/libs/light/libxl_arm.c | 7 ++++--- tools/libs/light/libxl_types.idl | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-)