| Message ID | 1460723596-13261-4-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
>>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: > --- 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" > ); If Andrew's suggestion to remove this asm() altogether doesn't work out, then I do not see justification for adding a frame pointer here - addressing through %esp should be quite fine. Which in turn would eliminate the need to convert jmp to call. Jan
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@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 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
>>> On 17.06.16 at 10:41, <daniel.kiper@oracle.com> 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@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 > > 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
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" ); /*
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(-)