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@apertussolutions.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
Changes since v6:
- add blank line to separate comment from line above it

Changes since v5:
- reword and expand comment on headroom
- consolidated declaration and assignment
---
 xen/arch/x86/include/asm/bootinfo.h | 14 ++++++++++++++
 xen/arch/x86/setup.c                | 21 ++++++++++++---------
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index ffa443406747..59e6696f9671 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -17,6 +17,20 @@
 struct boot_module {
     /* Transitionary only */
     module_t *mod;
+
+    /*
+     * A boot module may contain a compressed kernel that will require
+     * additional space, before the module data, into which the kernel will be
+     * decompressed.
+     *
+     * Memory layout at boot:
+     *     [ compressed kernel ]
+     * After boot module relocation:
+     *     [ estimated headroom + PAGE_SIZE rounding ][ compressed kernel ]
+     * After kernel decompression:
+     *     [ decompressed kernel ][ unused rounding ]
+     */
+    unsigned long headroom;
 };
 
 /*
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 1eafa0a61e0e..48809aa94451 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1028,7 +1028,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;
@@ -1394,7 +1394,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
@@ -1479,8 +1482,8 @@ 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 = PAGE_ALIGN(bm->headroom + mod[j].mod_end);
 
             if ( mod[j].reserved )
                 continue;
@@ -1493,14 +1496,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;
             }
         }
@@ -1527,7 +1530,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
 #endif
     }
 
-    if ( modules_headroom && !mod->reserved )
+    if ( bi->mods[0].headroom && !mod->reserved )
         panic("Not enough memory to relocate the dom0 kernel image\n");
     for ( i = 0; i < bi->nr_modules; ++i )
     {
@@ -2079,7 +2082,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, bi->mods[0].headroom,
                        initrdidx < bi->nr_modules ? mod + initrdidx : NULL,
                        kextra, bi->loader);
     if ( !dom0 )
-- 
2.30.2

On 21/10/2024 1:45 am, 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@apertussolutions.com>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Actually, based on the observation in the subsequent patch about pulling
the initial_image deletion earlier in the series, I'm going to recommend
something else here.

You should fully remove the mod pointer in __start_xen() (i.e. convert
mod[] references to bi->mods[]) before starting to remove other
variables such as module_headroom here.

This is largely doable because microcode and xsm both take the mbi
pointer and map mbi->module_addr themselves, rather than take
__start_xen()'s pointer.

Again, this reduces churn in the series, and minimises the extent to
which we're operating on multiple representations of the same data.

~Andrew

On 21/10/2024 1:45 am, 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@apertussolutions.com>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> ---
> Changes since v6:
> - add blank line to separate comment from line above it
>
> Changes since v5:
> - reword and expand comment on headroom
> - consolidated declaration and assignment
> ---
>  xen/arch/x86/include/asm/bootinfo.h | 14 ++++++++++++++
>  xen/arch/x86/setup.c                | 21 ++++++++++++---------
>  2 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
> index ffa443406747..59e6696f9671 100644
> --- a/xen/arch/x86/include/asm/bootinfo.h
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -17,6 +17,20 @@
>  struct boot_module {
>      /* Transitionary only */
>      module_t *mod;
> +
> +    /*
> +     * A boot module may contain a compressed kernel that will require
> +     * additional space, before the module data, into which the kernel will be
> +     * decompressed.

The grammar is a bit awkward.  Space beforehand is an implementation
choice of Xen, not a requirement.

Can I suggest:

---%<---
A boot module may need decompressing by Xen.  Headroom is an estimate of
the additional space required to decompress the module.

Headroom is accounted for at the start of the module.  Decompressing is
done in-place with input=start, output=start-headroom, expecting the
pointers to become equal (give or take some rounding) when decompression
is complete.
---%<---


> +     *
> +     * Memory layout at boot:
> +     *     [ compressed kernel ]
> +     * After boot module relocation:
> +     *     [ estimated headroom + PAGE_SIZE rounding ][ compressed kernel ]
> +     * After kernel decompression:
> +     *     [ decompressed kernel ][ unused rounding ]
> +     */

I'm not sure how helpful this is.  If anything, I'd say it ought to be:

                       start + size --+
          start --+                   |
                  v                   v
                  +-------------------+
                  | Compressed Module |
 +----------------+-------------------+
 |         Decompressed Module        |
 +----------------+-------------------+
 ^
 +-- start - headroom


~Andrew