Re: [PATCH v5 07/44] x86/boot: move headroom to boot modules

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-10-06 17:49, Daniel P. Smith wrote:
> The purpose of struct boot_module is to encapsulate the state of boot module as
> it is processed by Xen. Locating boot module state struct boot_module reduces
> the number of global variables as well as the number of state variables that
> must be passed around. It also lays the groundwork for hyperlaunch mult-domain
> construction, where multiple instances of state variables like headroom will be
> needed.
>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> ---
>   xen/arch/x86/include/asm/bootinfo.h |  5 +++++
>   xen/arch/x86/setup.c                | 23 ++++++++++++++---------
>   2 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/x86/include/asm/bootinfo.h 
> b/xen/arch/x86/include/asm/bootinfo.h
> index d19473d8941e..c7e6b4ebf0da 100644
> --- a/xen/arch/x86/include/asm/bootinfo.h
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -17,6 +17,11 @@
>   struct boot_module {
>       /* Transitionary only */
>       module_t *mod;
> +    /*
> +     * A boot module may contain a compressed kernel that Xen will need space
> +     * reserved, into which it will be decompressed.
Maybe "Extra space, before the module data, for compressed kernel 
modules to be decompressed into."
And some ascii art could help:

[ headroom ][ compressed data ]
 <decompression>
[ decompressed data ]

(Not sure how to create a down arrow...)

> +     */
> +    unsigned long headroom;
>   };
>   
>   /*
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index ba9f110d98c6..dd82ca3d43e2 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1012,7 +1012,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long 
> mbi_p)
>       struct boot_info *bi;
>       multiboot_info_t *mbi;
>       module_t *mod;
> -    unsigned long nr_pages, raw_max_page, modules_headroom, module_map[1];
> +    unsigned long nr_pages, raw_max_page, module_map[1];
>       int i, j, e820_warn = 0, bytes = 0;
>       unsigned long eb_start, eb_end;
>       bool acpi_boot_table_init_done = false, relocated = false;
> @@ -1371,7 +1371,10 @@ void asmlinkage __init noreturn __start_xen(unsigned 
> long mbi_p)
>           mod[bi->nr_modules].mod_end = __2M_rwdata_end - _stext;
>       }
>   
> -    modules_headroom = bzimage_headroom(bootstrap_map(mod), mod->mod_end);
> +    bi->mods[0].headroom =
> +        bzimage_headroom(bootstrap_map(bi->mods[0].mod),
> +                         bi->mods[0].mod->mod_end);
> +
>       bootstrap_map(NULL);
>   
>   #ifndef highmem_start
> @@ -1456,8 +1459,10 @@ void asmlinkage __init noreturn __start_xen(unsigned 
> long mbi_p)
>                * decompressor overheads of mod[0] (the dom0 kernel).  When we
>                * move mod[0], we incorporate this as extra space at the start.
>                */
> -            unsigned long headroom = j ? 0 : modules_headroom;
> -            unsigned long size = PAGE_ALIGN(headroom + mod[j].mod_end);
> +            struct boot_module *bm = &bi->mods[j];
> +            unsigned long size;
> +
> +            size = PAGE_ALIGN(bm->headroom + mod[j].mod_end);
Just do
            unsigned long size = PAGE_ALIGN(bm->headroom + mod[j].mod_end);
?

>   
>               if ( mod[j].reserved )
>                   continue;
The rest looks good.

Regards,
Jason