[PATCH 0/2] x86/trampoline: Layout description improvements.

[PATCH 0/2] x86/trampoline: Layout description improvements.

[Andrew Cooper]

Andrew Cooper (2):
  x86/trampoline: Document how the trampoline is laid out
  x86/trampoline: Rationalise the constants to describe the size

 xen/arch/x86/boot/head.S              | 21 +---------
 xen/arch/x86/boot/reloc.c             |  5 +--
 xen/arch/x86/efi/efi-boot.h           |  2 +-
 xen/arch/x86/include/asm/config.h     |  5 ++-
 xen/arch/x86/include/asm/trampoline.h | 55 ++++++++++++++++++++++++++-
 xen/arch/x86/xen.lds.S                |  2 +-
 6 files changed, 62 insertions(+), 28 deletions(-)


base-commit: 38febce5b35a7a4391906499b2046f01ec0129ca
-- 
2.39.5

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

[Andrew Cooper]

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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>
CC: Frediano Ziglio <frediano.ziglio@cloud.com>
CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 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.  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.
+ *
+ * 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 ------------+
+ */
+
 #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.
  */
 /* SAF-0-safe */
 extern char trampoline_start[], trampoline_end[];

base-commit: 38febce5b35a7a4391906499b2046f01ec0129ca
-- 
2.39.5

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

[Jan Beulich]

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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> CC: Frediano Ziglio <frediano.ziglio@cloud.com>
> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  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

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

[Andrew Cooper]

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@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
>> CC: Frediano Ziglio <frediano.ziglio@cloud.com>
>> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>> ---
>>  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

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

[Frediano Ziglio]

On Wed, Nov 13, 2024 at 11:19 AM Andrew Cooper
<andrew.cooper3@citrix.com> 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@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> >> CC: Frediano Ziglio <frediano.ziglio@cloud.com>
> >> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> >> ---
> >>  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.
>

With that

Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.com>

Frediano

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

[Jan Beulich]

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@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
>>> CC: Frediano Ziglio <frediano.ziglio@cloud.com>
>>> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>>> ---
>>>  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

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

[Jan Beulich]

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

[PATCH 2/2] x86/trampoline: Rationalise the constants to describe the size

[Andrew Cooper]

The logic is far more sane to follow with a total size, and the position of
the end of the heap.  Remove or fix the the remaining descriptions of how the
trampoline is laid out.

No functional change.  The compiled binary is identical.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>
CC: Frediano Ziglio <frediano.ziglio@cloud.com>
CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/arch/x86/boot/head.S          | 21 ++-------------------
 xen/arch/x86/boot/reloc.c         |  5 ++---
 xen/arch/x86/efi/efi-boot.h       |  2 +-
 xen/arch/x86/include/asm/config.h |  5 +++--
 xen/arch/x86/xen.lds.S            |  2 +-
 5 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index dcda91cfda49..b31cf83758c1 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -494,7 +494,7 @@ trampoline_bios_setup:
 
 2:
         /* Reserve memory for the trampoline and the low-memory stack. */
-        sub     $((TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE)>>4),%ecx
+        sub     $TRAMPOLINE_SIZE >> 4, %ecx
 
         /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */
         xor     %cl, %cl
@@ -525,23 +525,6 @@ trampoline_setup:
         mov     %eax, sym_esi(multiboot_ptr)
 2:
 
-        /*
-         * Now trampoline_phys points to the following structure (lowest address
-         * is at the bottom):
-         *
-         * +------------------------+
-         * | TRAMPOLINE_STACK_SPACE |
-         * +------------------------+
-         * |     Data (MBI / PVH)   |
-         * +- - - - - - - - - - - - +
-         * |    TRAMPOLINE_SPACE    |
-         * +------------------------+
-         *
-         * Data grows downwards from the highest address of TRAMPOLINE_SPACE
-         * region to the end of the trampoline. The rest of TRAMPOLINE_SPACE is
-         * reserved for trampoline code and data.
-         */
-
         /* Interrogate CPU extended features via CPUID. */
         mov     $1, %eax
         cpuid
@@ -713,7 +696,7 @@ trampoline_setup:
 1:
         /* Switch to low-memory stack which lives at the end of trampoline region. */
         mov     sym_esi(trampoline_phys), %edi
-        lea     TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE(%edi),%esp
+        lea     TRAMPOLINE_SIZE(%edi), %esp
         lea     trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
         pushl   $BOOT_CS32
         push    %eax
diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index e50e161b2740..1f47e10f7fa6 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -65,7 +65,7 @@ typedef struct memctx {
     /*
      * Simple bump allocator.
      *
-     * It starts from the base of the trampoline and allocates downwards.
+     * It starts from end of of the trampoline heap and allocates downwards.
      */
     uint32_t ptr;
 } memctx;
@@ -349,8 +349,7 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, memctx *ctx)
 /* SAF-1-safe */
 void *reloc(uint32_t magic, uint32_t in)
 {
-    /* Get bottom-most low-memory stack address. */
-    memctx ctx = { trampoline_phys + TRAMPOLINE_SPACE };
+    memctx ctx = { trampoline_phys + TRAMPOLINE_HEAP_END };
 
     switch ( magic )
     {
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 7930b7c73892..9d3f2b71447e 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -633,7 +633,7 @@ static void __init efi_arch_memory_setup(void)
     if ( efi_enabled(EFI_LOADER) )
         cfg.size = trampoline_end - trampoline_start;
     else
-        cfg.size = TRAMPOLINE_SPACE + TRAMPOLINE_STACK_SPACE;
+        cfg.size = TRAMPOLINE_SIZE;
 
     status = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
                                    PFN_UP(cfg.size), &cfg.addr);
diff --git a/xen/arch/x86/include/asm/config.h b/xen/arch/x86/include/asm/config.h
index f8a5a4913b07..20141ede31a1 100644
--- a/xen/arch/x86/include/asm/config.h
+++ b/xen/arch/x86/include/asm/config.h
@@ -51,8 +51,9 @@
 
 #define IST_SHSTK_SIZE 1024
 
-#define TRAMPOLINE_STACK_SPACE  PAGE_SIZE
-#define TRAMPOLINE_SPACE        (KB(64) - TRAMPOLINE_STACK_SPACE)
+/* See asm/trampoline.h */
+#define TRAMPOLINE_SIZE         KB(64)
+#define TRAMPOLINE_HEAP_END     (TRAMPOLINE_SIZE - PAGE_SIZE)
 #define WAKEUP_STACK_MIN        3072
 
 #define MBI_SPACE_MIN           (2 * PAGE_SIZE)
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 35693f6e3380..e7d93d1f4ac3 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -410,7 +410,7 @@ ASSERT(!SIZEOF(.plt),      ".plt non-empty")
 ASSERT(!SIZEOF(.rela),     "leftover relocations")
 #endif
 
-ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN,
+ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_HEAP_END - MBI_SPACE_MIN,
     "not enough room for trampoline and mbi data")
 ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN,
     "wakeup stack too small")
-- 
2.39.5

Re: [PATCH 2/2] x86/trampoline: Rationalise the constants to describe the size

[Frediano Ziglio]

On Wed, Nov 13, 2024 at 9:31 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> The logic is far more sane to follow with a total size, and the position of
> the end of the heap.  Remove or fix the the remaining descriptions of how the

typo: the the

> trampoline is laid out.
>
> No functional change.  The compiled binary is identical.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> CC: Frediano Ziglio <frediano.ziglio@cloud.com>
> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  xen/arch/x86/boot/head.S          | 21 ++-------------------
>  xen/arch/x86/boot/reloc.c         |  5 ++---
>  xen/arch/x86/efi/efi-boot.h       |  2 +-
>  xen/arch/x86/include/asm/config.h |  5 +++--
>  xen/arch/x86/xen.lds.S            |  2 +-
>  5 files changed, 9 insertions(+), 26 deletions(-)
>
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index dcda91cfda49..b31cf83758c1 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -494,7 +494,7 @@ trampoline_bios_setup:
>
>  2:
>          /* Reserve memory for the trampoline and the low-memory stack. */
> -        sub     $((TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE)>>4),%ecx
> +        sub     $TRAMPOLINE_SIZE >> 4, %ecx
>
>          /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */
>          xor     %cl, %cl
> @@ -525,23 +525,6 @@ trampoline_setup:
>          mov     %eax, sym_esi(multiboot_ptr)
>  2:
>
> -        /*
> -         * Now trampoline_phys points to the following structure (lowest address
> -         * is at the bottom):
> -         *
> -         * +------------------------+
> -         * | TRAMPOLINE_STACK_SPACE |
> -         * +------------------------+
> -         * |     Data (MBI / PVH)   |
> -         * +- - - - - - - - - - - - +
> -         * |    TRAMPOLINE_SPACE    |
> -         * +------------------------+
> -         *
> -         * Data grows downwards from the highest address of TRAMPOLINE_SPACE
> -         * region to the end of the trampoline. The rest of TRAMPOLINE_SPACE is
> -         * reserved for trampoline code and data.
> -         */
> -

