Re: [PATCH v2 5/6] x86/boot: Use trampoline_phys variable directly from C code

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Thu, Oct 10, 2024 at 1:57 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 07/10/2024 3:15 pm, Frediano Ziglio wrote:
> > No more need to pass from assembly code.
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> > ---
> > Changes since v1:
> > - split the 2 variable changes into 2 commits.
> > ---
> >  xen/arch/x86/boot/head.S  | 13 ++++---------
> >  xen/arch/x86/boot/reloc.c |  9 ++++++---
> >  2 files changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> > index ade2c5c43d..32b658fa2b 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -510,22 +510,17 @@ 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
> > -
> > +        /* Boot video info to be filled from MB2. */
> >  #ifdef CONFIG_VIDEO
> > -        lea     sym_esi(boot_vid_info), %edx
> > +        lea     sym_esi(boot_vid_info), %ecx
> >  #else
> > -        xor     %edx, %edx
> > +        xor     %ecx, %ecx
> >  #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, video/stk) using fastcall. */
> >          call    reloc
> > -        add     $4, %esp
>
> Sorry - I was expecting you split the patches the other way around.
>

I'll do it again.

> If you drop video first, then trampoline second, there's no churn
> changing video from being on the stack to being in ecx.
>
> As it stands the "video/stk" needs to become "video/ecx".
>

I'll take care.

> >
> >  #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..4cca61adec 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>
> >
> > @@ -346,10 +349,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, uint32_t video_info)
> >  {
> > -    memctx ctx = { trampoline };
> > +    /* Get bottom-most low-memory stack address. */
> > +    memctx ctx = { (uint32_t)((long)trampoline_phys + TRAMPOLINE_SPACE) };
>
> You don't need any casts here.  The result can't exceed 1<<20.
>

I'll do

>
> Again, I agree this is a correct transform of the code, but the comment
> you've moved is incorrect AFAICT, and conflicts with
>
> typedef struct memctx {
>     /*
>      * Simple bump allocator.
>      *
>      * It starts from the base of the trampoline and allocates downwards.
>      */
>     uint32_t ptr;
> } memctx;
>
> which is probably my fault, but needs resolving.
>
> What does the trampoline layout actually look like?  Can we see about
> correcting the comments?
>

Can you suggest what I should write?

> ~Andrew

Frediano