Re: [Xen-devel] [PATCH v3 03/16] x86/boot: call reloc() using cdecl calling convention

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Fri, Apr 15, 2016 at 04:56:26PM +0100, Andrew Cooper wrote:
> On 15/04/16 13:33, Daniel Kiper wrote:
> > reloc() is not called according to cdecl calling convention.
> > This makes confusion and does not scale well for more arguments.
> > And patch adding multiboot2 protocol support have to pass 3
> > arguments instead of 2. Hence, move reloc() call to cdecl
> > calling convention.
> >
> > I add push %ebp/mov %esp,%ebp/leave instructions here. Though they
> > are not strictly needed in this patch. However, then assembly code
> > in patch adding multiboot2 protocol support is easier to read.
> >
> > Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
> > Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
> > ---
> > v3 - suggestions/fixes:
> >    - simplify assembly in xen/arch/x86/boot/reloc.c file
> >      (suggested by Jan Beulich),
> >    - reorder arguments for reloc() call from xen/arch/x86/boot/head.S
> >      (suggested by Jan Beulich),
> >    - improve commit message
> >      (suggested by Jan Beulich).
> > ---
> >  xen/arch/x86/boot/head.S  |    4 +++-
> >  xen/arch/x86/boot/reloc.c |   18 ++++++++++++++----
> >  2 files changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> > index 32a54a0..28ac721 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -119,8 +119,10 @@ __start:
> >
> >          /* Save the Multiboot info struct (after relocation) for later 
> > use. */
> >          mov     $sym_phys(cpu0_stack)+1024,%esp
> > -        push    %ebx
> > +        push    %eax                /* Boot trampoline address. */
> > +        push    %ebx                /* Multiboot information address. */
> >          call    reloc
> > +        add     $8,%esp             /* Remove reloc() args from stack. */
> >          mov     %eax,sym_phys(multiboot_ptr)
> >
> >          /* Initialize BSS (no nasty surprises!). */
> > diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
> > index 63045c0..006f41d 100644
> > --- a/xen/arch/x86/boot/reloc.c
> > +++ b/xen/arch/x86/boot/reloc.c
> > @@ -10,15 +10,25 @@
> >   *    Keir Fraser <keir@xxxxxxx>
> >   */
> >
> > -/* entered with %eax = BOOT_TRAMPOLINE */
> > +/*
> > + * This entry point is entered from xen/arch/x86/boot/head.S with:
> > + *   - 0x4(%esp) = MULTIBOOT_INFORMATION_ADDRESS,
> > + *   - 0x8(%esp) = BOOT_TRAMPOLINE_ADDRESS.
> > + */
> >  asm (
> >      "    .text                         \n"
> >      "    .globl _start                 \n"
> >      "_start:                           \n"
> > +    "    push %ebp                     \n"
> > +    "    mov  %esp,%ebp                \n"
> >      "    call 1f                       \n"
> > -    "1:  pop  %ebx                     \n"
> > -    "    mov  %eax,alloc-1b(%ebx)      \n"
> > -    "    jmp  reloc                    \n"
> > +    "1:  pop  %ecx                     \n"
> > +    "    mov  0xc(%ebp),%eax           \n"
> > +    "    mov  %eax,alloc-1b(%ecx)      \n"
> > +    "    push 0x8(%ebp)                \n"
> > +    "    call reloc                    \n"
> > +    "    leave                         \n"
> > +    "    ret                           \n"
> >      );
> >
> >  /*
>
> Come to think of this, why are we playing asm games like this at all?
>
> This object file gets linked with head.o anyway, and the reloc()
> function is safe to live anywhere in .init.text.  It might be worth

It does not. reloc.c is converted to asm and then included in head.S. However,
as we discussed during hackhaton I tried to link reloc.o directly with other
objects. As we expected it is easy to convert 32-bit ELF to 64-bit ELF file.
Though ld fails. As I saw the main problem is that virtual addresses start at
0xffff82d080200000. This value simply overflows 32-bit relocations, e.g.:

prelink.o: In function `reloc':
(.text+0x359): relocation truncated to fit: R_X86_64_32 against `.text'

There is a chance that we can fix it by changing virtual addresses to
physical addresses. At first sight it should work because final 32-bit
ELF image contains only physical addresses in ELF headers. However, I am
not sure that this way we will not break something. Hmmm... I have just
realized that at least debugging and crash analysis of hypervisor memory
could be more difficult. Am I missing anything else?

Additionally, there is a lack of _GLOBAL_OFFSET_TABLE_ symbol and ld
complains in that way:

prelink.o: In function `reloc':
(.text+0x36c): undefined reference to `_GLOBAL_OFFSET_TABLE_'

Probably we can fix this issue by putting above mentioned symbol somewhere
into xen.lds or something like that.

Andrew, IIRC, you told us that you linked 32-bit code wrapped into 64-bit
ELF file successfully. How did you do that?

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel