On Sat, Jul 8, 2023 at 11:30 AM Stefano Stabellini <[email protected]> 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 <[email protected]> > > Signed-off-by: Daniel P. Smith <[email protected]> > > > > --- > > 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 > > > > >