I fail to see a similar description somewhere now.

>          /* Interrogate CPU extended features via CPUID. */
>          mov     $1, %eax
>          cpuid
> @@ -713,7 +696,7 @@ trampoline_setup:
>  1:
>          /* Switch to low-memory stack which lives at the end of trampoline region. */
>          mov     sym_esi(trampoline_phys), %edi
> -        lea     TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE(%edi),%esp
> +        lea     TRAMPOLINE_SIZE(%edi), %esp
>          lea     trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
>          pushl   $BOOT_CS32
>          push    %eax
> diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
> index e50e161b2740..1f47e10f7fa6 100644
> --- a/xen/arch/x86/boot/reloc.c
> +++ b/xen/arch/x86/boot/reloc.c
> @@ -65,7 +65,7 @@ typedef struct memctx {
>      /*
>       * Simple bump allocator.
>       *
> -     * It starts from the base of the trampoline and allocates downwards.
> +     * It starts from end of of the trampoline heap and allocates downwards.

Nice !
Minor typo "It starts from the end of the trampoline heap and
allocates downwards."

>       */
>      uint32_t ptr;
>  } memctx;
> @@ -349,8 +349,7 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, memctx *ctx)
>  /* SAF-1-safe */
>  void *reloc(uint32_t magic, uint32_t in)
>  {
> -    /* Get bottom-most low-memory stack address. */
> -    memctx ctx = { trampoline_phys + TRAMPOLINE_SPACE };
> +    memctx ctx = { trampoline_phys + TRAMPOLINE_HEAP_END };
>
>      switch ( magic )
>      {
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 7930b7c73892..9d3f2b71447e 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -633,7 +633,7 @@ static void __init efi_arch_memory_setup(void)
>      if ( efi_enabled(EFI_LOADER) )
>          cfg.size = trampoline_end - trampoline_start;
>      else
> -        cfg.size = TRAMPOLINE_SPACE + TRAMPOLINE_STACK_SPACE;
> +        cfg.size = TRAMPOLINE_SIZE;
>
>      status = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
>                                     PFN_UP(cfg.size), &cfg.addr);
> diff --git a/xen/arch/x86/include/asm/config.h b/xen/arch/x86/include/asm/config.h
> index f8a5a4913b07..20141ede31a1 100644
> --- a/xen/arch/x86/include/asm/config.h
> +++ b/xen/arch/x86/include/asm/config.h
> @@ -51,8 +51,9 @@
>
>  #define IST_SHSTK_SIZE 1024
>
> -#define TRAMPOLINE_STACK_SPACE  PAGE_SIZE
> -#define TRAMPOLINE_SPACE        (KB(64) - TRAMPOLINE_STACK_SPACE)
> +/* See asm/trampoline.h */

I fail to see any description and need for a heap or why the size is 64kb.
There is a description about trampoline code and wakeup code but not
the fact we copy MBI data and so we need a heap.
Stack could be just due to the need of it, so implicit, heap a bit less.

> +#define TRAMPOLINE_SIZE         KB(64)
> +#define TRAMPOLINE_HEAP_END     (TRAMPOLINE_SIZE - PAGE_SIZE)
>  #define WAKEUP_STACK_MIN        3072
>
>  #define MBI_SPACE_MIN           (2 * PAGE_SIZE)
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index 35693f6e3380..e7d93d1f4ac3 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -410,7 +410,7 @@ ASSERT(!SIZEOF(.plt),      ".plt non-empty")
>  ASSERT(!SIZEOF(.rela),     "leftover relocations")
>  #endif
>
> -ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN,
> +ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_HEAP_END - MBI_SPACE_MIN,
>      "not enough room for trampoline and mbi data")
>  ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN,
>      "wakeup stack too small")

Code is nice, just that documentation is stated but missing in my opinion.

Frediano

Re: [PATCH 2/2] x86/trampoline: Rationalise the constants to describe the size

[Frediano Ziglio]

