| Message ID | 20250325110029.399838-1-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] tools/arm: Fix nr_spis handling v2 | expand |
On Tue, Mar 25, 2025 at 12:00:29PM +0100, Michal Orzel wrote: > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c > index 2d895408cac3..0adcaa373b54 100644 > --- a/tools/libs/light/libxl_arm.c > +++ b/tools/libs/light/libxl_arm.c > @@ -181,13 +181,18 @@ 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) { > - LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n", > - nr_spis); > - return ERROR_FAIL; > + /* Check if a user provided a value or not */ > + if (cfg_nr_spis != LIBXL_NR_SPIS_DEFAULT) { > + if (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 = cfg_nr_spis; > } > + else > + config->arch.nr_spis = nr_spis; Just one small coding style issue: to avoid confusion, whenever one side a of an if..else is using a block, both side should use a block. But that can be fixed on commit: Reviewed-by: Anthony PERARD <anthony.perard@vates.tech> Thanks,
On 26/03/2025 15:47, Anthony PERARD wrote: > > > On Tue, Mar 25, 2025 at 12:00:29PM +0100, Michal Orzel wrote: >> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c >> index 2d895408cac3..0adcaa373b54 100644 >> --- a/tools/libs/light/libxl_arm.c >> +++ b/tools/libs/light/libxl_arm.c >> @@ -181,13 +181,18 @@ 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) { >> - LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n", >> - nr_spis); >> - return ERROR_FAIL; >> + /* Check if a user provided a value or not */ >> + if (cfg_nr_spis != LIBXL_NR_SPIS_DEFAULT) { >> + if (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 = cfg_nr_spis; >> } >> + else >> + config->arch.nr_spis = nr_spis; > > Just one small coding style issue: to avoid confusion, whenever one side > a of an if..else is using a block, both side should use a block. But Oh, I should have checked with tools/libs/light/CODING_STYLE. > that can be fixed on commit: > Reviewed-by: Anthony PERARD <anthony.perard@vates.tech> Fixing on commit would be great. ~Michal
Hi Michal, > On 25 Mar 2025, at 12: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 newly added > LIBXL_NR_SPIS_DEFAULT i.e. 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> Sounds good to me, so for Arm side of things: Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers Bertrand > --- > Changes in v2: > - add LIBXL_NR_SPIS_DEFAULT, rearrange checks > --- > tools/include/libxl.h | 1 + > tools/libs/light/libxl_arm.c | 17 +++++++++++------ > tools/libs/light/libxl_types.idl | 2 +- > 3 files changed, 13 insertions(+), 7 deletions(-) > > diff --git a/tools/include/libxl.h b/tools/include/libxl.h > index f8fe4afd7dca..b7ad7735ca4c 100644 > --- a/tools/include/libxl.h > +++ b/tools/include/libxl.h > @@ -1579,6 +1579,7 @@ bool libxl_defbool_val(libxl_defbool db); > > const char *libxl_defbool_to_string(libxl_defbool b); > > +#define LIBXL_NR_SPIS_DEFAULT (~(uint32_t)0) > #define LIBXL_TIMER_MODE_DEFAULT -1 > #define LIBXL_MEMKB_DEFAULT ~0ULL > > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c > index 2d895408cac3..0adcaa373b54 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,18 @@ 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) { > - LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n", > - nr_spis); > - return ERROR_FAIL; > + /* Check if a user provided a value or not */ > + if (cfg_nr_spis != LIBXL_NR_SPIS_DEFAULT) { > + if (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 = cfg_nr_spis; > } > + else > + config->arch.nr_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) { > diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl > index bd4b8721ff19..9bb296993199 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': 'LIBXL_NR_SPIS_DEFAULT'}), > ])), > ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool), > ])), > -- > 2.25.1 >
diff --git a/tools/include/libxl.h b/tools/include/libxl.h index f8fe4afd7dca..b7ad7735ca4c 100644 --- a/tools/include/libxl.h +++ b/tools/include/libxl.h @@ -1579,6 +1579,7 @@ bool libxl_defbool_val(libxl_defbool db); const char *libxl_defbool_to_string(libxl_defbool b); +#define LIBXL_NR_SPIS_DEFAULT (~(uint32_t)0) #define LIBXL_TIMER_MODE_DEFAULT -1 #define LIBXL_MEMKB_DEFAULT ~0ULL diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index 2d895408cac3..0adcaa373b54 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,18 @@ 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) { - LOG(ERROR, "Provided nr_spis value is too small (minimum required %u)\n", - nr_spis); - return ERROR_FAIL; + /* Check if a user provided a value or not */ + if (cfg_nr_spis != LIBXL_NR_SPIS_DEFAULT) { + if (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 = cfg_nr_spis; } + else + config->arch.nr_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) { diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl index bd4b8721ff19..9bb296993199 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': 'LIBXL_NR_SPIS_DEFAULT'}), ])), ("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 newly added LIBXL_NR_SPIS_DEFAULT i.e. 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> --- Changes in v2: - add LIBXL_NR_SPIS_DEFAULT, rearrange checks --- tools/include/libxl.h | 1 + tools/libs/light/libxl_arm.c | 17 +++++++++++------ tools/libs/light/libxl_types.idl | 2 +- 3 files changed, 13 insertions(+), 7 deletions(-)