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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.06.16 at 10:41, <daniel.kiper@xxxxxxxxxx> wrote:
> 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?

I don't think you can switch the linking process to use physical
addresses: The final image has to have proper virtual addresses
used for any involved relocations. This would only be benign if
_all_ relocations were relative ones.

Jan

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