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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

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.

> My .02 anyway.
>
> >       */
> >      uint32_t ptr;
> >  } memctx;
> > --
> > 2.34.1
> >
> >
>
> Cheers,
> Alejandro

Frediano