Re: [PATCH v7 03/38] x86/boot: move headroom to boot modules

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/10/2024 1:45 am, Daniel P. Smith wrote:
> The purpose of struct boot_module is to encapsulate the state of boot module 
> as
> it is processed by Xen. Locating boot module state struct boot_module reduces
> the number of global variables as well as the number of state variables that
> must be passed around. It also lays the groundwork for hyperlaunch mult-domain
> construction, where multiple instances of state variables like headroom will 
> be
> needed.
>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> ---
> Changes since v6:
> - add blank line to separate comment from line above it
>
> Changes since v5:
> - reword and expand comment on headroom
> - consolidated declaration and assignment
> ---
>  xen/arch/x86/include/asm/bootinfo.h | 14 ++++++++++++++
>  xen/arch/x86/setup.c                | 21 ++++++++++++---------
>  2 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/x86/include/asm/bootinfo.h 
> b/xen/arch/x86/include/asm/bootinfo.h
> index ffa443406747..59e6696f9671 100644
> --- a/xen/arch/x86/include/asm/bootinfo.h
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -17,6 +17,20 @@
>  struct boot_module {
>      /* Transitionary only */
>      module_t *mod;
> +
> +    /*
> +     * A boot module may contain a compressed kernel that will require
> +     * additional space, before the module data, into which the kernel will 
> be
> +     * decompressed.

The grammar is a bit awkward.  Space beforehand is an implementation
choice of Xen, not a requirement.

Can I suggest:

---%<---
A boot module may need decompressing by Xen.  Headroom is an estimate of
the additional space required to decompress the module.

Headroom is accounted for at the start of the module.  Decompressing is
done in-place with input=start, output=start-headroom, expecting the
pointers to become equal (give or take some rounding) when decompression
is complete.
---%<---


> +     *
> +     * Memory layout at boot:
> +     *     [ compressed kernel ]
> +     * After boot module relocation:
> +     *     [ estimated headroom + PAGE_SIZE rounding ][ compressed kernel ]
> +     * After kernel decompression:
> +     *     [ decompressed kernel ][ unused rounding ]
> +     */

I'm not sure how helpful this is.  If anything, I'd say it ought to be:

                       start + size --+
          start --+                   |
                  v                   v
                  +-------------------+
                  | Compressed Module |
 +----------------+-------------------+
 |         Decompressed Module        |
 +----------------+-------------------+
 ^
 +-- start - headroom


~Andrew