Re: [PATCH v2 3/4] x86/trampoline: Document how the trampoline is laid out

["Alejandro Vallejo" <alejandro.vallejo@cloud.com>]

On Thu Nov 14, 2024 at 6:34 PM GMT, Andrew Cooper wrote:
> On 14/11/2024 11:17 am, Andrew Cooper wrote:
> > On 14/11/2024 10:48 am, Alejandro Vallejo wrote:
> >> On Thu Nov 14, 2024 at 9:08 AM GMT, Andrew Cooper wrote:
> >>> diff --git a/xen/arch/x86/include/asm/trampoline.h b/xen/arch/x86/include/asm/trampoline.h
> >>> index 8c1e0b48c2c9..559111d2ccfc 100644
> >>> --- a/xen/arch/x86/include/asm/trampoline.h
> >>> +++ b/xen/arch/x86/include/asm/trampoline.h
> >>> @@ -37,12 +37,65 @@
> >>>   * 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
> >>> + *    arbitrary part of the the boot trampoline.  The stack is only used with
> >>> + *    paging disabled.
> >>> + *
> >>> + * 3) Boot trampoline:
> >>> + *
> >>> + *    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, is 4k
> >>> + *    in size, and only used with paging disabled.
> >>> + *
> >>> + * 4) Heap space:
> >>> + *
> >>> + *    The first 1k of heap space is statically allocated scratch space 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,
> >> Wh> + *    module list, etc).  It does so with a bump allocator starting from the
> >>> + *    end of the heap and allocating backwards.
> > Was this a typo replying to the email?
> >
> >
> >>> + *
> >>> + * 5) Boot stack:
> >>> + *
> >>> + *    The boot stack is 4k in size at the end of the trampoline, taking the
> >>> + *    total trampoline size to 64k.
> >>> + *
> >>> + * Therefore, when placed, it looks somewhat like this:
> >>> + *
> >>> + *    +--- trampoline_phys
> >>> + *    v
> >>> + *    |<-------------------------------64K------------------------------->|
> >>> + *    |<-----4K----->|                                         |<---4K--->|
> >>> + *    +-------+------+-+---------------------------------------+----------+
> >>> + *    | AP+S3 |  Boot  | Heap                                  |    Stack |
> >>> + *    +-------+------+-+---------------------------------------+----------+
> >>> + *    ^       ^   <~~^ ^                                    <~~^       <~~^
> >>> + *    |       |      | +- trampoline_end[]                     |          |
> >>> + *    |       |      +--- wakeup_stack      reloc() allocator -+          |
> >>> + *    |       +---------- trampoline_perm_end      Boot Stack ------------+
> >>> + *    +------------------ trampoline_start[]
> >>> + */
> >> Neat. Nothing like a pretty picture to properly explain things.
> >>
> >>> +
> >>>  #include <xen/compiler.h>
> >>>  #include <xen/types.h>
> >>>  
> >>>  /*
> >>> - * Start and end of the trampoline section, as linked into Xen.  It is within
> >>> - * the .init section and reclaimed after boot.
> >>> + * Start and end of the trampoline section, as linked into Xen.  This covers
> >>> + * the AP, S3 and Boot regions, but not the heap or stack.  It is within the
> >>> + * .init section and reclaimed after boot.
> >> How can it be reclaimed after boot if it's used for S3 wakeups? I assume you
> >> meant that the heap and stack are reclaimed after boot, but from that wording
> >> it sounds like the other way around.
> > This is the one bit that is slightly problematic to represent.
> >
> > trampoline_{start,end}[] describe the AP/S3/Boot text/data *in the Xen
> > image*, which is in the .init section.
> >
> > trampoline_phys is where trampoline_start[] ends up when placed.
> >
> > Maybe I should have "Note: trampoline_start[] and trampoline_end[]
> > represent the shown boundaries as linked in Xen" at the bottom of the
> > diagram?
>
> I'm going to go ahead and do this, and commit the series.
>
> ~Andrew

That note looks clearer, indeed.

Cheers,
Alejandro