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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Wed, Nov 13, 2024 at 10:18 AM Frediano Ziglio
<frediano.ziglio@xxxxxxxxx> wrote:
>
> 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.
>

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

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

Frediano