On 13.11.2024 12:19, Andrew Cooper 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?
If by this you mean to then also cover what the first sentence of 3) said, then yes, that might be preferable. Jan
