1. Patchew
  2. Xen
  3. [PATCH v2 0/6] Reuse 32 bit C code more safely
  4. View patch

[PATCH v2 5/6] x86/boot: Use trampoline_phys variable directly from C code

Frediano Ziglio posted 6 patches 2 months, 2 weeks ago
There is a newer version of this series
[PATCH v2 5/6] x86/boot: Use trampoline_phys variable directly from C code
Posted by Frediano Ziglio 2 months, 2 weeks ago 
No more need to pass from assembly code.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
Changes since v1:
- split the 2 variable changes into 2 commits.
---
 xen/arch/x86/boot/head.S  | 13 ++++---------
 xen/arch/x86/boot/reloc.c |  9 ++++++---
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index ade2c5c43d..32b658fa2b 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -510,22 +510,17 @@ trampoline_setup:
         mov     %esi, sym_esi(xen_phys_start)
         mov     %esi, sym_esi(trampoline_xen_phys_start)
 
-        /* Get bottom-most low-memory stack address. */
-        mov     sym_esi(trampoline_phys), %ecx
-        add     $TRAMPOLINE_SPACE,%ecx
-
+        /* Boot video info to be filled from MB2. */
 #ifdef CONFIG_VIDEO
-        lea     sym_esi(boot_vid_info), %edx
+        lea     sym_esi(boot_vid_info), %ecx
 #else
-        xor     %edx, %edx
+        xor     %ecx, %ecx
 #endif
 
         /* Save Multiboot / PVH info struct (after relocation) for later use. */
-        push    %edx                /* Boot video info to be filled from MB2. */
         mov     %ebx, %edx          /* Multiboot / PVH information address. */
-        /*      reloc(magic/eax, info/edx, trampoline/ecx, video/stk) using fastcall. */
+        /*      reloc(magic/eax, info/edx, video/stk) using fastcall. */
         call    reloc
-        add     $4, %esp
 
 #ifdef CONFIG_PVH_GUEST
         cmpb    $0, sym_esi(pvh_boot)
diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index 94b078d7b1..4cca61adec 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -19,6 +19,9 @@
 #include <xen/kconfig.h>
 #include <xen/multiboot.h>
 #include <xen/multiboot2.h>
+#include <xen/page-size.h>
+
+#include <asm/trampoline.h>
 
 #include <public/arch-x86/hvm/start_info.h>
 
@@ -346,10 +349,10 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, memctx
 }
 
 /* SAF-1-safe */
