Re: [PATCH 3/3] x86/boot: Use boot_vid_info and trampoline_phys variables directly from C code

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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