Xen project Mailing List

On Sat, Jul 8, 2023 at 11:30 AM Stefano Stabellini <sstabellini@xxxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>
> ---
> 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
>
>

Re: [PATCH 01/10] x86 setup: move x86 boot module counting into a new boot_info struct