Re: [PATCH] x86/boot: Fix comment about setting up the real mode stack

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Fri, Nov 15, 2024 at 9:29 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 14.11.2024 19:22, Andrew Cooper wrote:
> > It may have taken a while, but it occurs to me that the mentioned commit 
> > fixed
> > a second problem too.
> >
> > On entering trampoline_boot_cpu_entry(), %esp points at the trampoline 
> > stack,
> > but in a 32bit flat segment.  It happens to be page aligned.
> >
> > When dropping into 16bit mode, the stack segment operates on %sp, preserving
> > the upper bits.  Prior to 1ed76797439e, the top nibble of %sp would depend 
> > on
> > where the trampoline was placed in low memory, and only had a 1/16 chance of
> > being 0 and therefore operating on the intended stack.
> >
> > There was a 15/16 chance of using a different page in the trampoline as if 
> > it
> > were the stack.  Therefore, zeroing %esp was correct, but for more reasons
> > than realised at the time.
>
> I'm afraid I don't follow this analysis. Said commit replaced clearing of %sp
> by clearing of %esp. That made no difference for anything using the 16-bit
> register. I don't see how the top nibble of %sp could have been non-zero
> prior to that change.
>

I think it refers to 1ed76797439e, not this change.

> > Update the comment to explain why zeroing %esp is correct in all cases.  
> > Move
> > it marginally earlier to when a 16bit stack segment is first loaded.
>
> The movement is fine, and the comment is fine by itself, too. It doesn't
> cover the significance of using 32-bit operand size, though (which may or may
> not be relevant, to a fair degree depending on the above).
>

I assume a usage in the firmware, besides, we switch to 32/64 at some
point too so having the upper part 0 is safer in any case.

> Jan
>
> > --- a/xen/arch/x86/boot/trampoline.S
> > +++ b/xen/arch/x86/boot/trampoline.S
> > @@ -176,6 +176,12 @@ trampoline_boot_cpu_entry:
> >          mov     %eax,%gs
> >          mov     %eax,%ss
> >
> > +        /*
> > +         * The stack is at trampoline_phys + 64k, which for a 16bit stack
> > +         * segment wants %sp starting at 0.
> > +         */
> > +        xor     %esp, %esp
> > +
> >          /* Switch to pseudo-rm CS, enter real mode, and flush insn queue. 
> > */
> >          mov     %cr0,%eax
> >          dec     %eax
> > @@ -190,8 +196,6 @@ trampoline_boot_cpu_entry:
> >          mov     %ax,%es
> >          mov     %ax,%ss
> >
> > -        /* Initialise stack pointer and IDT, and enable irqs. */
> > -        xor     %esp,%esp
> >          lidt    bootsym(rm_idt)
> >          sti
> >
>