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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Wed, Nov 13, 2024 at 9:31 AM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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@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/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