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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 10/7/24 14:55, Jason Andryuk wrote:
> 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."
I will rework it with your suggestions.

> And some ascii art could help:
>
> [ headroom ][ compressed data ]
>   <decompression>
> [ decompressed data ]
>
> (Not sure how to create a down arrow...)
Yes, in fact I would just show the three states like this:

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 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);
> ?
yep, not sure why I even split it.

v/r,
dps