| Message ID | 20241005080233.1248850-4-frediano.ziglio@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Reuse 32 bit C code more safely | expand |
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. 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
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) )
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(-)