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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/11/2024 10:20 am, Jan Beulich wrote:
> 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 positioning of various things is rather complicated.

Only a single 4k page is mapped into idle_pg_table[].

But, the AP/S3 path use:
  trampoline_cpu_started
  idt_48
  gdt_48
  trampoline_xen_phys_start
  trampoline_misc_enable_off
  trampoline_efer

Which is beyond the content of wakeup.S.  The GDT in particular needs to
stay valid with paging enabled, to load __HYPERVISOR_CS.

We have /* From here on early boot only. */ in trampoline.S but that
seems to be the extent of checking.  Everything needed for AP/S3 is in
the first 0x229.

I'm open to suggestions for how to describe this better, although the
left hand side of the diagram is already very busy.

I suppose I could do AP+S3 as a single section, along their combined data?

>
>>  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
> ...

Ah yes.  That ended up less clear than I was intending.  I'll adjust.

~Andrew