The purpose of struct boot_module is to encapsulate the state of boot modules.
Doing locates boot module state with its respective boot module, reduces
globals and multiple state variables being passed around. It also lays the
ground work for hyperlaunch where more multiple instances of these state
variables like headroom will be needed.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/arch/x86/include/asm/bootinfo.h |  1 +
 xen/arch/x86/setup.c                | 22 +++++++++++++---------
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index 844262495962..3e0e36df096b 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -13,6 +13,7 @@
 
 struct boot_module {
     module_t *early_mod;
+    unsigned long headroom;
 };
 
 struct boot_info {
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 8912956ee7f1..fd6cc7fac907 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1031,7 +1031,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
     unsigned int initrdidx, num_parked = 0;
     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;
@@ -1390,7 +1390,9 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
         mod[boot_info->nr_mods].mod_end = __2M_rwdata_end - _stext;
     }
 
-    modules_headroom = bzimage_headroom(bootstrap_map(mod), mod->mod_end);
+    boot_info->mods[0].headroom = bzimage_headroom(
+                        bootstrap_map(boot_info->mods[0].early_mod),
+                        boot_info->mods[0].early_mod->mod_end);
     bootstrap_map(NULL);
 
 #ifndef highmem_start
@@ -1475,8 +1477,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 = &boot_info->mods[j];
+            unsigned long size;
+
+            size = PAGE_ALIGN(bm->headroom + mod[j].mod_end);
 
             if ( mod[j].reserved )
                 continue;
@@ -1489,14 +1493,14 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
                 continue;
 
             if ( s < end &&
-                 (headroom ||
+                 (bm->headroom ||
                   ((end - size) >> PAGE_SHIFT) > mod[j].mod_start) )
             {
-                move_memory(end - size + headroom,
+                move_memory(end - size + bm->headroom,
                             (uint64_t)mod[j].mod_start << PAGE_SHIFT,
                             mod[j].mod_end);
                 mod[j].mod_start = (end - size) >> PAGE_SHIFT;
-                mod[j].mod_end += headroom;
+                mod[j].mod_end += bm->headroom;
                 mod[j].reserved = 1;
             }
         }
@@ -1523,7 +1527,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
 #endif
     }
 
-    if ( modules_headroom && !mod->reserved )
+    if ( boot_info->mods[0].headroom && !mod->reserved )
         panic("Not enough memory to relocate the dom0 kernel image\n");
     for ( i = 0; i < boot_info->nr_mods; ++i )
     {
@@ -2077,7 +2081,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
      * We're going to setup domain0 using the module(s) that we stashed safely
      * above our heap. The second module, if present, is an initrd ramdisk.
      */
-    dom0 = create_dom0(mod, modules_headroom,
+    dom0 = create_dom0(mod, boot_info->mods[0].headroom,
                        initrdidx < boot_info->nr_mods ? mod + initrdidx : NULL,
                        kextra, boot_info->boot_loader_name);
     if ( !dom0 )
-- 
2.30.2

On 30.08.2024 23:46, Daniel P. Smith wrote:
> The purpose of struct boot_module is to encapsulate the state of boot modules.
> Doing locates boot module state with its respective boot module, reduces

I'm struggling with the start of this sentence.

> @@ -1390,7 +1390,9 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
>          mod[boot_info->nr_mods].mod_end = __2M_rwdata_end - _stext;
>      }
>  
> -    modules_headroom = bzimage_headroom(bootstrap_map(mod), mod->mod_end);
> +    boot_info->mods[0].headroom = bzimage_headroom(
> +                        bootstrap_map(boot_info->mods[0].early_mod),
> +                        boot_info->mods[0].early_mod->mod_end);

Nit: This is badly indented. Either

    boot_info->mods[0].headroom = bzimage_headroom(
        bootstrap_map(boot_info->mods[0].early_mod),
        boot_info->mods[0].early_mod->mod_end);

or

    boot_info->mods[0].headroom =
        bzimage_headroom(
            bootstrap_map(boot_info->mods[0].early_mod),
            boot_info->mods[0].early_mod->mod_end);

or

    boot_info->mods[0].headroom =
        bzimage_headroom(bootstrap_map(boot_info->mods[0].early_mod),
                         boot_info->mods[0].early_mod->mod_end);

Even shortening "boot_info" will not avoid some line wrapping here, as it
looks.

Jan

On 9/4/24 02:45, Jan Beulich wrote:
> On 30.08.2024 23:46, Daniel P. Smith wrote:
>> The purpose of struct boot_module is to encapsulate the state of boot modules.
>> Doing locates boot module state with its respective boot module, reduces
> 
> I'm struggling with the start of this sentence.

Yep, grammar check failed to catch. Will fix.

>> @@ -1390,7 +1390,9 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
>>           mod[boot_info->nr_mods].mod_end = __2M_rwdata_end - _stext;
>>       }
>>   
>> -    modules_headroom = bzimage_headroom(bootstrap_map(mod), mod->mod_end);
>> +    boot_info->mods[0].headroom = bzimage_headroom(
>> +                        bootstrap_map(boot_info->mods[0].early_mod),
>> +                        boot_info->mods[0].early_mod->mod_end);
> 
> Nit: This is badly indented. Either
> 
>      boot_info->mods[0].headroom = bzimage_headroom(
>          bootstrap_map(boot_info->mods[0].early_mod),
>          boot_info->mods[0].early_mod->mod_end);
> 
> or
> 
>      boot_info->mods[0].headroom =
>          bzimage_headroom(
>              bootstrap_map(boot_info->mods[0].early_mod),
>              boot_info->mods[0].early_mod->mod_end);
> 
> or
> 
>      boot_info->mods[0].headroom =
>          bzimage_headroom(bootstrap_map(boot_info->mods[0].early_mod),
>                           boot_info->mods[0].early_mod->mod_end);
> 
> Even shortening "boot_info" will not avoid some line wrapping here, as it
> looks.

I would lean towards the latter, as I find it the most clear. With the 
shortening of "boot_info" and dropping "early_" per Andy, It might bring 
it up one line. Will see when I do it.

v/r,
dps

On 30/08/2024 10:46 pm, Daniel P. Smith wrote:
> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
> index 844262495962..3e0e36df096b 100644
> --- a/xen/arch/x86/include/asm/bootinfo.h
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -13,6 +13,7 @@
>  
>  struct boot_module {
>      module_t *early_mod;
> +    unsigned long headroom;

This needs a comment explaining what it's for.  Perhaps crib from ...

> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 8912956ee7f1..fd6cc7fac907 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1475,8 +1477,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.
>               */

... here, while also editing to to prevent it going stale.

It is this patch which stops modules_headroom being strictly the dom0
kernel.

~Andrew

On 9/3/24 19:40, Andrew Cooper wrote:
> On 30/08/2024 10:46 pm, Daniel P. Smith wrote:
>> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
>> index 844262495962..3e0e36df096b 100644
>> --- a/xen/arch/x86/include/asm/bootinfo.h
>> +++ b/xen/arch/x86/include/asm/bootinfo.h
>> @@ -13,6 +13,7 @@
>>   
>>   struct boot_module {
>>       module_t *early_mod;
>> +    unsigned long headroom;
> 
> This needs a comment explaining what it's for.  Perhaps crib from ...

Ack.

>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 8912956ee7f1..fd6cc7fac907 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1475,8 +1477,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.
>>                */
> 
> ... here, while also editing to to prevent it going stale.
> 
> It is this patch which stops modules_headroom being strictly the dom0
> kernel.

Correct, because once multi-domain construction is introduced, there 
will be more than one kernel needing headroom for extraction.

v/r,
dps