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. ~Andrew
