On Wed, Nov 13, 2024 at 11:19 AM Andrew Cooper <andrew.coop...@citrix.com> wrote: > > On 13/11/2024 10:20 am, Jan Beulich wrote: > > On 13.11.2024 10:30, 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 0x0140 > >> Boot: 0x01f0 0x1780 > >> Heap: 0x1970 0xe690 > >> Stack: 0xf000 0x1000 > >> > >> and wakeup_stack overlays boot_edd_info. > >> > >> 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/include/asm/trampoline.h | 55 ++++++++++++++++++++++++++- > >> 1 file changed, 53 insertions(+), 2 deletions(-) > >> > >> diff --git a/xen/arch/x86/include/asm/trampoline.h > >> b/xen/arch/x86/include/asm/trampoline.h > >> index 8c1e0b48c2c9..d801bea400dc 100644 > >> --- a/xen/arch/x86/include/asm/trampoline.h > >> +++ b/xen/arch/x86/include/asm/trampoline.h > >> @@ -37,12 +37,63 @@ > >> * 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 > >> + * undefined part of the the boot trampoline. The stack is only used > >> with > >> + * paging disabled. > >> + * > >> + * 3) Boot trampoline: > >> + * > >> + * This region houses various data used by the AP/S3 paths too. > > This is confusing to have here - isn't the boot part (that isn't in the > > same page as the tail of the AP/S3 region) being boot-time only, and hence > > unavailable for S3 and post-boot AP bringup? Both here and with the numbers > > in the description - what position did you use as separator between 2) and > > 3)? > > > > Then again it may be just me who is confused: Didn't we, at some point, > > limit > > the resident trampoline to just one page? Was that only a plan, or a patch > > that never was committed? > > The positioning of various things is rather complicated. > > Only a single 4k page is mapped into idle_pg_table[]. > > But, the AP/S3 path use: > trampoline_cpu_started > idt_48 > gdt_48 > trampoline_xen_phys_start > trampoline_misc_enable_off > trampoline_efer > > Which is beyond the content of wakeup.S. The GDT in particular needs to > stay valid with paging enabled, to load __HYPERVISOR_CS. > > We have /* From here on early boot only. */ in trampoline.S but that > seems to be the extent of checking. Everything needed for AP/S3 is in > the first 0x229. > > I'm open to suggestions for how to describe this better, although the > left hand side of the diagram is already very busy. > > I suppose I could do AP+S3 as a single section, along their combined data? > > > > >> 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 and has 4k > >> + * space reserved. > >> + * > >> + * 4) Heap space: > >> + * > >> + * The first 1k of heap space is statically allocated 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: > >> + * > >> + * 4k of space is reserved for the boot stack, at trampoline_phys + > >> 64k. > > Perhaps add "ending" to clarify it doesn't go beyond +64k? It's being > > expressed > > ... > > Ah yes. That ended up less clear than I was intending. I'll adjust. >
With that Reviewed-by: Frediano Ziglio <frediano.zig...@cloud.com> Frediano
