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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.11.2024 12:19, Andrew Cooper wrote:
> On 13/11/2024 10:20 am, Jan Beulich wrote:
>> On 13.11.2024 10:30, Andrew Cooper wrote:
>>> --- 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 was fearing it might be as weak. We probably want to gain a symbol there,
for use in another linker script assertion.

Jan