On Wed, Nov 13, 2024 at 10:18 AM Frediano Ziglio
<frediano.ziglio@cloud.com> wrote:
>
> On Wed, Nov 13, 2024 at 9:31 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >
> > The logic is far more sane to follow with a total size, and the position of
> > the end of the heap.  Remove or fix the the remaining descriptions of how the
>
> typo: the the
>
> > trampoline is laid out.
> >
> > No functional change.  The compiled binary is identical.
> >
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > CC: Jan Beulich <JBeulich@suse.com>
> > CC: Roger Pau Monné <roger.pau@citrix.com>
> > CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> > CC: Frediano Ziglio <frediano.ziglio@cloud.com>
> > CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> > ---
> >  xen/arch/x86/boot/head.S          | 21 ++-------------------
> >  xen/arch/x86/boot/reloc.c         |  5 ++---
> >  xen/arch/x86/efi/efi-boot.h       |  2 +-
> >  xen/arch/x86/include/asm/config.h |  5 +++--
> >  xen/arch/x86/xen.lds.S            |  2 +-
> >  5 files changed, 9 insertions(+), 26 deletions(-)
> >
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> > index dcda91cfda49..b31cf83758c1 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -494,7 +494,7 @@ trampoline_bios_setup:
> >
> >  2:
> >          /* Reserve memory for the trampoline and the low-memory stack. */
> > -        sub     $((TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE)>>4),%ecx
> > +        sub     $TRAMPOLINE_SIZE >> 4, %ecx
> >
> >          /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */
> >          xor     %cl, %cl
> > @@ -525,23 +525,6 @@ trampoline_setup:
> >          mov     %eax, sym_esi(multiboot_ptr)
> >  2:
> >
> > -        /*
> > -         * Now trampoline_phys points to the following structure (lowest address
> > -         * is at the bottom):
> > -         *
> > -         * +------------------------+
> > -         * | TRAMPOLINE_STACK_SPACE |
> > -         * +------------------------+
> > -         * |     Data (MBI / PVH)   |
> > -         * +- - - - - - - - - - - - +
> > -         * |    TRAMPOLINE_SPACE    |
> > -         * +------------------------+
> > -         *
> > -         * Data grows downwards from the highest address of TRAMPOLINE_SPACE
> > -         * region to the end of the trampoline. The rest of TRAMPOLINE_SPACE is
> > -         * reserved for trampoline code and data.
> > -         */
> > -
>
> I fail to see a similar description somewhere now.
>
> >          /* Interrogate CPU extended features via CPUID. */
> >          mov     $1, %eax
> >          cpuid
> > @@ -713,7 +696,7 @@ trampoline_setup:
> >  1:
> >          /* Switch to low-memory stack which lives at the end of trampoline region. */
> >          mov     sym_esi(trampoline_phys), %edi
> > -        lea     TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE(%edi),%esp
> > +        lea     TRAMPOLINE_SIZE(%edi), %esp
> >          lea     trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
> >          pushl   $BOOT_CS32
> >          push    %eax
> > diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
> > index e50e161b2740..1f47e10f7fa6 100644
> > --- a/xen/arch/x86/boot/reloc.c
> > +++ b/xen/arch/x86/boot/reloc.c
> > @@ -65,7 +65,7 @@ typedef struct memctx {
> >      /*
> >       * Simple bump allocator.
> >       *
> > -     * It starts from the base of the trampoline and allocates downwards.
> > +     * It starts from end of of the trampoline heap and allocates downwards.
>
> Nice !
> Minor typo "It starts from the end of the trampoline heap and
> allocates downwards."
>
> >       */
> >      uint32_t ptr;
> >  } memctx;
> > @@ -349,8 +349,7 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, memctx *ctx)
> >  /* SAF-1-safe */
> >  void *reloc(uint32_t magic, uint32_t in)
> >  {
> > -    /* Get bottom-most low-memory stack address. */
> > -    memctx ctx = { trampoline_phys + TRAMPOLINE_SPACE };
> > +    memctx ctx = { trampoline_phys + TRAMPOLINE_HEAP_END };
> >
> >      switch ( magic )
> >      {
> > diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> > index 7930b7c73892..9d3f2b71447e 100644
> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -633,7 +633,7 @@ static void __init efi_arch_memory_setup(void)
> >      if ( efi_enabled(EFI_LOADER) )
> >          cfg.size = trampoline_end - trampoline_start;
> >      else
> > -        cfg.size = TRAMPOLINE_SPACE + TRAMPOLINE_STACK_SPACE;
> > +        cfg.size = TRAMPOLINE_SIZE;
> >
> >      status = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
> >                                     PFN_UP(cfg.size), &cfg.addr);
> > diff --git a/xen/arch/x86/include/asm/config.h b/xen/arch/x86/include/asm/config.h
> > index f8a5a4913b07..20141ede31a1 100644
> > --- a/xen/arch/x86/include/asm/config.h
> > +++ b/xen/arch/x86/include/asm/config.h
> > @@ -51,8 +51,9 @@
> >
> >  #define IST_SHSTK_SIZE 1024
> >
> > -#define TRAMPOLINE_STACK_SPACE  PAGE_SIZE
> > -#define TRAMPOLINE_SPACE        (KB(64) - TRAMPOLINE_STACK_SPACE)
> > +/* See asm/trampoline.h */
>
> I fail to see any description and need for a heap or why the size is 64kb.
> There is a description about trampoline code and wakeup code but not
> the fact we copy MBI data and so we need a heap.
> Stack could be just due to the need of it, so implicit, heap a bit less.
>
> > +#define TRAMPOLINE_SIZE         KB(64)
> > +#define TRAMPOLINE_HEAP_END     (TRAMPOLINE_SIZE - PAGE_SIZE)
> >  #define WAKEUP_STACK_MIN        3072
> >
> >  #define MBI_SPACE_MIN           (2 * PAGE_SIZE)
> > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > index 35693f6e3380..e7d93d1f4ac3 100644
> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -410,7 +410,7 @@ ASSERT(!SIZEOF(.plt),      ".plt non-empty")
> >  ASSERT(!SIZEOF(.rela),     "leftover relocations")
> >  #endif
> >
> > -ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN,
> > +ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_HEAP_END - MBI_SPACE_MIN,
> >      "not enough room for trampoline and mbi data")
> >  ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN,
> >      "wakeup stack too small")
>
> Code is nice, just that documentation is stated but missing in my opinion.
>

