On Wed, Nov 13, 2024 at 9:31 AM Andrew Cooper <andrew.coop...@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.coop...@citrix.com>
> ---
> CC: Jan Beulich <jbeul...@suse.com>
> CC: Roger Pau Monné <roger....@citrix.com>
> CC: Daniel P. Smith <dpsm...@apertussolutions.com>
> CC: Frediano Ziglio <frediano.zig...@cloud.com>
> CC: Alejandro Vallejo <alejandro.vall...@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
