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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Sat, Oct 5, 2024 at 2:43 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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