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

Andrew Cooper posted 1 patch 2 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20241114182219.1983073-1-andrew.cooper3@citrix.com
xen/arch/x86/boot/trampoline.S | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
[PATCH] x86/boot: Fix comment about setting up the real mode stack
Posted by Andrew Cooper 2 months, 2 weeks ago
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.

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.

Fixes: 1ed76797439e ("x86/boot: fix BIOS memory corruption on certain IBM systems")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>
CC: Frediano Ziglio <frediano.ziglio@cloud.com>
CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 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. */
-        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
-- 
2.39.5


Re: [PATCH] x86/boot: Fix comment about setting up the real mode stack
Posted by Frediano Ziglio 2 months, 2 weeks ago
On Thu, Nov 14, 2024 at 6:22 PM Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> CC: Frediano Ziglio <frediano.ziglio@cloud.com>
> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  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
Re: [PATCH] x86/boot: Fix comment about setting up the real mode stack
Posted by Jan Beulich 2 months, 2 weeks ago
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
>
Re: [PATCH] x86/boot: Fix comment about setting up the real mode stack
Posted by Frediano Ziglio 2 months, 2 weeks ago
On Fri, Nov 15, 2024 at 9:29 AM Jan Beulich <jbeulich@suse.com> 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
> >
>