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
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
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
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
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.