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

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

[Andrew Cooper]

Extended somewhat from v1.  See patches for differences.

Andrew Cooper (4):
  x86/trampoline: Check the size permanent trampoline at link time
  x86/trampoline: Simplify the wakeup_stack checks
  x86/trampoline: Document how the trampoline is laid out
  x86/trampoline: Rationalise the constants to describe the size

 xen/arch/x86/boot/head.S              | 23 ++--------
 xen/arch/x86/boot/reloc.c             |  5 +--
 xen/arch/x86/boot/trampoline.S        |  5 +--
 xen/arch/x86/boot/wakeup.S            |  8 ++--
 xen/arch/x86/efi/efi-boot.h           |  2 +-
 xen/arch/x86/include/asm/config.h     |  6 ---
 xen/arch/x86/include/asm/trampoline.h | 65 ++++++++++++++++++++++++++-
 xen/arch/x86/xen.lds.S                | 15 +++++--
 8 files changed, 88 insertions(+), 41 deletions(-)


base-commit: 380b32a476e714275c53e51f6482b3b650aff6f8
-- 
2.39.5

[PATCH v2 1/4] x86/trampoline: Check the size permanent trampoline at link time

[Andrew Cooper]

This is a little safer than leaving it to hope.

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>

v2:
 * New.
---
 xen/arch/x86/boot/trampoline.S | 2 ++
 xen/arch/x86/xen.lds.S         | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index b8ab0ffdcbb0..55e4a3e402f7 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -161,6 +161,8 @@ GLOBAL(trampoline_cpu_started)
         .equ    wakeup_stack, trampoline_start + PAGE_SIZE
         .global wakeup_stack
 
+LABEL(trampoline_perm_end, 0)
+
 /* From here on early boot only. */
 
         .code32
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 35693f6e3380..221fc2ef3f35 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -410,6 +410,13 @@ ASSERT(!SIZEOF(.plt),      ".plt non-empty")
 ASSERT(!SIZEOF(.rela),     "leftover relocations")
 #endif
 
+/*
+ * The permanent trampoline resides in a single 4k page.  Placement logic
+ * takes care to ensure that trampoline_phys is page aligned.
+ */
+ASSERT((trampoline_perm_end - trampoline_start) <= PAGE_SIZE,
+       "Permentant trampoline too large")
+
 ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN,
     "not enough room for trampoline and mbi data")
 ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN,
-- 
2.39.5

Re: [PATCH v2 1/4] x86/trampoline: Check the size permanent trampoline at link time

[Jan Beulich]

On 14.11.2024 10:08, Andrew Cooper wrote:
> This is a little safer than leaving it to hope.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

[PATCH v2 2/4] x86/trampoline: Simplify the wakeup_stack checks

[Andrew Cooper]

