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