-void *reloc(uint32_t magic, uint32_t in, uint32_t trampoline,
-            uint32_t video_info)
+void *reloc(uint32_t magic, uint32_t in, uint32_t video_info)
 {
-    memctx ctx = { trampoline };
+    /* Get bottom-most low-memory stack address. */
+    memctx ctx = { (uint32_t)((long)trampoline_phys + TRAMPOLINE_SPACE) };
 
     switch ( magic )
     {
-- 
2.34.1
Re: [PATCH v2 5/6] x86/boot: Use trampoline_phys variable directly from C code
Posted by Andrew Cooper 2 months, 1 week ago 
On 07/10/2024 3:15 pm, Frediano Ziglio wrote:
> No more need to pass from assembly code.
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> ---
> Changes since v1:
> - split the 2 variable changes into 2 commits.
> ---
>  xen/arch/x86/boot/head.S  | 13 ++++---------
>  xen/arch/x86/boot/reloc.c |  9 ++++++---
>  2 files changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index ade2c5c43d..32b658fa2b 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -510,22 +510,17 @@ trampoline_setup:
>          mov     %esi, sym_esi(xen_phys_start)
>          mov     %esi, sym_esi(trampoline_xen_phys_start)
>  
> -        /* Get bottom-most low-memory stack address. */
> -        mov     sym_esi(trampoline_phys), %ecx
> -        add     $TRAMPOLINE_SPACE,%ecx
> -
> +        /* Boot video info to be filled from MB2. */
>  #ifdef CONFIG_VIDEO
> -        lea     sym_esi(boot_vid_info), %edx
> +        lea     sym_esi(boot_vid_info), %ecx
>  #else
> -        xor     %edx, %edx
> +        xor     %ecx, %ecx
>  #endif
>  
>          /* Save Multiboot / PVH info struct (after relocation) for later use. */
> -        push    %edx                /* Boot video info to be filled from MB2. */
>          mov     %ebx, %edx          /* Multiboot / PVH information address. */
> -        /*      reloc(magic/eax, info/edx, trampoline/ecx, video/stk) using fastcall. */
> +        /*      reloc(magic/eax, info/edx, video/stk) using fastcall. */
>          call    reloc
> -        add     $4, %esp

Sorry - I was expecting you split the patches the other way around.

If you drop video first, then trampoline second, there's no churn
changing video from being on the stack to being in ecx.

As it stands the "video/stk" needs to become "video/ecx".

>  
>  #ifdef CONFIG_PVH_GUEST
>          cmpb    $0, sym_esi(pvh_boot)
> diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
> index 94b078d7b1..4cca61adec 100644
> --- a/xen/arch/x86/boot/reloc.c
> +++ b/xen/arch/x86/boot/reloc.c
> @@ -19,6 +19,9 @@
>  #include <xen/kconfig.h>
>  #include <xen/multiboot.h>
>  #include <xen/multiboot2.h>
> +#include <xen/page-size.h>
> +
> +#include <asm/trampoline.h>
>  
>  #include <public/arch-x86/hvm/start_info.h>
>  
> @@ -346,10 +349,10 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, memctx
>  }
>  
>  /* SAF-1-safe */
> -void *reloc(uint32_t magic, uint32_t in, uint32_t trampoline,
> -            uint32_t video_info)
> +void *reloc(uint32_t magic, uint32_t in, uint32_t video_info)
>  {
> -    memctx ctx = { trampoline };
> +    /* Get bottom-most low-memory stack address. */
> +    memctx ctx = { (uint32_t)((long)trampoline_phys + TRAMPOLINE_SPACE) };

You don't need any casts here.  The result can't exceed 1<<20.


Again, I agree this is a correct transform of the code, but the comment
you've moved is incorrect AFAICT, and conflicts with

typedef struct memctx {
    /*
     * Simple bump allocator.
     *
     * It starts from the base of the trampoline and allocates downwards.
     */
    uint32_t ptr;
} memctx;

which is probably my fault, but needs resolving.

What does the trampoline layout actually look like?  Can we see about
correcting the comments?

~Andrew
Re: [PATCH v2 5/6] x86/boot: Use trampoline_phys variable directly from C code
Posted by Frediano Ziglio 2 months, 1 week ago 
On Thu, Oct 10, 2024 at 1:57 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 07/10/2024 3:15 pm, Frediano Ziglio wrote:
> > No more need to pass from assembly code.
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> > ---
> > Changes since v1:
> > - split the 2 variable changes into 2 commits.
> > ---
> >  xen/arch/x86/boot/head.S  | 13 ++++---------
> >  xen/arch/x86/boot/reloc.c |  9 ++++++---
> >  2 files changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> > index ade2c5c43d..32b658fa2b 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -510,22 +510,17 @@ trampoline_setup:
> >          mov     %esi, sym_esi(xen_phys_start)
> >          mov     %esi, sym_esi(trampoline_xen_phys_start)
> >
> > -        /* Get bottom-most low-memory stack address. */
> > -        mov     sym_esi(trampoline_phys), %ecx
> > -        add     $TRAMPOLINE_SPACE,%ecx
> > -
> > +        /* Boot video info to be filled from MB2. */
> >  #ifdef CONFIG_VIDEO
> > -        lea     sym_esi(boot_vid_info), %edx
> > +        lea     sym_esi(boot_vid_info), %ecx
> >  #else
> > -        xor     %edx, %edx
> > +        xor     %ecx, %ecx
> >  #endif
> >
> >          /* Save Multiboot / PVH info struct (after relocation) for later use. */
> > -        push    %edx                /* Boot video info to be filled from MB2. */
> >          mov     %ebx, %edx          /* Multiboot / PVH information address. */
> > -        /*      reloc(magic/eax, info/edx, trampoline/ecx, video/stk) using fastcall. */
> > +        /*      reloc(magic/eax, info/edx, video/stk) using fastcall. */
> >          call    reloc
> > -        add     $4, %esp
>
> Sorry - I was expecting you split the patches the other way around.
>

I'll do it again.

> If you drop video first, then trampoline second, there's no churn
> changing video from being on the stack to being in ecx.
>
> As it stands the "video/stk" needs to become "video/ecx".
>

I'll take care.

> >
> >  #ifdef CONFIG_PVH_GUEST
> >          cmpb    $0, sym_esi(pvh_boot)
> > diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
> > index 94b078d7b1..4cca61adec 100644
> > --- a/xen/arch/x86/boot/reloc.c
> > +++ b/xen/arch/x86/boot/reloc.c
> > @@ -19,6 +19,9 @@
> >  #include <xen/kconfig.h>
> >  #include <xen/multiboot.h>
> >  #include <xen/multiboot2.h>
> > +#include <xen/page-size.h>
> > +
> > +#include <asm/trampoline.h>
> >
> >  #include <public/arch-x86/hvm/start_info.h>
> >
> > @@ -346,10 +349,10 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, memctx
> >  }
> >
> >  /* SAF-1-safe */
> > -void *reloc(uint32_t magic, uint32_t in, uint32_t trampoline,
> > -            uint32_t video_info)
> > +void *reloc(uint32_t magic, uint32_t in, uint32_t video_info)
> >  {
> > -    memctx ctx = { trampoline };
> > +    /* Get bottom-most low-memory stack address. */
> > +    memctx ctx = { (uint32_t)((long)trampoline_phys + TRAMPOLINE_SPACE) };
>
> You don't need any casts here.  The result can't exceed 1<<20.
>

I'll do

>
> Again, I agree this is a correct transform of the code, but the comment
> you've moved is incorrect AFAICT, and conflicts with
>
> typedef struct memctx {
>     /*
>      * Simple bump allocator.
>      *
>      * It starts from the base of the trampoline and allocates downwards.
>      */
>     uint32_t ptr;
> } memctx;
>
> which is probably my fault, but needs resolving.
>
> What does the trampoline layout actually look like?  Can we see about
> correcting the comments?
>

Can you suggest what I should write?

> ~Andrew

Frediano

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.