On 11/10/2024 4:06 pm, Alejandro Vallejo wrote: > On Fri Oct 11, 2024 at 2:58 PM BST, Frediano Ziglio wrote: >> On Fri, Oct 11, 2024 at 2:38 PM Andrew Cooper <[email protected]> >> 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 >>>>> <[email protected]> wrote: >>>>>> On Fri, Oct 11, 2024 at 09:52:44AM +0100, Frediano Ziglio wrote: >>>>>>> Signed-off-by: Frediano Ziglio <[email protected]> >>>>>>> --- >>>>>>> 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 :)
Today, the pointer that becomes memctx.ptr is phys+SPACE, which does not include the stack. So initial memctx.ptr starts immediately below the stack, and the bump allocator goes backwards (leftwards). ~Andrew
