Re: [PATCH 09/12] x86/boot: add start and size fields to struct boot_module

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-11-02 13:25, Daniel P. Smith wrote:
> Introduce the start and size fields to struct boot_module and
> assigns their value during boot_info construction. All uses of module_t to get
> the address and size of a module are replaced with start and size.
>
> The EFI entry point is a special case, as the EFI file loading boot service may
> load a file beyond the 4G barrier. As a result, to make the address fit in the
> 32bit integer used by the MB1 module_t structure, the frame number is stored in
> mod_start and size in mod_end. Until the EFI entry point is enlightened to work
> with boot_info and boot_module, multiboot_fill_boot_info will handle the
> alternate values in mod_start and mod_end when EFI is detected.
>
> A result of the switch to start/size removes all uses of the mod field in
> struct boot_modules, along with the uses of bootstra_map() and release_module()
> functions. With all usage gone, they all are dropped here.
>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> --
> Changes since v7:
> - add the start/size change to bootstrap_map_bm()
> - convert all BM start/size when introduced, consolidates:
>      x86/boot: populate boot module for xen entry
>      x86/boot: transition relocation calculations to struct boot_module
> - consolidates all the removal commits
>
> Changes since v6:
> - put the efi conversion for mod_start and mod_end back along with check
> - dropped unnecessary cast
> - updated the population of start and size fields to take into account efi
>
> Changes since v5:
> - switched EFI population of mod_start/mod_end to addresses
>
> a#    edit 336ac1fc0019 x86/boot: introduce boot domain
> ---
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index d9785acf89b6..18b93d6a272a 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -313,13 +313,29 @@ static struct boot_info *__init multiboot_fill_boot_info(
>        */
>       for ( i = 0; i < MAX_NR_BOOTMODS && i < bi->nr_modules; i++ )
>       {
> -        bi->mods[i].mod = &mods[i];
> -
>           bi->mods[i].cmdline_pa = mods[i].string;
> +
> +        if ( !efi_enabled(EFI_LOADER) )
> +        {
> +            /*
> +             * The EFI loader gives us modules which are already frame/size.
> +             * Switch back to address/size.
> +             */
This comment...

> +            bi->mods[i].start = mods[i].mod_start;
> +            bi->mods[i].size = mods[i].mod_end - mods[i].mod_start;
> +        }
> +        else
> +        {
> +            /*
> +             * PVH and BIOS loaders give us modules which are start/end.
> +             * Switch to address/size.
> +             */
And this comment are reversed?  But I would just use positive logic:

if ( efi_enabled(EFI_LOADER) )
   /* EFI case */
else
   /* non-EFI */

> +            bi->mods[i].start = pfn_to_paddr(mods[i].mod_start);
> +            bi->mods[i].size = mods[i].mod_end;
> +        }
>       }
>   
>       /* Variable 'i' should be one entry past the last module. */
> -    bi->mods[i].mod = &mods[bi->nr_modules];
>       bi->mods[i].type = BOOTMOD_XEN;
>   
>       return bi;
> @@ -335,8 +351,8 @@ unsigned long __init initial_images_nrpages(nodeid_t node)
>   
>       for ( nr = i = 0; i < bi->nr_modules; ++i )
>       {
> -        unsigned long start = bi->mods[i].mod->mod_start;
> -        unsigned long end   = start + PFN_UP(bi->mods[i].mod->mod_end);
> +        unsigned long start = bi->mods[i].start;
This should be paddr_to_pfn(bi->mods[i].start)?

> +        unsigned long end   = start + PFN_UP(bi->mods[i].size);
>   
>           if ( end > node_start && node_end > start )
>               nr += min(node_end, end) - max(node_start, start);
> @@ -1745,13 +1733,11 @@ void asmlinkage __init noreturn __start_xen(void)
>   
>       for ( i = 0; i < bi->nr_modules; ++i )
>       {
> -        const struct boot_module *bm = &bi->mods[i];
> +        unsigned long s = bi->mods[i].start, l = bi->mods[i].size;
>   
> -        set_pdx_range(bm->mod->mod_start,
> -                      bm->mod->mod_start + PFN_UP(bm->mod->mod_end));
> -        map_pages_to_xen((unsigned long)mfn_to_virt(bm->mod->mod_start),
> -                         _mfn(bm->mod->mod_start),
> -                         PFN_UP(bm->mod->mod_end), PAGE_HYPERVISOR);
> +        set_pdx_range(paddr_to_pfn(s), paddr_to_pfn(s) + PFN_UP(l));
This is fine today since s (.start) is checked for page alignment.  The 
other option would be `paddr_to_pfn(s + l) + 1`, but I'm not sure that 
is an improvement.
You don't have to change anything.  Just noting something I noticed.

Regards,
Jason

> +        map_pages_to_xen((unsigned long)maddr_to_virt(s), maddr_to_mfn(s),
> +                         PFN_UP(l), PAGE_HYPERVISOR);
>       }
>   
>   #ifdef CONFIG_KEXEC