Hi,
  I realized I messed the first patch of the series, so with that,
beside the small typos

Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.com>

Frediano

Re: [PATCH 2/2] x86/trampoline: Rationalise the constants to describe the size

[Jan Beulich]

On 13.11.2024 10:30, Andrew Cooper wrote:
> --- a/xen/arch/x86/include/asm/config.h
> +++ b/xen/arch/x86/include/asm/config.h
> @@ -51,8 +51,9 @@
>  
>  #define IST_SHSTK_SIZE 1024
>  
> -#define TRAMPOLINE_STACK_SPACE  PAGE_SIZE
> -#define TRAMPOLINE_SPACE        (KB(64) - TRAMPOLINE_STACK_SPACE)
> +/* See asm/trampoline.h */
> +#define TRAMPOLINE_SIZE         KB(64)
> +#define TRAMPOLINE_HEAP_END     (TRAMPOLINE_SIZE - PAGE_SIZE)

Is there actually a reason these can't move to trampoline.h?

Jan

Re: [PATCH 2/2] x86/trampoline: Rationalise the constants to describe the size

[Andrew Cooper]

On 13/11/2024 10:23 am, Jan Beulich wrote:
> On 13.11.2024 10:30, Andrew Cooper wrote:
>> --- a/xen/arch/x86/include/asm/config.h
>> +++ b/xen/arch/x86/include/asm/config.h
>> @@ -51,8 +51,9 @@
>>  
>>  #define IST_SHSTK_SIZE 1024
>>  
>> -#define TRAMPOLINE_STACK_SPACE  PAGE_SIZE
>> -#define TRAMPOLINE_SPACE        (KB(64) - TRAMPOLINE_STACK_SPACE)
>> +/* See asm/trampoline.h */
>> +#define TRAMPOLINE_SIZE         KB(64)
>> +#define TRAMPOLINE_HEAP_END     (TRAMPOLINE_SIZE - PAGE_SIZE)
> Is there actually a reason these can't move to trampoline.h?

I considered that, and ruled it out, but I can't remember why exactly.

Right now, trampoline.h is C-only, but it can gain some __ASSEMBLY__
easily enough.


The two later constants, WAKEUP_STACK_MIN and MBI_SPACE_MIN are used
only in linker assertions, and of dubious value.  In particular, the
size of the VESA information in the heap is not accounted for in the
MBI_SPACE_MIN check.

I have an idea to remove all of this boot metadata shuffling when the
boot_info work is a bit better done, which is why I left the constants
alone.

~Andrew

Re: [PATCH 2/2] x86/trampoline: Rationalise the constants to describe the size

[Jan Beulich]

