| Message ID | 20241021004613.18793-4-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Boot modules for Hyperlaunch | expand |
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
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
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 )