Re: [PATCH 02/10] x86 setup: per-arch bootmodule structure, headroom field

[Christopher Clark <christopher.w.clark@xxxxxxxxx>]

On Sat, Jul 8, 2023 at 12:15 PM Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:

On Sat, 1 Jul 2023, Christopher Clark wrote:
> Next step in incremental work towards adding a non-multiboot internal
> representation of boot modules, converting the fields being accessed for
> the startup calculations.
>
> Add a new array of structs for per-boot-module state, though only
> allocate space for a single array entry in this change since that is all
> that is required for functionality modified in this patch: moving the
> headroom field for the image decompression calculation into a new
> per-arch boot module struct and then using it in x86 dom0 construction.
>
> Introduces a per-arch header for x86 for arch-specific boot module
> structures, and add a member to the common boot module structure for
> access to it.
>
> No functional change intended.
>
> Signed-off-by: Christopher Clark <christopher.w.clark@xxxxxxxxx>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>

[...]


> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
> new file mode 100644
> index 0000000000..a25054f372
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -0,0 +1,18 @@
> +#ifndef __ARCH_X86_BOOTINFO_H__
> +#define __ARCH_X86_BOOTINFO_H__
> +
> +struct arch_bootmodule {
> +    unsigned headroom;
> +};
> +
> +#endif
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

[...]

> diff --git a/xen/include/xen/bootinfo.h b/xen/include/xen/bootinfo.h
> index 6a7d55d20e..b72ae31a66 100644
> --- a/xen/include/xen/bootinfo.h
> +++ b/xen/include/xen/bootinfo.h
> @@ -3,8 +3,19 @@
> 
>  #include <xen/types.h>
> 
> +#ifdef CONFIG_X86
> +#include <asm/bootinfo.h>
> +#else
> +    struct arch_bootmodule { };
> +#endif
> +
> +struct boot_module {
> +    struct arch_bootmodule *arch;
> +};
> +
>  struct boot_info {
>      unsigned int nr_mods;
> +    struct boot_module *mods;

Also here you already made the effort of using the same data structures
we use on ARM, you might as well use the same names too. Otherwise when
we try to use them on ARM it will require a rename somewhere.

Thanks for the review. We consciously made an effort to derive from the Arm data structures to ensure that we're able to support the logic that Arm needs. Arm's bootmodules were a

good start as a means for abstraction, and the design for hyperlaunch was striving to abstract even further to incorporate x86-ism and hopefully enough foresight for PPC and Risc-V.

Christopher

>  };
> 
>  #endif
> --
> 2.25.1
>
>