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:
>>> 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?

If by this you mean to then also cover what the first sentence of 3) said,
then yes, that might be preferable.

Jan