Re: [PATCH 01/10] x86 setup: move x86 boot module counting into a new boot_info struct

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.07.2023 09:18, Christopher Clark wrote:
> @@ -1127,18 +1139,18 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>             bootsym(boot_edd_info_nr));
>  
>      /* Check that we have at least one Multiboot module. */
> -    if ( !(mbi->flags & MBI_MODULES) || (mbi->mods_count == 0) )
> +    if ( !(mbi->flags & MBI_MODULES) || (boot_info->nr_mods == 0) )
>          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 ( boot_info->nr_mods > sizeof(module_map) * 8 )
>      {
> -        mbi->mods_count = sizeof(module_map) * 8;
> +        boot_info->nr_mods = sizeof(module_map) * 8;

As long as you don't replace all consumers of mbi->mods_count (see in
particular the call trees down from early_microcode_init() and down from
xsm_multiboot_init()), I don't think you can drop the original assignment.

>          printk("Excessive multiboot modules - using the first %u only\n",
> -               mbi->mods_count);
> +               boot_info->nr_mods);
>      }
>  
> -    bitmap_fill(module_map, mbi->mods_count);
> +    bitmap_fill(module_map, boot_info->nr_mods);
>      __clear_bit(0, module_map); /* Dom0 kernel is always first */
>  
>      if ( pvh_boot )
> @@ -1311,9 +1323,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      kexec_reserve_area(&boot_e820);
>  
>      initial_images = mod;
> -    nr_initial_images = mbi->mods_count;
> +    boot_info->nr_mods = boot_info->nr_mods;

Overly mechanical change? To prevent such going unnoticed, maybe
boot_info should be pointer-to-const? Would of course require the
bounding to occur earlier, so you truly don't need to write the struct
after filling it in multiboot_to_bootinfo(). Or you move the call to
that function down a little. Yet other options also exist.

> -    for ( i = 0; !efi_enabled(EFI_LOADER) && i < mbi->mods_count; i++ )
> +    for ( i = 0; !efi_enabled(EFI_LOADER) && i < boot_info->nr_mods; i++ )

Seeing all sorts of changes of this kind - did you consider naming the
pointer variable (which will become a function parameter as to what I
understood from your reply to Stefano) just "bi"? (I wouldn't suggest
this if the variable was to remain file-scope.)

Jan