modules_headroom is a misleading name as it applies strictly to mod[0] only,
and the movement loop is deeply unintuitive and completely undocumented.

Provide help to whomever needs to look at this code next.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Daniel Smith <dpsmith@apertussolutions.com>
CC: Christopher Clark <christopher.w.clark@gmail.com>
---
 xen/arch/x86/setup.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index caf235c6286d..59907fae095f 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1432,6 +1432,11 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
         /* Is the region suitable for relocating the multiboot modules? */
         for ( j = mbi->mods_count - 1; j >= 0; j-- )
         {
+            /*
+             * 'headroom' is a guess for the decompressed size and
+             * decompressor overheads of mod[0] (the dom0 kernel).  When we
+             * move mod[0], we incorperate this as extra space at the start.
+             */
             unsigned long headroom = j ? 0 : modules_headroom;
             unsigned long size = PAGE_ALIGN(headroom + mod[j].mod_end);
 
-- 
2.30.2

On 2024-04-26 10:01, Andrew Cooper wrote:
> modules_headroom is a misleading name as it applies strictly to mod[0] only,
> and the movement loop is deeply unintuitive and completely undocumented.
> 
> Provide help to whomever needs to look at this code next.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Daniel Smith <dpsmith@apertussolutions.com>
> CC: Christopher Clark <christopher.w.clark@gmail.com>
> ---
>   xen/arch/x86/setup.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index caf235c6286d..59907fae095f 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1432,6 +1432,11 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
>           /* Is the region suitable for relocating the multiboot modules? */
>           for ( j = mbi->mods_count - 1; j >= 0; j-- )
>           {
> +            /*
> +             * 'headroom' is a guess for the decompressed size and
> +             * decompressor overheads of mod[0] (the dom0 kernel).  When we
> +             * move mod[0], we incorperate this as extra space at the start.

                                   incorporate

With that:
Reviewed-by: Jason Andryuk <jason.andryuk@gmail.com>

Thanks,
Jason

> +             */
>               unsigned long headroom = j ? 0 : modules_headroom;
>               unsigned long size = PAGE_ALIGN(headroom + mod[j].mod_end);
>   

On 26.04.2024 16:01, Andrew Cooper wrote:
> modules_headroom is a misleading name as it applies strictly to mod[0] only,
> and the movement loop is deeply unintuitive and completely undocumented.
> 
> Provide help to whomever needs to look at this code next.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>