| Message ID | 20240830214730.1621-6-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Boot modules for Hyperlaunch | expand |
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 e785ed1c5982..844262495962 100644 > --- a/xen/arch/x86/include/asm/bootinfo.h > +++ b/xen/arch/x86/include/asm/bootinfo.h > @@ -8,10 +8,16 @@ > #ifndef __XEN_X86_BOOTINFO_H__ > #define __XEN_X86_BOOTINFO_H__ > > +#include <xen/multiboot.h> > #include <xen/types.h> > > +struct boot_module { > + module_t *early_mod; This could do with a /* Transitionary only */ comment. In this patch it's not too bad, but it does get worse as new fields are added, before being removed. I'd also drop the "early_" part. I know it's the initial_images array we're converting, but "early_" doesn't convey any extra meaning, and it makes a number of lines get quite hairy. > +}; > + > struct boot_info { > unsigned int nr_mods; > + struct boot_module *mods; struct boot_module modules[MAX_NR_BOOTMODS + 1]; Probably at the end of the structure. In turn it ... > > const char *boot_loader_name; > const char *cmdline; > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index c6b45ced00ae..28fdbf4d4c2b 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -278,12 +278,17 @@ custom_param("acpi", parse_acpi_param); > > static const char *cmdline_cook(const char *p, const char *loader_name); > > +/* Max number of boot modules a bootloader can provide in addition to Xen */ > +#define MAX_NR_BOOTMODS 63 > + > static const module_t *__initdata initial_images; > static struct boot_info __initdata *boot_info; > > -static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) > +static void __init multiboot_to_bootinfo(multiboot_info_t *mbi, module_t *mods) > { > static struct boot_info __initdata info; > + static struct boot_module __initdata boot_mods[MAX_NR_BOOTMODS + 1]; ... drops this static. > + unsigned int i; > > info.nr_mods = mbi->mods_count; > > @@ -303,6 +308,14 @@ static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) > info.mmap_length = mbi->mmap_length; > } > > + info.mods = boot_mods; > + > + for ( i=0; i < info.nr_mods; i++ ) i = 0 > + boot_mods[i].early_mod = &mods[i]; > + > + /* map the last mb module for xen entry */ > + boot_mods[info.nr_mods].early_mod = &mods[info.nr_mods]; The comment is good, but note how this is just one extra iteration of the loop, (so use <= for the bound). ~Andew
On 30.08.2024 23:46, Daniel P. Smith wrote: > @@ -303,6 +308,14 @@ static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) > info.mmap_length = mbi->mmap_length; > } > > + info.mods = boot_mods; > + > + for ( i=0; i < info.nr_mods; i++ ) > + boot_mods[i].early_mod = &mods[i]; > + > + /* map the last mb module for xen entry */ Nit: Comment style. Jan
On 9/3/24 19:29, 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 e785ed1c5982..844262495962 100644 >> --- a/xen/arch/x86/include/asm/bootinfo.h >> +++ b/xen/arch/x86/include/asm/bootinfo.h >> @@ -8,10 +8,16 @@ >> #ifndef __XEN_X86_BOOTINFO_H__ >> #define __XEN_X86_BOOTINFO_H__ >> >> +#include <xen/multiboot.h> >> #include <xen/types.h> >> >> +struct boot_module { >> + module_t *early_mod; > > This could do with a /* Transitionary only */ comment. In this patch > it's not too bad, but it does get worse as new fields are added, before > being removed. Yep, can add a comment. > I'd also drop the "early_" part. I know it's the initial_images array > we're converting, but "early_" doesn't convey any extra meaning, and it > makes a number of lines get quite hairy. I can drop it. >> +}; >> + >> struct boot_info { >> unsigned int nr_mods; >> + struct boot_module *mods; > > struct boot_module modules[MAX_NR_BOOTMODS + 1]; > > Probably at the end of the structure. In turn it ... I can move it here, though just to be clear, are you suggesting that it is kept at the end of the structure as more fields are added. >> >> const char *boot_loader_name; >> const char *cmdline; >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >> index c6b45ced00ae..28fdbf4d4c2b 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -278,12 +278,17 @@ custom_param("acpi", parse_acpi_param); >> >> static const char *cmdline_cook(const char *p, const char *loader_name); >> >> +/* Max number of boot modules a bootloader can provide in addition to Xen */ >> +#define MAX_NR_BOOTMODS 63 >> + >> static const module_t *__initdata initial_images; >> static struct boot_info __initdata *boot_info; >> >> -static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) >> +static void __init multiboot_to_bootinfo(multiboot_info_t *mbi, module_t *mods) >> { >> static struct boot_info __initdata info; >> + static struct boot_module __initdata boot_mods[MAX_NR_BOOTMODS + 1]; > > ... drops this static. Will be dropped. >> + unsigned int i; >> >> info.nr_mods = mbi->mods_count; >> >> @@ -303,6 +308,14 @@ static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) >> info.mmap_length = mbi->mmap_length; >> } >> >> + info.mods = boot_mods; >> + >> + for ( i=0; i < info.nr_mods; i++ ) > > i = 0 Ack. >> + boot_mods[i].early_mod = &mods[i]; >> + >> + /* map the last mb module for xen entry */ >> + boot_mods[info.nr_mods].early_mod = &mods[info.nr_mods]; > > The comment is good, but note how this is just one extra iteration of > the loop, (so use <= for the bound). I will move the comment above the loop and adjust the condition. v/r, dps
On 9/4/24 02:33, Jan Beulich wrote: > On 30.08.2024 23:46, Daniel P. Smith wrote: >> @@ -303,6 +308,14 @@ static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) >> info.mmap_length = mbi->mmap_length; >> } >> >> + info.mods = boot_mods; >> + >> + for ( i=0; i < info.nr_mods; i++ ) >> + boot_mods[i].early_mod = &mods[i]; >> + >> + /* map the last mb module for xen entry */ > > Nit: Comment style. Ack, will fix as part of relocation per Andy's comment. v/r, dps
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h index e785ed1c5982..844262495962 100644 --- a/xen/arch/x86/include/asm/bootinfo.h +++ b/xen/arch/x86/include/asm/bootinfo.h @@ -8,10 +8,16 @@ #ifndef __XEN_X86_BOOTINFO_H__ #define __XEN_X86_BOOTINFO_H__ +#include <xen/multiboot.h> #include <xen/types.h> +struct boot_module { + module_t *early_mod; +}; + struct boot_info { unsigned int nr_mods; + struct boot_module *mods; const char *boot_loader_name; const char *cmdline; diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index c6b45ced00ae..28fdbf4d4c2b 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -278,12 +278,17 @@ custom_param("acpi", parse_acpi_param); static const char *cmdline_cook(const char *p, const char *loader_name); +/* Max number of boot modules a bootloader can provide in addition to Xen */ +#define MAX_NR_BOOTMODS 63 + static const module_t *__initdata initial_images; static struct boot_info __initdata *boot_info; -static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) +static void __init multiboot_to_bootinfo(multiboot_info_t *mbi, module_t *mods) { static struct boot_info __initdata info; + static struct boot_module __initdata boot_mods[MAX_NR_BOOTMODS + 1]; + unsigned int i; info.nr_mods = mbi->mods_count; @@ -303,6 +308,14 @@ static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) info.mmap_length = mbi->mmap_length; } + info.mods = boot_mods; + + for ( i=0; i < info.nr_mods; i++ ) + boot_mods[i].early_mod = &mods[i]; + + /* map the last mb module for xen entry */ + boot_mods[info.nr_mods].early_mod = &mods[info.nr_mods]; + boot_info = &info; } @@ -1062,7 +1075,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) mod = __va(mbi->mods_addr); } - multiboot_to_bootinfo(mbi); + multiboot_to_bootinfo(mbi, mod); if ( (kextra = strstr(boot_info->cmdline, " -- ")) != NULL ) { @@ -1164,7 +1177,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) bootsym(boot_edd_info_nr)); /* Check that we have at least one Multiboot module. */ - if ( !(mbi->flags & MBI_MODULES) || (boot_info->nr_mods == 0) ) + if ( boot_info->nr_mods == 0 ) panic("dom0 kernel not specified. Check bootloader configuration\n"); /* Check that we don't have a silly number of modules. */
This will introduce a new struct boot_module to provide a rich state representation around modules provided by the boot loader. Support is for 64 boot modules, one held in reserve for Xen, and up to 63 can be provided by the boot loader. The array of struct boot_modules will be accessible via a reference held in struct boot_info. A temporary `early_mod` parameter is included in struct boot_module to ease the transition from using Multiboot v1 structures over to struct boot_module. Once the transition is complete, the parameter will be dropped from the structure. Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> --- xen/arch/x86/include/asm/bootinfo.h | 6 ++++++ xen/arch/x86/setup.c | 19 ++++++++++++++++--- 2 files changed, 22 insertions(+), 3 deletions(-)