By checking that the permanent trampoline fits within 1k (at the time of
writing, it's 0x229 bytes), we can simplify the wakeup_stack handling.

Move the setup into wakeup.S, because it's rather out of place in
trampoline.S, and change it to a local symbol.

Drop wakeup_stack_start and WAKEUP_STACK_MIN entirely.

No functional change.

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>

v2:
 * New
---
 xen/arch/x86/boot/trampoline.S    | 5 -----
 xen/arch/x86/boot/wakeup.S        | 8 +++++---
 xen/arch/x86/include/asm/config.h | 2 --
 xen/arch/x86/xen.lds.S            | 7 ++++---
 4 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 55e4a3e402f7..924bda37c1b7 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -156,11 +156,6 @@ GLOBAL(trampoline_xen_phys_start)
 GLOBAL(trampoline_cpu_started)
         .byte   0
 
-/* The first page of trampoline is permanent, the rest boot-time only. */
-/* Reuse the boot trampoline on the 1st trampoline page as stack for wakeup. */
-        .equ    wakeup_stack, trampoline_start + PAGE_SIZE
-        .global wakeup_stack
-
 LABEL(trampoline_perm_end, 0)
 
 /* From here on early boot only. */
diff --git a/xen/arch/x86/boot/wakeup.S b/xen/arch/x86/boot/wakeup.S
index df5ea2445739..d53f92b02463 100644
--- a/xen/arch/x86/boot/wakeup.S
+++ b/xen/arch/x86/boot/wakeup.S
@@ -1,5 +1,10 @@
         .code16
 
+/* The first page of trampoline is permanent, the rest boot-time only. */
+/* Reuse the boot logic on the first trampoline page as stack for wakeup. */
+        .equ    wakeup_stack, trampoline_start + PAGE_SIZE
+        .local  wakeup_stack
+
 #define wakesym(sym) (sym - wakeup_start)
 
 /*
@@ -166,6 +171,3 @@ wakeup_64:
         /* Jump to high mappings and the higher-level wakeup code. */
         movabs  $s3_resume, %rbx
         jmp     *%rbx
-
-/* Stack for wakeup: rest of first trampoline page. */
-ENTRY(wakeup_stack_start)
diff --git a/xen/arch/x86/include/asm/config.h b/xen/arch/x86/include/asm/config.h
index f8a5a4913b07..84696e0a7db5 100644
--- a/xen/arch/x86/include/asm/config.h
+++ b/xen/arch/x86/include/asm/config.h
@@ -53,8 +53,6 @@
 
 #define TRAMPOLINE_STACK_SPACE  PAGE_SIZE
 #define TRAMPOLINE_SPACE        (KB(64) - TRAMPOLINE_STACK_SPACE)
-#define WAKEUP_STACK_MIN        3072
-
 #define MBI_SPACE_MIN           (2 * PAGE_SIZE)
 
 /* Primary stack is restricted to 8kB by guard pages. */
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 221fc2ef3f35..224b46771d0c 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -413,11 +413,12 @@ ASSERT(!SIZEOF(.rela),     "leftover relocations")
 /*
  * The permanent trampoline resides in a single 4k page.  Placement logic
  * takes care to ensure that trampoline_phys is page aligned.
+ *
+ * The wakeup stack wants to reside in the same page and wants to be at least
+ * 3k in size, so make sure the text/data fits in 1k.
  */
-ASSERT((trampoline_perm_end - trampoline_start) <= PAGE_SIZE,
+ASSERT((trampoline_perm_end - trampoline_start) <= 1024,
        "Permentant trampoline too large")
 
 ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE - 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 v2 2/4] x86/trampoline: Simplify the wakeup_stack checks

[Jan Beulich]

On 14.11.2024 10:08, Andrew Cooper wrote:
> By checking that the permanent trampoline fits within 1k (at the time of
> writing, it's 0x229 bytes), we can simplify the wakeup_stack handling.
> 
> Move the setup into wakeup.S, because it's rather out of place in
> trampoline.S, and change it to a local symbol.
> 
> Drop wakeup_stack_start and WAKEUP_STACK_MIN entirely.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one nit:

> --- a/xen/arch/x86/boot/trampoline.S
> +++ b/xen/arch/x86/boot/trampoline.S
> @@ -156,11 +156,6 @@ GLOBAL(trampoline_xen_phys_start)
>  GLOBAL(trampoline_cpu_started)
>          .byte   0
>  
> -/* The first page of trampoline is permanent, the rest boot-time only. */
> -/* Reuse the boot trampoline on the 1st trampoline page as stack for wakeup. */
> -        .equ    wakeup_stack, trampoline_start + PAGE_SIZE
> -        .global wakeup_stack
> -
>  LABEL(trampoline_perm_end, 0)
>  
>  /* From here on early boot only. */
> diff --git a/xen/arch/x86/boot/wakeup.S b/xen/arch/x86/boot/wakeup.S
> index df5ea2445739..d53f92b02463 100644
> --- a/xen/arch/x86/boot/wakeup.S
> +++ b/xen/arch/x86/boot/wakeup.S
> @@ -1,5 +1,10 @@
>          .code16
>  
> +/* The first page of trampoline is permanent, the rest boot-time only. */
> +/* Reuse the boot logic on the first trampoline page as stack for wakeup. */
> +        .equ    wakeup_stack, trampoline_start + PAGE_SIZE
> +        .local  wakeup_stack
> +
>  #define wakesym(sym) (sym - wakeup_start)

As you move it, it would be nice for the commentary to become a single
multi-line comment rather than two single-line ones.

Jan

[PATCH v2 3/4] 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  0x0229
  Boot:   0x02d9  0x1697
  Heap:   0x1970  0xe690
  Stack:  0xf000  0x1000

and wakeup_stack overlays boot_edd_info.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.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>

v2:
 * Rebase over the introduction of trampoline_perm_end
 * Fix the description of the boot stack position
---
 xen/arch/x86/include/asm/trampoline.h | 57 ++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 2 deletions(-)

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,
+ *    module list, etc).  It does so with a bump allocator starting from the
+ *    end of the heap and allocating backwards.
+ *
+ * 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[]
+ */
+
 #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[];
-- 
2.39.5

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

[Jan Beulich]

On 14.11.2024 10:08, 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  0x0229
>   Boot:   0x02d9  0x1697
>   Heap:   0x1970  0xe690
>   Stack:  0xf000  0x1000
> 
> and wakeup_stack overlays boot_edd_info.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

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

[Alejandro Vallejo]

On Thu Nov 14, 2024 at 9:08 AM GMT, 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  0x0229
>   Boot:   0x02d9  0x1697
>   Heap:   0x1970  0xe690
>   Stack:  0xf000  0x1000
>
> and wakeup_stack overlays boot_edd_info.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.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>
>
> v2:
>  * Rebase over the introduction of trampoline_perm_end
>  * Fix the description of the boot stack position
> ---
>  xen/arch/x86/include/asm/trampoline.h | 57 ++++++++++++++++++++++++++-
>  1 file changed, 55 insertions(+), 2 deletions(-)
>
> 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.
> + *
> + * 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.

>   */
>  /* SAF-0-safe */
>  extern char trampoline_start[], trampoline_end[];

Cheers,
Alejandro

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

[Andrew Cooper]

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?

~Andrew

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

[Andrew Cooper]

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

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

[Alejandro Vallejo]

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

[PATCH v2 4/4] 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 remaining descriptions of how the
trampoline is laid out.

Move the relevant constants into trampoline.h, which requires making the
header safe to include in assembly files.

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>

v2:
 * Fix typos
 * Rebase over removal of WAKEUP_STACK_MIN
 * Move constants into trampoline.h
---
 xen/arch/x86/boot/head.S              | 23 ++++-------------------
 xen/arch/x86/boot/reloc.c             |  5 ++---
 xen/arch/x86/efi/efi-boot.h           |  2 +-
 xen/arch/x86/include/asm/config.h     |  4 ----
 xen/arch/x86/include/asm/trampoline.h |  8 ++++++++
 xen/arch/x86/xen.lds.S                |  3 ++-
 6 files changed, 17 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index dcda91cfda49..1b3bd16fe575 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -8,6 +8,8 @@
 #include <asm/processor.h>
 #include <asm/msr-index.h>
 #include <asm/cpufeature.h>
+#include <asm/trampoline.h>
+
 #include <public/elfnote.h>
 
 #define ENTRY(name)                             \
@@ -494,7 +496,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 +527,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 +698,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..7a375ad41c1c 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 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 84696e0a7db5..19746f956ec3 100644
--- a/xen/arch/x86/include/asm/config.h
+++ b/xen/arch/x86/include/asm/config.h
@@ -51,10 +51,6 @@
 
 #define IST_SHSTK_SIZE 1024
 
-#define TRAMPOLINE_STACK_SPACE  PAGE_SIZE
-#define TRAMPOLINE_SPACE        (KB(64) - TRAMPOLINE_STACK_SPACE)
-#define MBI_SPACE_MIN           (2 * PAGE_SIZE)
-
 /* Primary stack is restricted to 8kB by guard pages. */
 #define PRIMARY_STACK_SIZE 8192
 
diff --git a/xen/arch/x86/include/asm/trampoline.h b/xen/arch/x86/include/asm/trampoline.h
index 559111d2ccfc..cb3e3d06c7bc 100644
--- a/xen/arch/x86/include/asm/trampoline.h
+++ b/xen/arch/x86/include/asm/trampoline.h
@@ -90,6 +90,13 @@
  */
 
 #include <xen/compiler.h>
+
+#define TRAMPOLINE_SIZE         KB(64)
+#define TRAMPOLINE_HEAP_END     (TRAMPOLINE_SIZE - PAGE_SIZE)
+#define MBI_SPACE_MIN           (2 * PAGE_SIZE)
+
+#ifndef __ASSEMBLY__
+
 #include <xen/types.h>
 
 /*
@@ -160,4 +167,5 @@ extern uint8_t kbd_shift_flags;
 extern uint16_t boot_edid_caps;
 extern uint8_t boot_edid_info[128];
 
+#endif /* !__ASSEMBLY__ */
 #endif /* X86_ASM_TRAMPOLINE_H */
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 224b46771d0c..42217eaf2485 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -9,6 +9,7 @@
 #endif
 #include <xen/xen.lds.h>
 #include <asm/page.h>
+#include <asm/trampoline.h>
 
 #ifdef EFI
 
@@ -420,5 +421,5 @@ ASSERT(!SIZEOF(.rela),     "leftover relocations")
 ASSERT((trampoline_perm_end - trampoline_start) <= 1024,
        "Permentant trampoline too large")
 
-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")
-- 
2.39.5

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

[Jan Beulich]

On 14.11.2024 10:08, Andrew Cooper 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 remaining descriptions of how the
> trampoline is laid out.
> 
> Move the relevant constants into trampoline.h, which requires making the
> header safe to include in assembly files.
> 
> No functional change.  The compiled binary is identical.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>