On 13.11.2024 12:52, Andrew Cooper wrote:
> On 13/11/2024 10:23 am, Jan Beulich wrote:
>> On 13.11.2024 10:30, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/include/asm/config.h
>>> +++ b/xen/arch/x86/include/asm/config.h
>>> @@ -51,8 +51,9 @@
>>>  
>>>  #define IST_SHSTK_SIZE 1024
>>>  
>>> -#define TRAMPOLINE_STACK_SPACE  PAGE_SIZE
>>> -#define TRAMPOLINE_SPACE        (KB(64) - TRAMPOLINE_STACK_SPACE)
>>> +/* See asm/trampoline.h */
>>> +#define TRAMPOLINE_SIZE         KB(64)
>>> +#define TRAMPOLINE_HEAP_END     (TRAMPOLINE_SIZE - PAGE_SIZE)
>> Is there actually a reason these can't move to trampoline.h?
> 
> I considered that, and ruled it out, but I can't remember why exactly.
> 
> Right now, trampoline.h is C-only, but it can gain some __ASSEMBLY__
> easily enough.
> 
> 
> The two later constants, WAKEUP_STACK_MIN and MBI_SPACE_MIN are used
> only in linker assertions, and of dubious value.  In particular, the
> size of the VESA information in the heap is not accounted for in the
> MBI_SPACE_MIN check.
> 
> I have an idea to remove all of this boot metadata shuffling when the
> boot_info work is a bit better done, which is why I left the constants
> alone.

Fair enough then.

Jan

Re: [PATCH 2/2] x86/trampoline: Rationalise the constants to describe the size

[Andrew Cooper]

On 13/11/2024 9:30 am, Andrew Cooper wrote:
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 7930b7c73892..9d3f2b71447e 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -633,7 +633,7 @@ static void __init efi_arch_memory_setup(void)
>      if ( efi_enabled(EFI_LOADER) )
>          cfg.size = trampoline_end - trampoline_start;
>      else
> -        cfg.size = TRAMPOLINE_SPACE + TRAMPOLINE_STACK_SPACE;
> +        cfg.size = TRAMPOLINE_SIZE;

Something I forgot to mention.

The EFI_LOADER side of this conditional means that the heap isn't valid.

This includes modelist, vesa_glob_info and vesa_mode_info from video.S,
but I can't find where they're used at all.

There's a separate struct vesa_mode_info in reloc.c but that is a
representation of the MB2 vbe_mode_info tag and not the same thing AFAICT.

~Andrew

Re: [PATCH 2/2] x86/trampoline: Rationalise the constants to describe the size

[Frediano Ziglio]

On Wed, Nov 13, 2024 at 11:36 AM Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>
> On 13/11/2024 9:30 am, Andrew Cooper wrote:
> > diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> > index 7930b7c73892..9d3f2b71447e 100644
> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -633,7 +633,7 @@ static void __init efi_arch_memory_setup(void)
> >      if ( efi_enabled(EFI_LOADER) )
> >          cfg.size = trampoline_end - trampoline_start;
> >      else
> > -        cfg.size = TRAMPOLINE_SPACE + TRAMPOLINE_STACK_SPACE;
> > +        cfg.size = TRAMPOLINE_SIZE;
>
> Something I forgot to mention.
>
> The EFI_LOADER side of this conditional means that the heap isn't valid.
>
> This includes modelist, vesa_glob_info and vesa_mode_info from video.S,
> but I can't find where they're used at all.
>
> There's a separate struct vesa_mode_info in reloc.c but that is a
> representation of the MB2 vbe_mode_info tag and not the same thing AFAICT.
>

I think MBI data on EFI path is parsed in another path.

Frediano

Re: [PATCH 2/2] x86/trampoline: Rationalise the constants to describe the size

[Jan Beulich]

On 13.11.2024 12:36, Andrew Cooper wrote:
> On 13/11/2024 9:30 am, Andrew Cooper wrote:
>> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
>> index 7930b7c73892..9d3f2b71447e 100644
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -633,7 +633,7 @@ static void __init efi_arch_memory_setup(void)
>>      if ( efi_enabled(EFI_LOADER) )
>>          cfg.size = trampoline_end - trampoline_start;
>>      else
>> -        cfg.size = TRAMPOLINE_SPACE + TRAMPOLINE_STACK_SPACE;
>> +        cfg.size = TRAMPOLINE_SIZE;
> 
> Something I forgot to mention.
> 
> The EFI_LOADER side of this conditional means that the heap isn't valid.
> 
> This includes modelist, vesa_glob_info and vesa_mode_info from video.S,
> but I can't find where they're used at all.

If nothing else, then iirc to hand (whole struct) to Dom0 upon request.

Jan