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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Thu, Nov 14, 2024 at 6:22 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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.
>

It's not clear the additional reasons, even the original commit
mentioned wrong usage of upper bits.

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

I don't see such an explanation, I mean, from "The stack is at
trampoline_phys + 64k, which for a 16bit stack segment wants %sp
starting at 0" I could assume "xor %sp, %sp" is correct too.

> Fixes: 1ed76797439e ("x86/boot: fix BIOS memory corruption on certain IBM 
> systems")

Does this commit really "fixes" something.

The subject "Fix comment about setting up the real mode stack" seems
to indicate an update of a comment, is it considered a fix?

This commit also moves the initialisation of %esp. Is there a reason for this?

> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> CC: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> CC: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> ---
>  xen/arch/x86/boot/trampoline.S | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
> index 924bda37c1b7..f5a1d61280c5 100644
> --- 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. */

Fine, surely this commit without stack pointer handling is useless.

> -        xor     %esp,%esp
>          lidt    bootsym(rm_idt)
>          sti
>
>
> base-commit: 41c80496084aa3601230e01c3bc579a0a6d8359a
> prerequisite-patch-id: 6eb3b54ccd19effe3a89768e0ec5c7a2496a233a
> prerequisite-patch-id: b9c480479c1f4021e9c3fe659811e28bf88f6eca
> prerequisite-patch-id: 68f0d0fff4312fb059861efddbef95ddf4635b0e
> prerequisite-patch-id: 66902cf11d58181ff63b8ee4efb4078df5828490

Frediano