Re: [PATCH v1 01/18] kconfig: allow configuration of maximum modules

[Julien Grall <julien@xxxxxxx>]

Hi Daniel,

On 06/07/2022 22:04, Daniel P. Smith wrote:
> For x86 the number of allowable multiboot modules varies between the different
> entry points, non-efi boot, pvh boot, and efi boot. In the case of both Arm and
> x86 this value is fixed to values based on generalized assumptions. With
> hyperlaunch for x86 and dom0less on Arm, use of static sizes results in large
> allocations compiled into the hypervisor that will go unused by many use cases.
>
> This commit introduces a Kconfig variable that is set with sane defaults based
> on configuration selection. This variable is in turned used as the array size
> for the cases where a static allocated array of boot modules is declared.
>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Christopher Clark <christopher.clark@xxxxxxxxxx>
I am not entirely sure where this reviewed-by is coming from. Is this 
from internal review?
If yes, my recommendation would be to provide the reviewed-by on the 
mailing list. Ideally, the review should also be done in the open, but I 
understand some company wish to do a fully internal review first.
At least from a committer perspective, this helps me to know whether the 
reviewed-by still apply. An example would be if you send a v2, I would 
not be able to know whether Christoffer still agreed on the change.
> ---
>   xen/arch/Kconfig                  | 12 ++++++++++++
>   xen/arch/arm/include/asm/setup.h  |  5 +++--
>   xen/arch/x86/efi/efi-boot.h       |  2 +-
>   xen/arch/x86/guest/xen/pvh-boot.c |  2 +-
>   xen/arch/x86/setup.c              |  4 ++--
>   5 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
> index f16eb0df43..24139057be 100644
> --- a/xen/arch/Kconfig
> +++ b/xen/arch/Kconfig
> @@ -17,3 +17,15 @@ config NR_CPUS
>           For CPU cores which support Simultaneous Multi-Threading or similar
>           technologies, this the number of logical threads which Xen will
>           support.
> +
> +config NR_BOOTMODS
> +       int "Maximum number of boot modules that a loader can pass"
> +       range 1 32768
> +       default "8" if X86
> +       default "32" if ARM
> +       help
> +         Controls the build-time size of various arrays allocated for
> +         parsing the boot modules passed by a loader when starting Xen.
> +
> +         This is of particular interest when using Xen's hypervisor domain
> +         capabilities such as dom0less.
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index 2bb01ecfa8..312a3e4209 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -10,7 +10,8 @@
>   
>   #define NR_MEM_BANKS 256
>   
> -#define MAX_MODULES 32 /* Current maximum useful modules */
> +/* Current maximum useful modules */
> +#define MAX_MODULES CONFIG_NR_BOOTMODS
>   
>   typedef enum {
>       BOOTMOD_XEN,
> @@ -38,7 +39,7 @@ struct meminfo {
>    * The domU flag is set for kernels and ramdisks of "xen,domain" nodes.
>    * The purpose of the domU flag is to avoid getting confused in
>    * kernel_probe, where we try to guess which is the dom0 kernel and
> - * initrd to be compatible with all versions of the multiboot spec.
> + * initrd to be compatible with all versions of the multiboot spec.
In general, I much prefer if coding style changes are done separately 
because it helps the review (I don't have to stare at the line to figure 
out what changed).
I am not going to force this here. However, the strict minimum is to 
mention the change in the commit message.
>    */
>   #define BOOTMOD_MAX_CMDLINE 1024
>   struct bootmodule {
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 6e65b569b0..4e1a799749 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -18,7 +18,7 @@ static multiboot_info_t __initdata mbi = {
>    * The array size needs to be one larger than the number of modules we
>    * support - see __start_xen().
>    */
> -static module_t __initdata mb_modules[5];
> +static module_t __initdata mb_modules[CONFIG_NR_BOOTMODS + 1];
Please explain in the commit message why the number of modules was 
bumped from 5 to 9.
>   
>   static void __init edd_put_string(u8 *dst, size_t n, const char *src)
>   {
> diff --git a/xen/arch/x86/guest/xen/pvh-boot.c 
> b/xen/arch/x86/guest/xen/pvh-boot.c
> index 498625eae0..834b1ad16b 100644
> --- a/xen/arch/x86/guest/xen/pvh-boot.c
> +++ b/xen/arch/x86/guest/xen/pvh-boot.c
> @@ -32,7 +32,7 @@ bool __initdata pvh_boot;
>   uint32_t __initdata pvh_start_info_pa;
>   
>   static multiboot_info_t __initdata pvh_mbi;
> -static module_t __initdata pvh_mbi_mods[8];
> +static module_t __initdata pvh_mbi_mods[CONFIG_NR_BOOTMOD + 1];
What's the +1 for?

>   static const char *__initdata pvh_loader = "PVH Directboot";
>   
>   static void __init convert_pvh_info(multiboot_info_t **mbi,
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index f08b07b8de..2aa1e28c8f 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1020,9 +1020,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>           panic("dom0 kernel not specified. Check bootloader configuration\n");
>   
>       /* Check that we don't have a silly number of modules. */
> -    if ( mbi->mods_count > sizeof(module_map) * 8 )
> +    if ( mbi->mods_count > CONFIG_NR_BOOTMODS )
>       {
> -        mbi->mods_count = sizeof(module_map) * 8;
> +        mbi->mods_count = CONFIG_NR_BOOTMODS;
>           printk("Excessive multiboot modules - using the first %u only\n",
>                  mbi->mods_count);
>       }
AFAIU, this check is to make sure that we will not overrun module_map in 
the next line:
bitmap_fill(module_map, mbi->mods_count);

The current definition of module_map will allow 64 modules. But you are 
allowing 32768. So I think you either want to keep the check or define 
module_map as:
DECLARE_BITMAP(module_map, CONFIG_NR_BOOTMODS);

Cheers,

--
Julien Grall