[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] tools/arm: Fix nr_spis handling v2
On 20/03/2025 17:46, Anthony PERARD wrote: > > > On Tue, Mar 18, 2025 at 10:00:13AM +0100, 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@xxxxxxx> >> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx> >> --- >> 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; > > Could you try to check only once if there is a user provided value for > nr_spis? Sure, but that's one extra variable or more if/else blocks. > >> 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'}), > > Could you introduce something like LIBXL_NR_SPIS_DEFAULT in libxl.h > instead? (Like we have LIBXL_MAX_GRANT_DEFAULT or LIBXL_MEMKB_DEFAULT) Sure, no problem. ~Michal
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |