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

Reply via email to