We need to write in some location but no reasons to not
trying to restore what we potentially overwrote.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 xen/arch/x86/boot/head.S | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index d8ac0f0494..3e1e9e05b6 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -418,13 +418,17 @@ __pvh_start:
          * absolute stack address as the native path, for lack of a better
          * alternative.
          */
-        mov     $0x1000, %esp
+        mov     $0xffc, %esp
+        pop     %edx
 
         /* Calculate the load base address. */
         call    1f
 1:      pop     %esi
         sub     $sym_offs(1b), %esi
 
+        /* Restore clobbered stack */
+        push    %edx
+
         /* Set up stack. */
         lea     STACK_SIZE - CPUINFO_sizeof + sym_esi(cpu0_stack), %esp
 
@@ -468,13 +472,17 @@ __start:
          * this page for a temporary stack, being one of the safest locations
          * to clobber.
          */
-        mov     $0x1000, %esp
+        mov     $0xffc, %esp
+        pop     %edx
 
         /* Calculate the load base address. */
         call    1f
 1:      pop     %esi
         sub     $sym_offs(1b), %esi
 
+        /* Restore clobbered stack */
+        push    %edx
+
         /* Set up stack. */
         lea     STACK_SIZE - CPUINFO_sizeof + sym_esi(cpu0_stack), %esp
 
-- 
2.46.0

On 21/08/2024 2:32 pm, Frediano Ziglio wrote:
> We need to write in some location but no reasons to not
> trying to restore what we potentially overwrote.
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>

Please follow how Linux does this.

e.g.
https://lore.kernel.org/xen-devel/20240814195053.5564-3-jason.andryuk@amd.com/

Specifically, ...

> ---
>  xen/arch/x86/boot/head.S | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index d8ac0f0494..3e1e9e05b6 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -418,13 +418,17 @@ __pvh_start:
>           * absolute stack address as the native path, for lack of a better
>           * alternative.
>           */

... the reasoning in this comment here is incorrect for non-BIOS
systems, and causes memory corruption for Coreboot based boot.

I've been meaning to fix it for ages, but seeing as you're changing it... :)

The first field under %ebx in a boot ABI we recognise is a much better
choice that an arbitrary location in the first page of memory.

~Andrew