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

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Fri Oct 11, 2024 at 2:58 PM BST, Frediano Ziglio wrote:
> 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--->|                   |

s/12K/4K/

My brain merged the 12bits in the wrong place. Too much bit twiddling.

> > >  +-------------+-----+-------------+
> > >  | 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

So it's reversed from what I thought

 |<--------------64K-------------->|
 |                   |<-----4K---->|
 +-------------+-----+-------------+
 |  text-(ish) | mbi | stack-space |
 +-------------+-----+-------------+
                  ^                ^
                  |                |
                  |                +-- initial memctx.ptr
                  +------------------- copied Multiboot info + modules


Your version of the comment is a definite improvement over the nonsense that
was there before. Sorry for the noise :)

Cheers,
Alejandro