Re: [PATCH 1/2] x86/trampoline: Document how the trampoline is laid out

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.11.2024 10:30, Andrew Cooper wrote:
> This is, to the best of my knowledge, accurate.  I am providing no comment on
> how sane I believe it to be.
> 
> At the time of writing, the sizes of the regions are:
> 
>           offset  size
>   AP:     0x0000  0x00b0
>   S3:     0x00b0  0x0140
>   Boot:   0x01f0  0x1780
>   Heap:   0x1970  0xe690
>   Stack:  0xf000  0x1000
> 
> and wakeup_stack overlays boot_edd_info.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> CC: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> CC: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> ---
>  xen/arch/x86/include/asm/trampoline.h | 55 ++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/include/asm/trampoline.h 
> b/xen/arch/x86/include/asm/trampoline.h
> index 8c1e0b48c2c9..d801bea400dc 100644
> --- a/xen/arch/x86/include/asm/trampoline.h
> +++ b/xen/arch/x86/include/asm/trampoline.h
> @@ -37,12 +37,63 @@
>   * manually as part of placement.
>   */
>  
> +/*
> + * Layout of the trampoline.  Logical areas, in ascending order:
> + *
> + * 1) AP boot:
> + *
> + *    The INIT-SIPI-SIPI entrypoint.  This logic is stack-less so the 
> identity
> + *    mapping (which must be executable) can at least be Read Only.
> + *
> + * 2) S3 resume:
> + *
> + *    The S3 wakeup logic may need to interact with the BIOS, so needs a
> + *    stack.  The stack pointer is set to trampoline_phys + 4k and clobbers 
> an
> + *    undefined part of the the boot trampoline.  The stack is only used with
> + *    paging disabled.
> + *
> + * 3) Boot trampoline:
> + *
> + *    This region houses various data used by the AP/S3 paths too.

This is confusing to have here - isn't the boot part (that isn't in the
same page as the tail of the AP/S3 region) being boot-time only, and hence
unavailable for S3 and post-boot AP bringup? Both here and with the numbers
in the description - what position did you use as separator between 2) and
3)?

Then again it may be just me who is confused: Didn't we, at some point, limit
the resident trampoline to just one page? Was that only a plan, or a patch
that never was committed?

>  The boot
> + *    trampoline collects data from the BIOS (E820/EDD/EDID/etc), so needs a
> + *    stack.  The stack pointer is set to trampoline_phys + 64k and has 4k
> + *    space reserved.
> + *
> + * 4) Heap space:
> + *
> + *    The first 1k of heap space is statically allocated for VESA 
> information.
> + *
> + *    The remainder of the heap is used by reloc(), logic which is otherwise
> + *    outside of the trampoline, to collect the bootloader metadata (cmdline,
> + *    module list, etc).  It does so with a bump allocator starting from the
> + *    end of the heap and allocating backwards.
> + *
> + * 5) Boot stack:
> + *
> + *    4k of space is reserved for the boot stack, at trampoline_phys + 64k.

Perhaps add "ending" to clarify it doesn't go beyond +64k? It's being expressed
...

> + * Therefore, when placed, it looks somewhat like this:
> + *
> + *    +--- trampoline_phys
> + *    v
> + *    |<-------------------------------64K------------------------------->|
> + *    |<-----4K----->|                                         |<---4K--->|
> + *    +----+----+----+-+---------------------------------------+----------+
> + *    | AP | S3 | Boot | Heap                                  |    Stack |
> + *    +----+----+------+---------------------------------------+----------+
> + *    ^           <~~^ ^                                    <~~^       <~~^
> + *    |              | +- trampoline_end[]                     |          |
> + *    |              +--- S3 Stack          reloc() allocator -+          |
> + *    +------------------ trampoline_start[]       Boot Stack ------------+
> + */

... by this scheme, but could still do with being made explicit, just to avoid
ambiguity or suspicion of things have gone stale again.

Jan