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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

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?

Cheers,
Alejandro