No more need to pass from assembly code.
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
xen/arch/x86/boot/build32.lds.S | 1 +
xen/arch/x86/boot/head.S | 14 +-------------
xen/arch/x86/boot/reloc.c | 19 ++++++++++---------
3 files changed, 12 insertions(+), 22 deletions(-)
diff --git a/xen/arch/x86/boot/build32.lds.S b/xen/arch/x86/boot/build32.lds.S
index 2d10a75fb1..6598a24e04 100644
--- a/xen/arch/x86/boot/build32.lds.S
+++ b/xen/arch/x86/boot/build32.lds.S
@@ -48,6 +48,7 @@ SECTIONS
DECLARE_IMPORT(__trampoline_seg_start);
DECLARE_IMPORT(__trampoline_seg_stop);
DECLARE_IMPORT(trampoline_phys);
+ DECLARE_IMPORT(boot_vid_info);
. = . + GAP;
*(.text)
*(.text.*)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index ade2c5c43d..dcda91cfda 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -510,22 +510,10 @@ 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
-
-#ifdef CONFIG_VIDEO
- lea sym_esi(boot_vid_info), %edx
-#else
- xor %edx, %edx
-#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) 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..8527fa8d01 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>
@@ -176,7 +179,7 @@ static multiboot_info_t *mbi_reloc(uint32_t mbi_in, memctx *ctx)
return mbi_out;
}
-static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, memctx *ctx)
+static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, memctx *ctx)
{
const multiboot2_fixed_t *mbi_fix = _p(mbi_in);
const multiboot2_memory_map_t *mmap_src;
@@ -185,7 +188,7 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, memctx
memory_map_t *mmap_dst;
multiboot_info_t *mbi_out;
#ifdef CONFIG_VIDEO
- struct boot_video_info *video = NULL;
+ struct boot_video_info *video = &boot_vid_info;
#endif
uint32_t ptr;
unsigned int i, mod_idx = 0;
@@ -290,12 +293,11 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, memctx
#ifdef CONFIG_VIDEO
case MULTIBOOT2_TAG_TYPE_VBE:
- if ( video_out )
+ if ( video )
{
const struct vesa_ctrl_info *ci;
const struct vesa_mode_info *mi;
- video = _p(video_out);
ci = (const void *)get_mb2_data(tag, vbe, vbe_control_info);
mi = (const void *)get_mb2_data(tag, vbe, vbe_mode_info);
@@ -321,7 +323,6 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, memctx
if ( (get_mb2_data(tag, framebuffer, framebuffer_type) !=
MULTIBOOT2_FRAMEBUFFER_TYPE_RGB) )
{
- video_out = 0;
video = NULL;
}
break;
@@ -346,10 +347,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)
{
- memctx ctx = { trampoline };
+ /* Get bottom-most low-memory stack address. */
+ memctx ctx = { (uint32_t)((long)trampoline_phys + TRAMPOLINE_SPACE) };
switch ( magic )
{
@@ -357,7 +358,7 @@ void *reloc(uint32_t magic, uint32_t in, uint32_t trampoline,
return mbi_reloc(in, &ctx);
case MULTIBOOT2_BOOTLOADER_MAGIC:
- return mbi2_reloc(in, video_info, &ctx);
+ return mbi2_reloc(in, &ctx);
case XEN_HVM_START_MAGIC_VALUE:
if ( IS_ENABLED(CONFIG_PVH_GUEST) )
--
2.34.1On 05/10/2024 9:02 am, Frediano Ziglio wrote:
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index ade2c5c43d..dcda91cfda 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -510,22 +510,10 @@ 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
> -
> -#ifdef CONFIG_VIDEO
> - lea sym_esi(boot_vid_info), %edx
> -#else
> - xor %edx, %edx
> -#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) using fastcall. */
> call reloc
> - add $4, %esp
>
Please split this patch in two. Just for testing sanity sake if nothing
else.
Now, while I think the patch is a correct transform of the code, ...
> #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..8527fa8d01 100644
> --- a/xen/arch/x86/boot/reloc.c
> +++ b/xen/arch/x86/boot/reloc.c
> @@ -185,7 +188,7 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, memctx
> memory_map_t *mmap_dst;
> multiboot_info_t *mbi_out;
> #ifdef CONFIG_VIDEO
> - struct boot_video_info *video = NULL;
> + struct boot_video_info *video = &boot_vid_info;
... doesn't this demonstrate that we're again writing into the
trampoline in-Xen, prior to it placing it in low memory?
> @@ -346,10 +347,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)
> {
> - memctx ctx = { trampoline };
> + /* Get bottom-most low-memory stack address. */
> + memctx ctx = { (uint32_t)((long)trampoline_phys + TRAMPOLINE_SPACE) };
Again, while this is a correct transformation (best as I can tell),
wtf? Doesn't this mean we're bump-allocating downwards into our own stack?
~Andrew
On Sat, Oct 5, 2024 at 2:43 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 05/10/2024 9:02 am, Frediano Ziglio wrote:
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> > index ade2c5c43d..dcda91cfda 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -510,22 +510,10 @@ 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
> > -
> > -#ifdef CONFIG_VIDEO
> > - lea sym_esi(boot_vid_info), %edx
> > -#else
> > - xor %edx, %edx
> > -#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) using fastcall. */
> > call reloc
> > - add $4, %esp
> >
>
> Please split this patch in two. Just for testing sanity sake if nothing
> else.
>
Sorry, it's not clear how it should be split. What are the 2 parts ?
> Now, while I think the patch is a correct transform of the code, ...
>
> > #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..8527fa8d01 100644
> > --- a/xen/arch/x86/boot/reloc.c
> > +++ b/xen/arch/x86/boot/reloc.c
> > @@ -185,7 +188,7 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, memctx
> > memory_map_t *mmap_dst;
> > multiboot_info_t *mbi_out;
> > #ifdef CONFIG_VIDEO
> > - struct boot_video_info *video = NULL;
> > + struct boot_video_info *video = &boot_vid_info;
>
> ... doesn't this demonstrate that we're again writing into the
> trampoline in-Xen, prior to it placing it in low memory?
>
Yes, C is more readable to human beings.
There's nothing to demonstrate as far as I'm concerned. I pointed out
different times the assumption you can write into the trampoline to
set it up is spread in multiple places. This change just makes it more
clear just using a more readable language.
> > @@ -346,10 +347,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)
> > {
> > - memctx ctx = { trampoline };
> > + /* Get bottom-most low-memory stack address. */
> > + memctx ctx = { (uint32_t)((long)trampoline_phys + TRAMPOLINE_SPACE) };
>
> Again, while this is a correct transformation (best as I can tell),
> wtf? Doesn't this mean we're bump-allocating downwards into our own stack?
>
> ~Andrew
Yes, that's how it works, no regressions here.
Frediano
On Mon, Oct 7, 2024 at 11:06 AM Frediano Ziglio <frediano.ziglio@cloud.com> wrote: > > On Sat, Oct 5, 2024 at 2:43 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > > > On 05/10/2024 9:02 am, Frediano Ziglio wrote: > > > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > > > index ade2c5c43d..dcda91cfda 100644 > > > --- a/xen/arch/x86/boot/head.S > > > +++ b/xen/arch/x86/boot/head.S > > > @@ -510,22 +510,10 @@ 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 > > > - > > > -#ifdef CONFIG_VIDEO > > > - lea sym_esi(boot_vid_info), %edx > > > -#else > > > - xor %edx, %edx > > > -#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) using fastcall. */ > > > call reloc > > > - add $4, %esp > > > > > > > Please split this patch in two. Just for testing sanity sake if nothing > > else. > > > > Sorry, it's not clear how it should be split. What are the 2 parts ? > Never mind, understood, one variable per commit. Frediano
© 2016 - 2025 Red Hat, Inc.