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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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@suse.com>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
> 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@xen.org>
>   */
>  
> -/* 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
giving it a more descriptive name, as it would become a global symbol. 
How about relocate_trampoline_32bit() ?

~Andrew

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