| Message ID | 20230701071835.41599-2-christopher.w.clark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | v3: Boot modules for Hyperlaunch | expand |
On Sat, 1 Jul 2023, Christopher Clark wrote: > An initial step towards a non-multiboot internal representation of boot > modules for common code, starting with x86 setup and converting the > fields that are accessed for the startup calculations. > > Introduce a new header, <xen/bootinfo.h>, and populate it with a new > boot_info structure initially containing a count of the number of boot > modules. > > The naming of the header, structure and fields is intended to respect > the boot structures on Arm -- see arm/include/asm/setup.h -- as part of > work towards aligning common architecture-neutral boot logic and > structures. Thanks for aligning the two archs. At some point we should also have ARM use the common headers. > No functional change intended. > > Signed-off-by: Christopher Clark <christopher.w.clark@gmail.com> > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > > --- > Changes since v1: patch is a subset of v1 series patches 2 and 3. > > xen/arch/x86/setup.c | 58 +++++++++++++++++++++++--------------- > xen/include/xen/bootinfo.h | 20 +++++++++++++ > 2 files changed, 55 insertions(+), 23 deletions(-) > create mode 100644 xen/include/xen/bootinfo.h > > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index 74e3915a4d..708639b236 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -1,3 +1,4 @@ > +#include <xen/bootinfo.h> > #include <xen/init.h> > #include <xen/lib.h> > #include <xen/err.h> > @@ -268,7 +269,16 @@ static int __init cf_check parse_acpi_param(const char *s) > custom_param("acpi", parse_acpi_param); > > static const module_t *__initdata initial_images; > -static unsigned int __initdata nr_initial_images; > +static struct boot_info __initdata *boot_info; Why can't this be not a pointer? > +static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) > +{ > + static struct boot_info __initdata info; Then we don't need this > + info.nr_mods = mbi->mods_count; > + > + boot_info = &info; And we could just do: boot_info.nr_mods = mbi->mods_count; ? > +} > > unsigned long __init initial_images_nrpages(nodeid_t node) > { > @@ -277,7 +287,7 @@ unsigned long __init initial_images_nrpages(nodeid_t node) > unsigned long nr; > unsigned int i; > > - for ( nr = i = 0; i < nr_initial_images; ++i ) > + for ( nr = i = 0; i < boot_info->nr_mods; ++i ) > { > unsigned long start = initial_images[i].mod_start; > unsigned long end = start + PFN_UP(initial_images[i].mod_end); > @@ -293,7 +303,7 @@ void __init discard_initial_images(void) > { > unsigned int i; > > - for ( i = 0; i < nr_initial_images; ++i ) > + for ( i = 0; i < boot_info->nr_mods; ++i ) > { > uint64_t start = (uint64_t)initial_images[i].mod_start << PAGE_SHIFT; > > @@ -301,7 +311,7 @@ void __init discard_initial_images(void) > start + PAGE_ALIGN(initial_images[i].mod_end)); > } > > - nr_initial_images = 0; > + boot_info->nr_mods = 0; > initial_images = NULL; > } > > @@ -1020,6 +1030,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) > mod = __va(mbi->mods_addr); > } > > + multiboot_to_bootinfo(mbi); > + > loader = (mbi->flags & MBI_LOADERNAME) > ? (char *)__va(mbi->boot_loader_name) : "unknown"; > > @@ -1127,18 +1139,18 @@ void __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) || (mbi->mods_count == 0) ) > + if ( !(mbi->flags & MBI_MODULES) || (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. */ > - if ( mbi->mods_count > sizeof(module_map) * 8 ) > + if ( boot_info->nr_mods > sizeof(module_map) * 8 ) > { > - mbi->mods_count = sizeof(module_map) * 8; > + boot_info->nr_mods = sizeof(module_map) * 8; > printk("Excessive multiboot modules - using the first %u only\n", > - mbi->mods_count); > + boot_info->nr_mods); > } > > - bitmap_fill(module_map, mbi->mods_count); > + bitmap_fill(module_map, boot_info->nr_mods); > __clear_bit(0, module_map); /* Dom0 kernel is always first */ > > if ( pvh_boot ) > @@ -1311,9 +1323,9 @@ void __init noreturn __start_xen(unsigned long mbi_p) > kexec_reserve_area(&boot_e820); > > initial_images = mod; > - nr_initial_images = mbi->mods_count; > + boot_info->nr_mods = boot_info->nr_mods; > > - for ( i = 0; !efi_enabled(EFI_LOADER) && i < mbi->mods_count; i++ ) > + for ( i = 0; !efi_enabled(EFI_LOADER) && i < boot_info->nr_mods; i++ ) > { > if ( mod[i].mod_start & (PAGE_SIZE - 1) ) > panic("Bootloader didn't honor module alignment request\n"); > @@ -1337,8 +1349,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) > * respective reserve_e820_ram() invocation below. No need to > * query efi_boot_mem_unused() here, though. > */ > - mod[mbi->mods_count].mod_start = virt_to_mfn(_stext); > - mod[mbi->mods_count].mod_end = __2M_rwdata_end - _stext; > + mod[boot_info->nr_mods].mod_start = virt_to_mfn(_stext); > + mod[boot_info->nr_mods].mod_end = __2M_rwdata_end - _stext; > } > > modules_headroom = bzimage_headroom(bootstrap_map(mod), mod->mod_end); > @@ -1398,7 +1410,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) > { > /* Don't overlap with modules. */ > end = consider_modules(s, e, reloc_size + mask, > - mod, mbi->mods_count, -1); > + mod, boot_info->nr_mods, -1); > end &= ~mask; > } > else > @@ -1419,7 +1431,7 @@ void __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-- ) > + for ( j = boot_info->nr_mods - 1; j >= 0; j-- ) > { > unsigned long headroom = j ? 0 : modules_headroom; > unsigned long size = PAGE_ALIGN(headroom + mod[j].mod_end); > @@ -1429,7 +1441,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) > > /* Don't overlap with other modules (or Xen itself). */ > end = consider_modules(s, e, size, mod, > - mbi->mods_count + relocated, j); > + boot_info->nr_mods + relocated, j); > > if ( highmem_start && end > highmem_start ) > continue; > @@ -1456,7 +1468,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) > { > /* Don't overlap with modules (or Xen itself). */ > e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size), mod, > - mbi->mods_count + relocated, -1); > + boot_info->nr_mods + relocated, -1); > if ( s >= e ) > break; > if ( e > kexec_crash_area_limit ) > @@ -1471,7 +1483,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) > > if ( modules_headroom && !mod->reserved ) > panic("Not enough memory to relocate the dom0 kernel image\n"); > - for ( i = 0; i < mbi->mods_count; ++i ) > + for ( i = 0; i < boot_info->nr_mods; ++i ) > { > uint64_t s = (uint64_t)mod[i].mod_start << PAGE_SHIFT; > > @@ -1540,7 +1552,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) > ASSERT(j); > } > map_e = boot_e820.map[j].addr + boot_e820.map[j].size; > - for ( j = 0; j < mbi->mods_count; ++j ) > + for ( j = 0; j < boot_info->nr_mods; ++j ) > { > uint64_t end = pfn_to_paddr(mod[j].mod_start) + > mod[j].mod_end; > @@ -1616,7 +1628,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) > } > } > > - for ( i = 0; i < mbi->mods_count; ++i ) > + for ( i = 0; i < boot_info->nr_mods; ++i ) > { > set_pdx_range(mod[i].mod_start, > mod[i].mod_start + PFN_UP(mod[i].mod_end)); > @@ -1999,8 +2011,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) > cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ", > cpu_has_nx ? "" : "not "); > > - initrdidx = find_first_bit(module_map, mbi->mods_count); > - if ( bitmap_weight(module_map, mbi->mods_count) > 1 ) > + initrdidx = find_first_bit(module_map, boot_info->nr_mods); > + if ( bitmap_weight(module_map, boot_info->nr_mods) > 1 ) > printk(XENLOG_WARNING > "Multiple initrd candidates, picking module #%u\n", > initrdidx); > @@ -2010,7 +2022,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) > * above our heap. The second module, if present, is an initrd ramdisk. > */ > dom0 = create_dom0(mod, modules_headroom, > - initrdidx < mbi->mods_count ? mod + initrdidx : NULL, > + initrdidx < boot_info->nr_mods ? mod + initrdidx : NULL, > kextra, loader); > if ( !dom0 ) > panic("Could not set up DOM0 guest OS\n"); > diff --git a/xen/include/xen/bootinfo.h b/xen/include/xen/bootinfo.h > new file mode 100644 > index 0000000000..6a7d55d20e > --- /dev/null > +++ b/xen/include/xen/bootinfo.h > @@ -0,0 +1,20 @@ > +#ifndef __XEN_BOOTINFO_H__ > +#define __XEN_BOOTINFO_H__ > + > +#include <xen/types.h> I don't think you need types.h right now > +struct boot_info { This is what we call struct bootmodules on ARM right? Would it help if we used the same name? I am not asking to make the ARM code common because I think that would probably be a lot more work. > + unsigned int nr_mods; > +}; > + > +#endif > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * tab-width: 4 > + * indent-tabs-mode: nil > + * End: > + */ > -- > 2.25.1 > >
On Sat, Jul 8, 2023 at 11:30 AM Stefano Stabellini <sstabellini@kernel.org> wrote: > On Sat, 1 Jul 2023, Christopher Clark wrote: > > An initial step towards a non-multiboot internal representation of boot > > modules for common code, starting with x86 setup and converting the > > fields that are accessed for the startup calculations. > > > > Introduce a new header, <xen/bootinfo.h>, and populate it with a new > > boot_info structure initially containing a count of the number of boot > > modules. > > > > The naming of the header, structure and fields is intended to respect > > the boot structures on Arm -- see arm/include/asm/setup.h -- as part of > > work towards aligning common architecture-neutral boot logic and > > structures. > > Thanks for aligning the two archs. At some point we should also have ARM > use the common headers. > > > > No functional change intended. > > > > Signed-off-by: Christopher Clark <christopher.w.clark@gmail.com> > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > > > > --- > > Changes since v1: patch is a subset of v1 series patches 2 and 3. > > > > xen/arch/x86/setup.c | 58 +++++++++++++++++++++++--------------- > > xen/include/xen/bootinfo.h | 20 +++++++++++++ > > 2 files changed, 55 insertions(+), 23 deletions(-) > > create mode 100644 xen/include/xen/bootinfo.h > > > > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > > index 74e3915a4d..708639b236 100644 > > --- a/xen/arch/x86/setup.c > > +++ b/xen/arch/x86/setup.c > > @@ -1,3 +1,4 @@ > > +#include <xen/bootinfo.h> > > #include <xen/init.h> > > #include <xen/lib.h> > > #include <xen/err.h> > > @@ -268,7 +269,16 @@ static int __init cf_check parse_acpi_param(const > char *s) > > custom_param("acpi", parse_acpi_param); > > > > static const module_t *__initdata initial_images; > > -static unsigned int __initdata nr_initial_images; > > +static struct boot_info __initdata *boot_info; > > Why can't this be not a pointer? > In a later patch (10/10 in the same series posted), the boot_info pointer is passed as an argument to start_xen. On x86 there are currently three different entry points to this that have different environments which must all be made to behave the same, and passing the argument as a pointer is a lowest-common-denominater due to the 32bit x86 multiboot entry point. Additionally another entry point will be coming soon for TrenchBoot. Defining it as a pointer now where this logic is introduced saves having to do a conversion of all accesses when the later change is made. I can add a note about this to the commit message. > > > > +static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) > > +{ > > + static struct boot_info __initdata info; > > Then we don't need this > (see above) > > > > + info.nr_mods = mbi->mods_count; > > + > > + boot_info = &info; > > And we could just do: > > boot_info.nr_mods = mbi->mods_count; > > ? > (see above) > > > > +} > > > > unsigned long __init initial_images_nrpages(nodeid_t node) > > { > > @@ -277,7 +287,7 @@ unsigned long __init initial_images_nrpages(nodeid_t > node) > > unsigned long nr; > > unsigned int i; > > > > - for ( nr = i = 0; i < nr_initial_images; ++i ) > > + for ( nr = i = 0; i < boot_info->nr_mods; ++i ) > > { > > unsigned long start = initial_images[i].mod_start; > > unsigned long end = start + PFN_UP(initial_images[i].mod_end); > > @@ -293,7 +303,7 @@ void __init discard_initial_images(void) > > { > > unsigned int i; > > > > - for ( i = 0; i < nr_initial_images; ++i ) > > + for ( i = 0; i < boot_info->nr_mods; ++i ) > > { > > uint64_t start = (uint64_t)initial_images[i].mod_start << > PAGE_SHIFT; > > > > @@ -301,7 +311,7 @@ void __init discard_initial_images(void) > > start + > PAGE_ALIGN(initial_images[i].mod_end)); > > } > > > > - nr_initial_images = 0; > > + boot_info->nr_mods = 0; > > initial_images = NULL; > > } > > > > @@ -1020,6 +1030,8 @@ void __init noreturn __start_xen(unsigned long > mbi_p) > > mod = __va(mbi->mods_addr); > > } > > > > + multiboot_to_bootinfo(mbi); > > + > > loader = (mbi->flags & MBI_LOADERNAME) > > ? (char *)__va(mbi->boot_loader_name) : "unknown"; > > > > @@ -1127,18 +1139,18 @@ void __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) || (mbi->mods_count == 0) ) > > + if ( !(mbi->flags & MBI_MODULES) || (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. */ > > - if ( mbi->mods_count > sizeof(module_map) * 8 ) > > + if ( boot_info->nr_mods > sizeof(module_map) * 8 ) > > { > > - mbi->mods_count = sizeof(module_map) * 8; > > + boot_info->nr_mods = sizeof(module_map) * 8; > > printk("Excessive multiboot modules - using the first %u > only\n", > > - mbi->mods_count); > > + boot_info->nr_mods); > > } > > > > - bitmap_fill(module_map, mbi->mods_count); > > + bitmap_fill(module_map, boot_info->nr_mods); > > __clear_bit(0, module_map); /* Dom0 kernel is always first */ > > > > if ( pvh_boot ) > > @@ -1311,9 +1323,9 @@ void __init noreturn __start_xen(unsigned long > mbi_p) > > kexec_reserve_area(&boot_e820); > > > > initial_images = mod; > > - nr_initial_images = mbi->mods_count; > > + boot_info->nr_mods = boot_info->nr_mods; > > > > - for ( i = 0; !efi_enabled(EFI_LOADER) && i < mbi->mods_count; i++ ) > > + for ( i = 0; !efi_enabled(EFI_LOADER) && i < boot_info->nr_mods; > i++ ) > > { > > if ( mod[i].mod_start & (PAGE_SIZE - 1) ) > > panic("Bootloader didn't honor module alignment request\n"); > > @@ -1337,8 +1349,8 @@ void __init noreturn __start_xen(unsigned long > mbi_p) > > * respective reserve_e820_ram() invocation below. No need to > > * query efi_boot_mem_unused() here, though. > > */ > > - mod[mbi->mods_count].mod_start = virt_to_mfn(_stext); > > - mod[mbi->mods_count].mod_end = __2M_rwdata_end - _stext; > > + mod[boot_info->nr_mods].mod_start = virt_to_mfn(_stext); > > + mod[boot_info->nr_mods].mod_end = __2M_rwdata_end - _stext; > > } > > > > modules_headroom = bzimage_headroom(bootstrap_map(mod), > mod->mod_end); > > @@ -1398,7 +1410,7 @@ void __init noreturn __start_xen(unsigned long > mbi_p) > > { > > /* Don't overlap with modules. */ > > end = consider_modules(s, e, reloc_size + mask, > > - mod, mbi->mods_count, -1); > > + mod, boot_info->nr_mods, -1); > > end &= ~mask; > > } > > else > > @@ -1419,7 +1431,7 @@ void __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-- ) > > + for ( j = boot_info->nr_mods - 1; j >= 0; j-- ) > > { > > unsigned long headroom = j ? 0 : modules_headroom; > > unsigned long size = PAGE_ALIGN(headroom + mod[j].mod_end); > > @@ -1429,7 +1441,7 @@ void __init noreturn __start_xen(unsigned long > mbi_p) > > > > /* Don't overlap with other modules (or Xen itself). */ > > end = consider_modules(s, e, size, mod, > > - mbi->mods_count + relocated, j); > > + boot_info->nr_mods + relocated, j); > > > > if ( highmem_start && end > highmem_start ) > > continue; > > @@ -1456,7 +1468,7 @@ void __init noreturn __start_xen(unsigned long > mbi_p) > > { > > /* Don't overlap with modules (or Xen itself). */ > > e = consider_modules(s, e, > PAGE_ALIGN(kexec_crash_area.size), mod, > > - mbi->mods_count + relocated, -1); > > + boot_info->nr_mods + relocated, -1); > > if ( s >= e ) > > break; > > if ( e > kexec_crash_area_limit ) > > @@ -1471,7 +1483,7 @@ void __init noreturn __start_xen(unsigned long > mbi_p) > > > > if ( modules_headroom && !mod->reserved ) > > panic("Not enough memory to relocate the dom0 kernel image\n"); > > - for ( i = 0; i < mbi->mods_count; ++i ) > > + for ( i = 0; i < boot_info->nr_mods; ++i ) > > { > > uint64_t s = (uint64_t)mod[i].mod_start << PAGE_SHIFT; > > > > @@ -1540,7 +1552,7 @@ void __init noreturn __start_xen(unsigned long > mbi_p) > > ASSERT(j); > > } > > map_e = boot_e820.map[j].addr + boot_e820.map[j].size; > > - for ( j = 0; j < mbi->mods_count; ++j ) > > + for ( j = 0; j < boot_info->nr_mods; ++j ) > > { > > uint64_t end = pfn_to_paddr(mod[j].mod_start) + > > mod[j].mod_end; > > @@ -1616,7 +1628,7 @@ void __init noreturn __start_xen(unsigned long > mbi_p) > > } > > } > > > > - for ( i = 0; i < mbi->mods_count; ++i ) > > + for ( i = 0; i < boot_info->nr_mods; ++i ) > > { > > set_pdx_range(mod[i].mod_start, > > mod[i].mod_start + PFN_UP(mod[i].mod_end)); > > @@ -1999,8 +2011,8 @@ void __init noreturn __start_xen(unsigned long > mbi_p) > > cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ", > > cpu_has_nx ? "" : "not "); > > > > - initrdidx = find_first_bit(module_map, mbi->mods_count); > > - if ( bitmap_weight(module_map, mbi->mods_count) > 1 ) > > + initrdidx = find_first_bit(module_map, boot_info->nr_mods); > > + if ( bitmap_weight(module_map, boot_info->nr_mods) > 1 ) > > printk(XENLOG_WARNING > > "Multiple initrd candidates, picking module #%u\n", > > initrdidx); > > @@ -2010,7 +2022,7 @@ void __init noreturn __start_xen(unsigned long > mbi_p) > > * above our heap. The second module, if present, is an initrd > ramdisk. > > */ > > dom0 = create_dom0(mod, modules_headroom, > > - initrdidx < mbi->mods_count ? mod + initrdidx : > NULL, > > + initrdidx < boot_info->nr_mods ? mod + initrdidx > : NULL, > > kextra, loader); > > if ( !dom0 ) > > panic("Could not set up DOM0 guest OS\n"); > > diff --git a/xen/include/xen/bootinfo.h b/xen/include/xen/bootinfo.h > > new file mode 100644 > > index 0000000000..6a7d55d20e > > --- /dev/null > > +++ b/xen/include/xen/bootinfo.h > > @@ -0,0 +1,20 @@ > > +#ifndef __XEN_BOOTINFO_H__ > > +#define __XEN_BOOTINFO_H__ > > + > > +#include <xen/types.h> > > I don't think you need types.h right now > Ack - thanks > > > > +struct boot_info { > > This is what we call struct bootmodules on ARM right? Would it help if > we used the same name? > > I am not asking to make the ARM code common because I think that would > probably be a lot more work. > It becomes clearer to see by the end of the full hyperlaunch v1 series with the domain builder implemented, but it is also evident by the end of this series: the core/common boot info for Xen is more than just a set of bootmodules. This first patch is part of adding functionality to common incrementally, as a starting point, and reducing this boot info to just a bootmodules structure is going to be limiting it in this context. Christopher > > > > + unsigned int nr_mods; > > +}; > > + > > +#endif > > + > > +/* > > + * Local variables: > > + * mode: C > > + * c-file-style: "BSD" > > + * c-basic-offset: 4 > > + * tab-width: 4 > > + * indent-tabs-mode: nil > > + * End: > > + */ > > -- > > 2.25.1 > > > > >
(You might want to check your email settings because it looks like you sent an html email) On Thu, 20 Jul 2023, Christopher Clark wrote: > > +struct boot_info { > > This is what we call struct bootmodules on ARM right? Would it help if > we used the same name? > > I am not asking to make the ARM code common because I think that would > probably be a lot more work. This comment was wrong > It becomes clearer to see by the end of the full hyperlaunch v1 series with the domain builder implemented, but it is also evident by the > end of this series: the core/common boot info for Xen is more than just a set of bootmodules. This first patch is part of adding > functionality to common incrementally, as a starting point, and reducing this boot info to just a bootmodules structure is going to be > limiting it in this context. After having read the whole series, it is clear that you made such a fantastic progress toward unifying all the interfaces, both ARM and x86. You managed to introduce interfaces so similar to the existing ARM interfaces, that they are almost the same already. This is way better than I expected when I wrote that comment to the first patch. I think we should go the extra mile and move the ARM interfaces to common, and make any changes needed by x86 there in common and reflecting the changes back to ARM. This will also allow us to move more dom0less init code from ARM to common with fewer changes later on.
On 01.07.2023 09:18, Christopher Clark wrote: > @@ -1127,18 +1139,18 @@ void __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) || (mbi->mods_count == 0) ) > + if ( !(mbi->flags & MBI_MODULES) || (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. */ > - if ( mbi->mods_count > sizeof(module_map) * 8 ) > + if ( boot_info->nr_mods > sizeof(module_map) * 8 ) > { > - mbi->mods_count = sizeof(module_map) * 8; > + boot_info->nr_mods = sizeof(module_map) * 8; As long as you don't replace all consumers of mbi->mods_count (see in particular the call trees down from early_microcode_init() and down from xsm_multiboot_init()), I don't think you can drop the original assignment. > printk("Excessive multiboot modules - using the first %u only\n", > - mbi->mods_count); > + boot_info->nr_mods); > } > > - bitmap_fill(module_map, mbi->mods_count); > + bitmap_fill(module_map, boot_info->nr_mods); > __clear_bit(0, module_map); /* Dom0 kernel is always first */ > > if ( pvh_boot ) > @@ -1311,9 +1323,9 @@ void __init noreturn __start_xen(unsigned long mbi_p) > kexec_reserve_area(&boot_e820); > > initial_images = mod; > - nr_initial_images = mbi->mods_count; > + boot_info->nr_mods = boot_info->nr_mods; Overly mechanical change? To prevent such going unnoticed, maybe boot_info should be pointer-to-const? Would of course require the bounding to occur earlier, so you truly don't need to write the struct after filling it in multiboot_to_bootinfo(). Or you move the call to that function down a little. Yet other options also exist. > - for ( i = 0; !efi_enabled(EFI_LOADER) && i < mbi->mods_count; i++ ) > + for ( i = 0; !efi_enabled(EFI_LOADER) && i < boot_info->nr_mods; i++ ) Seeing all sorts of changes of this kind - did you consider naming the pointer variable (which will become a function parameter as to what I understood from your reply to Stefano) just "bi"? (I wouldn't suggest this if the variable was to remain file-scope.) Jan
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 74e3915a4d..708639b236 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1,3 +1,4 @@ +#include <xen/bootinfo.h> #include <xen/init.h> #include <xen/lib.h> #include <xen/err.h> @@ -268,7 +269,16 @@ static int __init cf_check parse_acpi_param(const char *s) custom_param("acpi", parse_acpi_param); static const module_t *__initdata initial_images; -static unsigned int __initdata nr_initial_images; +static struct boot_info __initdata *boot_info; + +static void __init multiboot_to_bootinfo(multiboot_info_t *mbi) +{ + static struct boot_info __initdata info; + + info.nr_mods = mbi->mods_count; + + boot_info = &info; +} unsigned long __init initial_images_nrpages(nodeid_t node) { @@ -277,7 +287,7 @@ unsigned long __init initial_images_nrpages(nodeid_t node) unsigned long nr; unsigned int i; - for ( nr = i = 0; i < nr_initial_images; ++i ) + for ( nr = i = 0; i < boot_info->nr_mods; ++i ) { unsigned long start = initial_images[i].mod_start; unsigned long end = start + PFN_UP(initial_images[i].mod_end); @@ -293,7 +303,7 @@ void __init discard_initial_images(void) { unsigned int i; - for ( i = 0; i < nr_initial_images; ++i ) + for ( i = 0; i < boot_info->nr_mods; ++i ) { uint64_t start = (uint64_t)initial_images[i].mod_start << PAGE_SHIFT; @@ -301,7 +311,7 @@ void __init discard_initial_images(void) start + PAGE_ALIGN(initial_images[i].mod_end)); } - nr_initial_images = 0; + boot_info->nr_mods = 0; initial_images = NULL; } @@ -1020,6 +1030,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) mod = __va(mbi->mods_addr); } + multiboot_to_bootinfo(mbi); + loader = (mbi->flags & MBI_LOADERNAME) ? (char *)__va(mbi->boot_loader_name) : "unknown"; @@ -1127,18 +1139,18 @@ void __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) || (mbi->mods_count == 0) ) + if ( !(mbi->flags & MBI_MODULES) || (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. */ - if ( mbi->mods_count > sizeof(module_map) * 8 ) + if ( boot_info->nr_mods > sizeof(module_map) * 8 ) { - mbi->mods_count = sizeof(module_map) * 8; + boot_info->nr_mods = sizeof(module_map) * 8; printk("Excessive multiboot modules - using the first %u only\n", - mbi->mods_count); + boot_info->nr_mods); } - bitmap_fill(module_map, mbi->mods_count); + bitmap_fill(module_map, boot_info->nr_mods); __clear_bit(0, module_map); /* Dom0 kernel is always first */ if ( pvh_boot ) @@ -1311,9 +1323,9 @@ void __init noreturn __start_xen(unsigned long mbi_p) kexec_reserve_area(&boot_e820); initial_images = mod; - nr_initial_images = mbi->mods_count; + boot_info->nr_mods = boot_info->nr_mods; - for ( i = 0; !efi_enabled(EFI_LOADER) && i < mbi->mods_count; i++ ) + for ( i = 0; !efi_enabled(EFI_LOADER) && i < boot_info->nr_mods; i++ ) { if ( mod[i].mod_start & (PAGE_SIZE - 1) ) panic("Bootloader didn't honor module alignment request\n"); @@ -1337,8 +1349,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) * respective reserve_e820_ram() invocation below. No need to * query efi_boot_mem_unused() here, though. */ - mod[mbi->mods_count].mod_start = virt_to_mfn(_stext); - mod[mbi->mods_count].mod_end = __2M_rwdata_end - _stext; + mod[boot_info->nr_mods].mod_start = virt_to_mfn(_stext); + mod[boot_info->nr_mods].mod_end = __2M_rwdata_end - _stext; } modules_headroom = bzimage_headroom(bootstrap_map(mod), mod->mod_end); @@ -1398,7 +1410,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) { /* Don't overlap with modules. */ end = consider_modules(s, e, reloc_size + mask, - mod, mbi->mods_count, -1); + mod, boot_info->nr_mods, -1); end &= ~mask; } else @@ -1419,7 +1431,7 @@ void __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-- ) + for ( j = boot_info->nr_mods - 1; j >= 0; j-- ) { unsigned long headroom = j ? 0 : modules_headroom; unsigned long size = PAGE_ALIGN(headroom + mod[j].mod_end); @@ -1429,7 +1441,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) /* Don't overlap with other modules (or Xen itself). */ end = consider_modules(s, e, size, mod, - mbi->mods_count + relocated, j); + boot_info->nr_mods + relocated, j); if ( highmem_start && end > highmem_start ) continue; @@ -1456,7 +1468,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) { /* Don't overlap with modules (or Xen itself). */ e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size), mod, - mbi->mods_count + relocated, -1); + boot_info->nr_mods + relocated, -1); if ( s >= e ) break; if ( e > kexec_crash_area_limit ) @@ -1471,7 +1483,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) if ( modules_headroom && !mod->reserved ) panic("Not enough memory to relocate the dom0 kernel image\n"); - for ( i = 0; i < mbi->mods_count; ++i ) + for ( i = 0; i < boot_info->nr_mods; ++i ) { uint64_t s = (uint64_t)mod[i].mod_start << PAGE_SHIFT; @@ -1540,7 +1552,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) ASSERT(j); } map_e = boot_e820.map[j].addr + boot_e820.map[j].size; - for ( j = 0; j < mbi->mods_count; ++j ) + for ( j = 0; j < boot_info->nr_mods; ++j ) { uint64_t end = pfn_to_paddr(mod[j].mod_start) + mod[j].mod_end; @@ -1616,7 +1628,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) } } - for ( i = 0; i < mbi->mods_count; ++i ) + for ( i = 0; i < boot_info->nr_mods; ++i ) { set_pdx_range(mod[i].mod_start, mod[i].mod_start + PFN_UP(mod[i].mod_end)); @@ -1999,8 +2011,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ", cpu_has_nx ? "" : "not "); - initrdidx = find_first_bit(module_map, mbi->mods_count); - if ( bitmap_weight(module_map, mbi->mods_count) > 1 ) + initrdidx = find_first_bit(module_map, boot_info->nr_mods); + if ( bitmap_weight(module_map, boot_info->nr_mods) > 1 ) printk(XENLOG_WARNING "Multiple initrd candidates, picking module #%u\n", initrdidx); @@ -2010,7 +2022,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) * above our heap. The second module, if present, is an initrd ramdisk. */ dom0 = create_dom0(mod, modules_headroom, - initrdidx < mbi->mods_count ? mod + initrdidx : NULL, + initrdidx < boot_info->nr_mods ? mod + initrdidx : NULL, kextra, loader); if ( !dom0 ) panic("Could not set up DOM0 guest OS\n"); diff --git a/xen/include/xen/bootinfo.h b/xen/include/xen/bootinfo.h new file mode 100644 index 0000000000..6a7d55d20e --- /dev/null +++ b/xen/include/xen/bootinfo.h @@ -0,0 +1,20 @@ +#ifndef __XEN_BOOTINFO_H__ +#define __XEN_BOOTINFO_H__ + +#include <xen/types.h> + +struct boot_info { + unsigned int nr_mods; +}; + +#endif + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */