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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> 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).

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
>