Re: [PATCH v3 5/5] x86/boot: Clarify comment

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Fri, Oct 11, 2024 at 2:38 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 11/10/2024 2:28 pm, Alejandro Vallejo wrote:
> > On Fri, Oct 11, 2024 at 02:08:37PM +0100, Frediano Ziglio wrote:
> >> On Fri, Oct 11, 2024 at 1:56 PM Alejandro Vallejo
> >> <alejandro.vallejo@xxxxxxxxx> wrote:
> >>> On Fri, Oct 11, 2024 at 09:52:44AM +0100, Frediano Ziglio wrote:
> >>>> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> >>>> ---
> >>>>  xen/arch/x86/boot/reloc.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
> >>>> index e50e161b27..e725cfb6eb 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 on top of space reserved for the trampoline and 
> >>>> allocates downwards.
> >>> nit: Not sure this is much clearer. The trampoline is not a stack (and 
> >>> even if
> >>> it was, I personally find "top" and "bottom" quite ambiguous when it grows
> >>> backwards), so calling top to its lowest address seems more confusing 
> >>> than not.
> >>>
> >>> If anything clarification ought to go in the which direction it takes. 
> >>> Leaving
> >>> "base" instead of "top" and replacing "downwards" by "backwards" to make 
> >>> it
> >>> crystal clear that it's a pointer that starts where the trampoline 
> >>> starts, but
> >>> moves in the opposite direction.
> >>>
> >> Base looks confusing to me, but surely that comment could be confusing.
> >> For the trampoline 64 KB are reserved. Last 4 KB are used as a normal
> >> stack (push/pop/call/whatever), first part gets a copy of the
> >> trampoline code/data (about 6 Kb) the rest (so 64 - 4 - ~6 = ~54 kb)
> >> is used for the copy of MBI information. That "rest" is what we are
> >> talking about here.
> > Last? From what I looked at it seems to be the first 12K.
> >
> >    #define TRAMPOLINE_STACK_SPACE  PAGE_SIZE
> >    #define TRAMPOLINE_SPACE        (KB(64) - TRAMPOLINE_STACK_SPACE)
> >
> > To put it another way, with left=lo-addr and right=hi-addr. The code seems 
> > to
> > do this...
> >
> >  |<--------------64K-------------->|
> >  |<-----12K--->|                   |
> >  +-------------+-----+-------------+
> >  | stack-space | mbi | trampoline  |
> >  +-------------+-----+-------------+
> >                ^  ^
> >                |  |
> >                |  +-- copied Multiboot info + modules
> >                +----- initial memctx.ptr
> >
> > ... with the stack growing backwards to avoid overflowing onto mbi.
> >
> > Or am I missing something?
>
> So I was hoping for some kind of diagram like this, to live in
> arch/x86/include/asm/trampoline.h with the other notes about the trampoline.
>
> But, is that diagram accurate?  Looking at

       /* 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_boot_cpu_entry-trampoline_start(%edi),%eax
       pushl   $BOOT_CS32
       push    %eax

       /* Copy bootstrap trampoline to low memory, below 1MB. */
       lea     sym_esi(trampoline_start), %esi
       mov     $((trampoline_end - trampoline_start) / 4),%ecx
       rep movsl

So, from low to high
- trampoline code/data (%edi at beginning of copy is trampoline_phys,
%esi is trampoline_start)
- space (used for MBI copy)
- stack (%esp is set to trampoline_phys + TRAMPOLINE_SPACE +
TRAMPOLINE_STACK_SPACE)

Frediano