[Qemu-devel] RFC: cleanups in ELF loader

[Qemu-devel] RFC: cleanups in ELF loader

[J. Mayer]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

Following what I've done in the syscalls emulation routines, it appeared
to me that there seems to be a lot of confusions between host and target
long in the ELF loader.
I tried to fix this. I also noticed that the image infos start_data
field was not computed as the Linux kernel does. As the ARM and the
Alpha targets use this information to initialize the program state
before execution, it seems a good idea (to me !) to fix it.
As this patch can have an impact on all user-mode emulated targets, I'd
be glad to have some comments about it to figure if it seems safe to be
commited or if some rework will be needed.

Thanks by advance.

-- 
J. Mayer <l_indien@magic.fr>
Never organized

[-- Attachment #2: elfload.diff --]
[-- Type: text/x-patch, Size: 11430 bytes --]

Index: linux-user/elfload.c
===================================================================
RCS file: /sources/qemu/qemu/linux-user/elfload.c,v
retrieving revision 1.48
diff -u -d -d -p -r1.48 elfload.c
--- linux-user/elfload.c	27 Sep 2007 04:10:43 -0000	1.48
+++ linux-user/elfload.c	30 Sep 2007 01:30:45 -0000
@@ -124,6 +124,7 @@ static inline void init_thread(struct ta
     /* XXX: it seems that r0 is zeroed after ! */
     regs->ARM_r0 = 0;
     /* For uClinux PIC binaries.  */
+    /* XXX: Linux does this only on ARM with no MMU (do we care ?) */
     regs->ARM_r10 = infop->start_data;
 }
 
@@ -516,8 +517,8 @@ static void bswap_sym(struct elf_sym *sy
  * to be put directly into the top of new user memory.
  *
  */
-static unsigned long copy_elf_strings(int argc,char ** argv, void **page,
-                                      target_ulong p)
+static target_ulong copy_elf_strings(int argc,char ** argv, void **page,
+                                     target_ulong p)
 {
     char *tmp, *tmp1, *pag = NULL;
     int len, offset = 0;
@@ -566,8 +567,8 @@ static unsigned long copy_elf_strings(in
     return p;
 }
 
-unsigned long setup_arg_pages(target_ulong p, struct linux_binprm * bprm,
-					      struct image_info * info)
+target_ulong setup_arg_pages(target_ulong p, struct linux_binprm * bprm,
+                             struct image_info * info)
 {
     target_ulong stack_base, size, error;
     int i;
@@ -605,7 +606,7 @@ unsigned long setup_arg_pages(target_ulo
     return p;
 }
 
-static void set_brk(unsigned long start, unsigned long end)
+static void set_brk(target_ulong start, target_ulong end)
 {
 	/* page-align the start and end addresses... */
         start = HOST_PAGE_ALIGN(start);
@@ -624,9 +625,9 @@ static void set_brk(unsigned long start,
 /* We need to explicitly zero any fractional pages after the data
    section (i.e. bss).  This would contain the junk from the file that
    should not be in memory. */
-static void padzero(unsigned long elf_bss, unsigned long last_bss)
+static void padzero(target_ulong elf_bss, target_ulong last_bss)
 {
-        unsigned long nbyte;
+        target_ulong nbyte;
 
 	if (elf_bss >= last_bss)
 		return;
@@ -637,12 +638,12 @@ static void padzero(unsigned long elf_bs
            patch target_mmap(), but it is more complicated as the file
            size must be known */
         if (qemu_real_host_page_size < qemu_host_page_size) {
-            unsigned long end_addr, end_addr1;
+            target_ulong end_addr, end_addr1;
             end_addr1 = (elf_bss + qemu_real_host_page_size - 1) &
                 ~(qemu_real_host_page_size - 1);
             end_addr = HOST_PAGE_ALIGN(elf_bss);
             if (end_addr1 < end_addr) {
-                mmap((void *)end_addr1, end_addr - end_addr1,
+                mmap((void *)g2h(end_addr1), end_addr - end_addr1,
                      PROT_READ|PROT_WRITE|PROT_EXEC,
                      MAP_FIXED|MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
             }
@@ -659,12 +660,12 @@ static void padzero(unsigned long elf_bs
 }
 
 
-static unsigned long create_elf_tables(target_ulong p, int argc, int envc,
-                                       struct elfhdr * exec,
-                                       unsigned long load_addr,
-                                       unsigned long load_bias,
-                                       unsigned long interp_load_addr, int ibcs,
-                                       struct image_info *info)
+static target_ulong create_elf_tables(target_ulong p, int argc, int envc,
+                                      struct elfhdr * exec,
+                                      target_ulong load_addr,
+                                      target_ulong load_bias,
+                                      target_ulong interp_load_addr, int ibcs,
+                                      struct image_info *info)
 {
         target_ulong sp;
         int size;
@@ -732,17 +733,17 @@ static unsigned long create_elf_tables(t
 }
 
 
-static unsigned long load_elf_interp(struct elfhdr * interp_elf_ex,
-				     int interpreter_fd,
-				     unsigned long *interp_load_addr)
+static target_ulong load_elf_interp(struct elfhdr * interp_elf_ex,
+                                    int interpreter_fd,
+                                    target_ulong *interp_load_addr)
 {
 	struct elf_phdr *elf_phdata  =  NULL;
 	struct elf_phdr *eppnt;
-	unsigned long load_addr = 0;
+	target_ulong load_addr = 0;
 	int load_addr_set = 0;
 	int retval;
-	unsigned long last_bss, elf_bss;
-	unsigned long error;
+	target_ulong last_bss, elf_bss;
+	target_ulong error;
 	int i;
 
 	elf_bss = 0;
@@ -818,8 +819,8 @@ static unsigned long load_elf_interp(str
 	  if (eppnt->p_type == PT_LOAD) {
 	    int elf_type = MAP_PRIVATE | MAP_DENYWRITE;
 	    int elf_prot = 0;
-	    unsigned long vaddr = 0;
-	    unsigned long k;
+	    target_ulong vaddr = 0;
+	    target_ulong k;
 
 	    if (eppnt->p_flags & PF_R) elf_prot =  PROT_READ;
 	    if (eppnt->p_flags & PF_W) elf_prot |= PROT_WRITE;
@@ -884,7 +885,7 @@ static unsigned long load_elf_interp(str
 	free(elf_phdata);
 
 	*interp_load_addr = load_addr;
-	return ((unsigned long) interp_elf_ex->e_entry) + load_addr;
+	return ((target_ulong) interp_elf_ex->e_entry) + load_addr;
 }
 
 /* Best attempt to load symbols from this ELF object. */
@@ -972,22 +973,22 @@ int load_elf_binary(struct linux_binprm 
     struct elfhdr interp_elf_ex;
     struct exec interp_ex;
     int interpreter_fd = -1; /* avoid warning */
-    unsigned long load_addr, load_bias;
+    target_ulong load_addr, load_bias;
     int load_addr_set = 0;
     unsigned int interpreter_type = INTERPRETER_NONE;
     unsigned char ibcs2_interpreter;
     int i;
-    unsigned long mapped_addr;
+    target_ulong mapped_addr;
     struct elf_phdr * elf_ppnt;
     struct elf_phdr *elf_phdata;
-    unsigned long elf_bss, k, elf_brk;
+    target_ulong elf_bss, k, elf_brk;
     int retval;
     char * elf_interpreter;
-    unsigned long elf_entry, interp_load_addr = 0;
+    target_ulong elf_entry, interp_load_addr = 0;
     int status;
-    unsigned long start_code, end_code, end_data;
-    unsigned long reloc_func_desc = 0;
-    unsigned long elf_stack;
+    target_ulong start_code, end_code, start_data, end_data;
+    target_ulong reloc_func_desc = 0;
+    target_ulong elf_stack;
     char passed_fileno[6];
 
     ibcs2_interpreter = 0;
@@ -1047,6 +1047,7 @@ int load_elf_binary(struct linux_binprm 
     elf_interpreter = NULL;
     start_code = ~0UL;
     end_code = 0;
+    start_data = 0;
     end_data = 0;
 
     for(i=0;i < elf_ex.e_phnum; i++) {
@@ -1180,9 +1181,9 @@ int load_elf_binary(struct linux_binprm 
     /* OK, This is the point of no return */
     info->end_data = 0;
     info->end_code = 0;
-    info->start_mmap = (unsigned long)ELF_START_MMAP;
+    info->start_mmap = (target_ulong)ELF_START_MMAP;
     info->mmap = 0;
-    elf_entry = (unsigned long) elf_ex.e_entry;
+    elf_entry = (target_ulong) elf_ex.e_entry;
 
     /* Do this so that we can load the interpreter, if need be.  We will
        change some of these later */
@@ -1199,7 +1200,7 @@ int load_elf_binary(struct linux_binprm 
     for(i = 0, elf_ppnt = elf_phdata; i < elf_ex.e_phnum; i++, elf_ppnt++) {
         int elf_prot = 0;
         int elf_flags = 0;
-        unsigned long error;
+        target_ulong error;
 
 	if (elf_ppnt->p_type != PT_LOAD)
             continue;
@@ -1257,6 +1258,8 @@ int load_elf_binary(struct linux_binprm 
         k = elf_ppnt->p_vaddr;
         if (k < start_code)
             start_code = k;
+        if (start_data < k)
+            start_data = k;
         k = elf_ppnt->p_vaddr + elf_ppnt->p_filesz;
         if (k > elf_bss)
             elf_bss = k;
@@ -1273,7 +1276,7 @@ int load_elf_binary(struct linux_binprm 
     elf_brk += load_bias;
     start_code += load_bias;
     end_code += load_bias;
-    //    start_data += load_bias;
+    start_data += load_bias;
     end_data += load_bias;
 
     if (elf_interpreter) {
@@ -1320,7 +1323,7 @@ int load_elf_binary(struct linux_binprm 
     info->start_brk = info->brk = elf_brk;
     info->end_code = end_code;
     info->start_code = start_code;
-    info->start_data = end_code;
+    info->start_data = start_data;
     info->end_data = end_data;
     info->start_stack = bprm->p;
 
Index: linux-user/main.c
===================================================================
RCS file: /sources/qemu/qemu/linux-user/main.c,v
retrieving revision 1.124
diff -u -d -d -p -r1.124 main.c
--- linux-user/main.c	30 Sep 2007 00:38:37 -0000	1.124
+++ linux-user/main.c	30 Sep 2007 01:30:45 -0000
@@ -1947,14 +1947,17 @@ int main(int argc, char **argv)
     if (loglevel) {
         page_dump(logfile);
 
-        fprintf(logfile, "start_brk   0x%08lx\n" , info->start_brk);
-        fprintf(logfile, "end_code    0x%08lx\n" , info->end_code);
-        fprintf(logfile, "start_code  0x%08lx\n" , info->start_code);
-        fprintf(logfile, "start_data  0x%08lx\n" , info->start_data);
-        fprintf(logfile, "end_data    0x%08lx\n" , info->end_data);
-        fprintf(logfile, "start_stack 0x%08lx\n" , info->start_stack);
-        fprintf(logfile, "brk         0x%08lx\n" , info->brk);
-        fprintf(logfile, "entry       0x%08lx\n" , info->entry);
+        fprintf(logfile, "start_brk   0x" TARGET_FMT_lx "\n", info->start_brk);
+        fprintf(logfile, "end_code    0x" TARGET_FMT_lx "\n", info->end_code);
+        fprintf(logfile, "start_code  0x" TARGET_FMT_lx "\n",
+                info->start_code);
+        fprintf(logfile, "start_data  0x" TARGET_FMT_lx "\n",
+                info->start_data);
+        fprintf(logfile, "end_data    0x" TARGET_FMT_lx "\n", info->end_data);
+        fprintf(logfile, "start_stack 0x" TARGET_FMT_lx "\n",
+                info->start_stack);
+        fprintf(logfile, "brk         0x" TARGET_FMT_lx "\n", info->brk);
+        fprintf(logfile, "entry       0x" TARGET_FMT_lx "\n", info->entry);
     }
 
     target_set_brk(info->brk);
Index: linux-user/qemu.h
===================================================================
RCS file: /sources/qemu/qemu/linux-user/qemu.h,v
retrieving revision 1.36
diff -u -d -d -p -r1.36 qemu.h
--- linux-user/qemu.h	27 Sep 2007 13:57:54 -0000	1.36
+++ linux-user/qemu.h	30 Sep 2007 01:30:45 -0000
@@ -17,18 +17,18 @@
  * task_struct fields in the kernel
  */
 struct image_info {
-        target_ulong    load_addr;
-	unsigned long	start_code;
-	unsigned long	end_code;
-        unsigned long   start_data;
-	unsigned long	end_data;
-	unsigned long	start_brk;
-	unsigned long	brk;
-	unsigned long	start_mmap;
-	unsigned long	mmap;
-	unsigned long	rss;
-	unsigned long	start_stack;
-	unsigned long	entry;
+	target_ulong	load_addr;
+	target_ulong	start_code;
+	target_ulong	end_code;
+   target_ulong    start_data;
+	target_ulong	end_data;
+	target_ulong	start_brk;
+	target_ulong	brk;
+	target_ulong	start_mmap;
+	target_ulong	mmap;
+	target_ulong	rss;
+	target_ulong	start_stack;
+	target_ulong	entry;
         target_ulong    code_offset;
         target_ulong    data_offset;
         char            **host_argv;
@@ -105,7 +105,7 @@ extern const char *qemu_uname_release;
 struct linux_binprm {
         char buf[128];
         void *page[MAX_ARG_PAGES];
-        unsigned long p;
+        target_ulong p;
 	int fd;
         int e_uid, e_gid;
         int argc, envc;

Re: [Qemu-devel] RFC: cleanups in ELF loader

[Edgar E. Iglesias]

On Sun, Sep 30, 2007 at 03:39:15AM +0200, J. Mayer wrote:
> Following what I've done in the syscalls emulation routines, it appeared
> to me that there seems to be a lot of confusions between host and target
> long in the ELF loader.
> I tried to fix this. I also noticed that the image infos start_data
> field was not computed as the Linux kernel does. As the ARM and the
> Alpha targets use this information to initialize the program state
> before execution, it seems a good idea (to me !) to fix it.
> As this patch can have an impact on all user-mode emulated targets, I'd
> be glad to have some comments about it to figure if it seems safe to be
> commited or if some rework will be needed.
> 
> Thanks by advance.

Hello,

I'm new but FWIW your changes look fine to me.

Recently while trying out the sparc64 user emulation on a 32bit host I found another similar cause of confusion. I'm not sure if it's a problem in reality but I thought I'd mention it for the record.

target_mmap currently returns a long, but most of it's callers seem to assign the returned address to a target_ulong (elfload.c) or to a target_long (syscall.c). On my particular build (64bit target and 32bit host) the address returned by the host mmap was sign-extended when assigned to a target_ulong.

For example, if the host returns 0x80000000, the 64bit address ended up beeing 0xffffffff80000000. I assumed this was intentional to simplify the catching of the -1 failure code. Not sure if it's a problem, but it confused me at least.

I would have expected target_mmap to return a target_long, and that it internally would conditionally sign extend the hosts mmap return value only if it was -1. This would make it easy for the callers to check for errors while preserving valid addresses through zero-extension.

Does this make any sense?

Best regards
-- 
Edgar E. Iglesias
Axis Communications AB

Re: [Qemu-devel] RFC: cleanups in ELF loader

[J. Mayer]

On Sun, 2007-09-30 at 04:15 +0200, Edgar E. Iglesias wrote:
> On Sun, Sep 30, 2007 at 03:39:15AM +0200, J. Mayer wrote:
> > Following what I've done in the syscalls emulation routines, it appeared
> > to me that there seems to be a lot of confusions between host and target
> > long in the ELF loader.
> > I tried to fix this. I also noticed that the image infos start_data
> > field was not computed as the Linux kernel does. As the ARM and the
> > Alpha targets use this information to initialize the program state
> > before execution, it seems a good idea (to me !) to fix it.
> > As this patch can have an impact on all user-mode emulated targets, I'd
> > be glad to have some comments about it to figure if it seems safe to be
> > commited or if some rework will be needed.
> > 
> > Thanks by advance.
> 
> Hello,
> 
> I'm new but FWIW your changes look fine to me.
> 
> Recently while trying out the sparc64 user emulation on a 32bit host I found another similar cause of confusion. I'm not sure if it's a problem in reality but I thought I'd mention it for the record.
> 
> target_mmap currently returns a long, but most of it's callers seem to assign the returned address to a target_ulong (elfload.c) or to a target_long (syscall.c). On my particular build (64bit target and 32bit host) the address returned by the host mmap was sign-extended when assigned to a target_ulong.
> 
> For example, if the host returns 0x80000000, the 64bit address ended up beeing 0xffffffff80000000. I assumed this was intentional to simplify the catching of the -1 failure code. Not sure if it's a problem, but it confused me at least.
> 
> I would have expected target_mmap to return a target_long, and that it internally would conditionally sign extend the hosts mmap return value only if it was -1. This would make it easy for the callers to check for errors while preserving valid addresses through zero-extension.
> 
> Does this make any sense?

It seems to me that you are right.
I'm still not able to run 64 bits user-mode programs on 32 bits hosts:
It seems to me that there are issues in mmap.c and signal.c. There still
are some issues to be fixed in syscall.c too or at least supicious code
generating compilation warnings which is to be checked.

Feel free to submit any patch that would help solve all those
confusions !

Regards.

-- 
J. Mayer <l_indien@magic.fr>
Never organized

Re: [Qemu-devel] RFC: cleanups in ELF loader

[Edgar E. Iglesias]

On Sun, Sep 30, 2007 at 04:28:57AM +0200, J. Mayer wrote:
> On Sun, 2007-09-30 at 04:15 +0200, Edgar E. Iglesias wrote:
> > On Sun, Sep 30, 2007 at 03:39:15AM +0200, J. Mayer wrote:
> > > Following what I've done in the syscalls emulation routines, it appeared
> > > to me that there seems to be a lot of confusions between host and target
> > > long in the ELF loader.
> > > I tried to fix this. I also noticed that the image infos start_data
> > > field was not computed as the Linux kernel does. As the ARM and the
> > > Alpha targets use this information to initialize the program state
> > > before execution, it seems a good idea (to me !) to fix it.
> > > As this patch can have an impact on all user-mode emulated targets, I'd
> > > be glad to have some comments about it to figure if it seems safe to be
> > > commited or if some rework will be needed.
> > > 
> > > Thanks by advance.
> > 
> > Hello,
> > 
> > I'm new but FWIW your changes look fine to me.
> > 
> > Recently while trying out the sparc64 user emulation on a 32bit host I found another similar cause of confusion. I'm not sure if it's a problem in reality but I thought I'd mention it for the record.
> > 
> > target_mmap currently returns a long, but most of it's callers seem to assign the returned address to a target_ulong (elfload.c) or to a target_long (syscall.c). On my particular build (64bit target and 32bit host) the address returned by the host mmap was sign-extended when assigned to a target_ulong.
> > 
> > For example, if the host returns 0x80000000, the 64bit address ended up beeing 0xffffffff80000000. I assumed this was intentional to simplify the catching of the -1 failure code. Not sure if it's a problem, but it confused me at least.
> > 
> > I would have expected target_mmap to return a target_long, and that it internally would conditionally sign extend the hosts mmap return value only if it was -1. This would make it easy for the callers to check for errors while preserving valid addresses through zero-extension.
> > 
> > Does this make any sense?
> 
> It seems to me that you are right.
> I'm still not able to run 64 bits user-mode programs on 32 bits hosts:
> It seems to me that there are issues in mmap.c and signal.c. There still
> are some issues to be fixed in syscall.c too or at least supicious code
> generating compilation warnings which is to be checked.
> 
> Feel free to submit any patch that would help solve all those
> confusions !

Thanks, I'll try get sometime for looking at it this week.

Best regards
-- 
Edgar E. Iglesias
Axis Communications AB

Re: [Qemu-devel] RFC: cleanups in ELF loader

[Thiemo Seufer]

J. Mayer wrote:
> Following what I've done in the syscalls emulation routines, it appeared
> to me that there seems to be a lot of confusions between host and target
> long in the ELF loader.

But the ELF fields are tied to the ELFCLASS of the supported ABI, not to
the register width of the machine emulation. If anything they should
become the ELF types.

(Your approach will e.g. break down for MIPS N32, where "long" is smaller
thant the register width, and the ABI uses ELFCLASS32.)

> I tried to fix this. I also noticed that the image infos start_data
> field was not computed as the Linux kernel does. As the ARM and the
> Alpha targets use this information to initialize the program state
> before execution, it seems a good idea (to me !) to fix it.

This part looks reasonable.


Thiemo

Re: [Qemu-devel] RFC: cleanups in ELF loader

[J. Mayer]

On Sun, 2007-09-30 at 14:38 +0100, Thiemo Seufer wrote:
> J. Mayer wrote:
> > Following what I've done in the syscalls emulation routines, it appeared
> > to me that there seems to be a lot of confusions between host and target
> > long in the ELF loader.
> 
> But the ELF fields are tied to the ELFCLASS of the supported ABI, not to
> the register width of the machine emulation. If anything they should
> become the ELF types.
> 
> (Your approach will e.g. break down for MIPS N32, where "long" is smaller
> thant the register width, and the ABI uses ELFCLASS32.)

OK, I will try to rework this.

[...]

-- 
J. Mayer <l_indien@magic.fr>
Never organized

Re: [Qemu-devel] RFC: cleanups in ELF loader

[J. Mayer]

On Sun, 2007-09-30 at 17:09 +0200, J. Mayer wrote:
> On Sun, 2007-09-30 at 14:38 +0100, Thiemo Seufer wrote:
> > J. Mayer wrote:
> > > Following what I've done in the syscalls emulation routines, it appeared
> > > to me that there seems to be a lot of confusions between host and target
> > > long in the ELF loader.
> > 
> > But the ELF fields are tied to the ELFCLASS of the supported ABI, not to
> > the register width of the machine emulation. If anything they should
> > become the ELF types.
> > 
> > (Your approach will e.g. break down for MIPS N32, where "long" is smaller
> > thant the register width, and the ABI uses ELFCLASS32.)
> 
> OK, I will try to rework this.

I did check in the linux kernel and it appears that all variables I
changed from unsigned long to target_ulong seem to be unsigned long in the
kernel. This looks fine for me as they are addresses in the process virtual address space, 
so they should fit in a target_ulong, whatever the target registers size could be.
What seems bugged to me (but I did not make any change in that part) is
the auxiliary vectors generation. This seems to me to be the only place
where Linux explicitelly uses elf_addr_t so Qemu should do the same.
Then, it seems that my patch is not so bad and should not break anything
but is not complete as it does not fix the auxiliary vectors.

Do you agree with this, or did I miss something else ?

-- 
J. Mayer <l_indien@magic.fr>
Never organized

Re: [Qemu-devel] RFC: cleanups in ELF loader

[Thiemo Seufer]

J. Mayer wrote:
> On Sun, 2007-09-30 at 17:09 +0200, J. Mayer wrote:
> > On Sun, 2007-09-30 at 14:38 +0100, Thiemo Seufer wrote:
> > > J. Mayer wrote:
> > > > Following what I've done in the syscalls emulation routines, it appeared
> > > > to me that there seems to be a lot of confusions between host and target
> > > > long in the ELF loader.
> > > 
> > > But the ELF fields are tied to the ELFCLASS of the supported ABI, not to
> > > the register width of the machine emulation. If anything they should
> > > become the ELF types.
> > > 
> > > (Your approach will e.g. break down for MIPS N32, where "long" is smaller
> > > thant the register width, and the ABI uses ELFCLASS32.)
> > 
> > OK, I will try to rework this.
> 
> I did check in the linux kernel and it appears that all variables I
> changed from unsigned long to target_ulong seem to be unsigned long in the
> kernel. This looks fine for me as they are addresses in the process virtual address space, 
> so they should fit in a target_ulong, whatever the target registers size could be.

As long as eventual sign extensions are done properly this is ok.
(Some places in the kernel have casts to elf_greg_t for that purpose.)

> What seems bugged to me (but I did not make any change in that part) is
> the auxiliary vectors generation. This seems to me to be the only place
> where Linux explicitelly uses elf_addr_t so Qemu should do the same.
> Then, it seems that my patch is not so bad and should not break anything
> but is not complete as it does not fix the auxiliary vectors.
> 
> Do you agree with this, or did I miss something else ?

The kernel loader (fs/binfmt_elf.c) is somewhat geared towards single
ABI architectures. PowerPC, MIPS, SPARC etc. have all their own kludge
(in arch/$ARCH/kernel/binfmt_elf*.c) to redefine some essential bits
and then #include fs/binfmt_elf.c.

The whole thing looks a bit scary, I'm not sure we should import it in
Qemu. OTOH it is generally a good idea to stay in sync with the kernel
implementation.


Thiemo

Re: [Qemu-devel] RFC: cleanups in ELF loader

[J. Mayer]

[-- Attachment #1: Type: text/plain, Size: 3348 bytes --]

On Mon, 2007-10-01 at 04:35 +0100, Thiemo Seufer wrote:
> J. Mayer wrote:
> > On Sun, 2007-09-30 at 17:09 +0200, J. Mayer wrote:
> > > On Sun, 2007-09-30 at 14:38 +0100, Thiemo Seufer wrote:
> > > > J. Mayer wrote:
> > > > > Following what I've done in the syscalls emulation routines, it appeared
> > > > > to me that there seems to be a lot of confusions between host and target
> > > > > long in the ELF loader.
> > > > 
> > > > But the ELF fields are tied to the ELFCLASS of the supported ABI, not to
> > > > the register width of the machine emulation. If anything they should
> > > > become the ELF types.
> > > > 
> > > > (Your approach will e.g. break down for MIPS N32, where "long" is smaller
> > > > thant the register width, and the ABI uses ELFCLASS32.)
> > > 
> > > OK, I will try to rework this.
> > 
> > I did check in the linux kernel and it appears that all variables I
> > changed from unsigned long to target_ulong seem to be unsigned long in the
> > kernel. This looks fine for me as they are addresses in the process virtual address space, 
> > so they should fit in a target_ulong, whatever the target registers size could be.
> 
> As long as eventual sign extensions are done properly this is ok.
> (Some places in the kernel have casts to elf_greg_t for that purpose.)
> 
> > What seems bugged to me (but I did not make any change in that part) is
> > the auxiliary vectors generation. This seems to me to be the only place
> > where Linux explicitelly uses elf_addr_t so Qemu should do the same.
> > Then, it seems that my patch is not so bad and should not break anything
> > but is not complete as it does not fix the auxiliary vectors.
> > 
> > Do you agree with this, or did I miss something else ?

Following this discussion, I reworked my patch a little bit in order to
use the elf_addr_t type for the aux infos creation. It appears to me
that it functionally changes nothing as elf_addr_t is defined to be
Elf32_OFf / Elf64_Off. But this makes the elf_create_tables function use
the same types as the Linux kernel one.

> The kernel loader (fs/binfmt_elf.c) is somewhat geared towards single
> ABI architectures. PowerPC, MIPS, SPARC etc. have all their own kludge
> (in arch/$ARCH/kernel/binfmt_elf*.c) to redefine some essential bits
> and then #include fs/binfmt_elf.c.
> 
> The whole thing looks a bit scary, I'm not sure we should import it in
> Qemu. OTOH it is generally a good idea to stay in sync with the kernel
> implementation.

The thing I see is different is that the n32 ABI redefines elf_greg_t
and elf_caddr_t as 32 bits. Maybe I missed something but those types
seem not to be used by the ELF loader (or maybe I should look in a more
recent kernel ;-) ).
Then, I have seen no apparent issue with the patch and I'm quite sure
that, even if it's not correct for some specific ABI, it would bring no
regression.

The only point where there could be an evident regression is for the ARM
target:
regs->ARM_r10 = infop->start_data;
As start_data were not computed the same way as it's done in the Linux
kernel, I did change this. I hope this is not to break anything for ARM
emulation but I don't know too much of the ARM ABI so I would be glad if
some ones who know could tell me if what I did might really break ARM
binaries registers initialisation.

-- 
J. Mayer <l_indien@magic.fr>
Never organized

[-- Attachment #2: elfload.diff --]
[-- Type: text/x-patch, Size: 13079 bytes --]

Index: elf.h
===================================================================
RCS file: /sources/qemu/qemu/elf.h,v
retrieving revision 1.10
diff -u -d -d -p -r1.10 elf.h
--- elf.h	16 Sep 2007 21:07:49 -0000	1.10
+++ elf.h	4 Oct 2007 03:12:51 -0000
@@ -1130,6 +1130,7 @@ typedef struct elf64_note {
 #define elf_note	elf32_note
 #define elf_shdr	elf32_shdr
 #define elf_sym		elf32_sym
+#define elf_addr_t	Elf32_Off
 
 #ifdef ELF_USES_RELOCA
 # define ELF_RELOC      Elf32_Rela
@@ -1144,6 +1145,7 @@ typedef struct elf64_note {
 #define elf_note	elf64_note
 #define elf_shdr	elf64_shdr
 #define elf_sym		elf64_sym
+#define elf_addr_t	Elf64_Off
 
 #ifdef ELF_USES_RELOCA
 # define ELF_RELOC      Elf64_Rela
Index: linux-user/elfload.c
===================================================================
RCS file: /sources/qemu/qemu/linux-user/elfload.c,v
retrieving revision 1.48
diff -u -d -d -p -r1.48 elfload.c
--- linux-user/elfload.c	27 Sep 2007 04:10:43 -0000	1.48
+++ linux-user/elfload.c	4 Oct 2007 03:12:51 -0000
@@ -124,6 +124,7 @@ static inline void init_thread(struct ta
     /* XXX: it seems that r0 is zeroed after ! */
     regs->ARM_r0 = 0;
     /* For uClinux PIC binaries.  */
+    /* XXX: Linux does this only on ARM with no MMU (do we care ?) */
     regs->ARM_r10 = infop->start_data;
 }
 
@@ -516,8 +517,8 @@ static void bswap_sym(struct elf_sym *sy
  * to be put directly into the top of new user memory.
  *
  */
-static unsigned long copy_elf_strings(int argc,char ** argv, void **page,
-                                      target_ulong p)
+static target_ulong copy_elf_strings(int argc,char ** argv, void **page,
+                                     target_ulong p)
 {
     char *tmp, *tmp1, *pag = NULL;
     int len, offset = 0;
@@ -566,8 +567,8 @@ static unsigned long copy_elf_strings(in
     return p;
 }
 
-unsigned long setup_arg_pages(target_ulong p, struct linux_binprm * bprm,
-					      struct image_info * info)
+target_ulong setup_arg_pages(target_ulong p, struct linux_binprm * bprm,
+                             struct image_info * info)
 {
     target_ulong stack_base, size, error;
     int i;
@@ -605,7 +606,7 @@ unsigned long setup_arg_pages(target_ulo
     return p;
 }
 
-static void set_brk(unsigned long start, unsigned long end)
+static void set_brk(target_ulong start, target_ulong end)
 {
 	/* page-align the start and end addresses... */
         start = HOST_PAGE_ALIGN(start);
@@ -624,9 +625,9 @@ static void set_brk(unsigned long start,
 /* We need to explicitly zero any fractional pages after the data
    section (i.e. bss).  This would contain the junk from the file that
    should not be in memory. */
-static void padzero(unsigned long elf_bss, unsigned long last_bss)
+static void padzero(target_ulong elf_bss, target_ulong last_bss)
 {
-        unsigned long nbyte;
+        target_ulong nbyte;
 
 	if (elf_bss >= last_bss)
 		return;
@@ -637,12 +638,12 @@ static void padzero(unsigned long elf_bs
            patch target_mmap(), but it is more complicated as the file
            size must be known */
         if (qemu_real_host_page_size < qemu_host_page_size) {
-            unsigned long end_addr, end_addr1;
+            target_ulong end_addr, end_addr1;
             end_addr1 = (elf_bss + qemu_real_host_page_size - 1) &
                 ~(qemu_real_host_page_size - 1);
             end_addr = HOST_PAGE_ALIGN(elf_bss);
             if (end_addr1 < end_addr) {
-                mmap((void *)end_addr1, end_addr - end_addr1,
+                mmap((void *)g2h(end_addr1), end_addr - end_addr1,
                      PROT_READ|PROT_WRITE|PROT_EXEC,
                      MAP_FIXED|MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
             }
@@ -659,18 +660,18 @@ static void padzero(unsigned long elf_bs
 }
 
 
-static unsigned long create_elf_tables(target_ulong p, int argc, int envc,
-                                       struct elfhdr * exec,
-                                       unsigned long load_addr,
-                                       unsigned long load_bias,
-                                       unsigned long interp_load_addr, int ibcs,
-                                       struct image_info *info)
+static target_ulong create_elf_tables(target_ulong p, int argc, int envc,
+                                      struct elfhdr * exec,
+                                      target_ulong load_addr,
+                                      target_ulong load_bias,
+                                      target_ulong interp_load_addr, int ibcs,
+                                      struct image_info *info)
 {
         target_ulong sp;
         int size;
         target_ulong u_platform;
         const char *k_platform;
-        const int n = sizeof(target_ulong);
+        const int n = sizeof(elf_addr_t);
 
         sp = p;
         u_platform = 0;
@@ -697,10 +698,20 @@ static unsigned long create_elf_tables(t
         if (size & 15)
             sp -= 16 - (size & 15);
 
+        /* This is correct because Linux defines
+         * elf_addr_t as Elf32_Off / Elf64_Off
+         */
+#if ELF_CLASS == ELFCLASS32
 #define NEW_AUX_ENT(id, val) do { \
-            sp -= n; tputl(sp, val); \
-            sp -= n; tputl(sp, id); \
+            sp -= n; tput32(sp, val); \
+            sp -= n; tput32(sp, id); \
           } while(0)
+#else
+#define NEW_AUX_ENT(id, val) do { \
+            sp -= n; tput64(sp, val); \
+            sp -= n; tput64(sp, id); \
+          } while(0)
+#endif
         NEW_AUX_ENT (AT_NULL, 0);
 
         /* There must be exactly DLINFO_ITEMS entries here.  */
@@ -732,17 +743,17 @@ static unsigned long create_elf_tables(t
 }
 
 
-static unsigned long load_elf_interp(struct elfhdr * interp_elf_ex,
-				     int interpreter_fd,
-				     unsigned long *interp_load_addr)
+static target_ulong load_elf_interp(struct elfhdr * interp_elf_ex,
+                                    int interpreter_fd,
+                                    target_ulong *interp_load_addr)
 {
 	struct elf_phdr *elf_phdata  =  NULL;
 	struct elf_phdr *eppnt;
-	unsigned long load_addr = 0;
+	target_ulong load_addr = 0;
 	int load_addr_set = 0;
 	int retval;
-	unsigned long last_bss, elf_bss;
-	unsigned long error;
+	target_ulong last_bss, elf_bss;
+	target_ulong error;
 	int i;
 
 	elf_bss = 0;
@@ -818,8 +829,8 @@ static unsigned long load_elf_interp(str
 	  if (eppnt->p_type == PT_LOAD) {
 	    int elf_type = MAP_PRIVATE | MAP_DENYWRITE;
 	    int elf_prot = 0;
-	    unsigned long vaddr = 0;
-	    unsigned long k;
+	    target_ulong vaddr = 0;
+	    target_ulong k;
 
 	    if (eppnt->p_flags & PF_R) elf_prot =  PROT_READ;
 	    if (eppnt->p_flags & PF_W) elf_prot |= PROT_WRITE;
@@ -884,7 +895,7 @@ static unsigned long load_elf_interp(str
 	free(elf_phdata);
 
 	*interp_load_addr = load_addr;
-	return ((unsigned long) interp_elf_ex->e_entry) + load_addr;
+	return ((target_ulong) interp_elf_ex->e_entry) + load_addr;
 }
 
 /* Best attempt to load symbols from this ELF object. */
@@ -972,22 +983,22 @@ int load_elf_binary(struct linux_binprm 
     struct elfhdr interp_elf_ex;
     struct exec interp_ex;
     int interpreter_fd = -1; /* avoid warning */
-    unsigned long load_addr, load_bias;
+    target_ulong load_addr, load_bias;
     int load_addr_set = 0;
     unsigned int interpreter_type = INTERPRETER_NONE;
     unsigned char ibcs2_interpreter;
     int i;
-    unsigned long mapped_addr;
+    target_ulong mapped_addr;
     struct elf_phdr * elf_ppnt;
     struct elf_phdr *elf_phdata;
-    unsigned long elf_bss, k, elf_brk;
+    target_ulong elf_bss, k, elf_brk;
     int retval;
     char * elf_interpreter;
-    unsigned long elf_entry, interp_load_addr = 0;
+    target_ulong elf_entry, interp_load_addr = 0;
     int status;
-    unsigned long start_code, end_code, end_data;
-    unsigned long reloc_func_desc = 0;
-    unsigned long elf_stack;
+    target_ulong start_code, end_code, start_data, end_data;
+    target_ulong reloc_func_desc = 0;
+    target_ulong elf_stack;
     char passed_fileno[6];
 
     ibcs2_interpreter = 0;
@@ -1047,6 +1057,7 @@ int load_elf_binary(struct linux_binprm 
     elf_interpreter = NULL;
     start_code = ~0UL;
     end_code = 0;
+    start_data = 0;
     end_data = 0;
 
     for(i=0;i < elf_ex.e_phnum; i++) {
@@ -1180,9 +1191,9 @@ int load_elf_binary(struct linux_binprm 
     /* OK, This is the point of no return */
     info->end_data = 0;
     info->end_code = 0;
-    info->start_mmap = (unsigned long)ELF_START_MMAP;
+    info->start_mmap = (target_ulong)ELF_START_MMAP;
     info->mmap = 0;
-    elf_entry = (unsigned long) elf_ex.e_entry;
+    elf_entry = (target_ulong) elf_ex.e_entry;
 
     /* Do this so that we can load the interpreter, if need be.  We will
        change some of these later */
@@ -1199,7 +1210,7 @@ int load_elf_binary(struct linux_binprm 
     for(i = 0, elf_ppnt = elf_phdata; i < elf_ex.e_phnum; i++, elf_ppnt++) {
         int elf_prot = 0;
         int elf_flags = 0;
-        unsigned long error;
+        target_ulong error;
 
 	if (elf_ppnt->p_type != PT_LOAD)
             continue;
@@ -1257,6 +1268,8 @@ int load_elf_binary(struct linux_binprm 
         k = elf_ppnt->p_vaddr;
         if (k < start_code)
             start_code = k;
+        if (start_data < k)
+            start_data = k;
         k = elf_ppnt->p_vaddr + elf_ppnt->p_filesz;
         if (k > elf_bss)
             elf_bss = k;
@@ -1273,7 +1286,7 @@ int load_elf_binary(struct linux_binprm 
     elf_brk += load_bias;
     start_code += load_bias;
     end_code += load_bias;
-    //    start_data += load_bias;
+    start_data += load_bias;
     end_data += load_bias;
 
     if (elf_interpreter) {
@@ -1320,7 +1333,7 @@ int load_elf_binary(struct linux_binprm 
     info->start_brk = info->brk = elf_brk;
     info->end_code = end_code;
     info->start_code = start_code;
-    info->start_data = end_code;
+    info->start_data = start_data;
     info->end_data = end_data;
     info->start_stack = bprm->p;
 
Index: linux-user/main.c
===================================================================
RCS file: /sources/qemu/qemu/linux-user/main.c,v
retrieving revision 1.127
diff -u -d -d -p -r1.127 main.c
--- linux-user/main.c	4 Oct 2007 01:54:44 -0000	1.127
+++ linux-user/main.c	4 Oct 2007 03:12:51 -0000
@@ -1950,14 +1950,17 @@ int main(int argc, char **argv)
     if (loglevel) {
         page_dump(logfile);
 
-        fprintf(logfile, "start_brk   0x%08lx\n" , info->start_brk);
-        fprintf(logfile, "end_code    0x%08lx\n" , info->end_code);
-        fprintf(logfile, "start_code  0x%08lx\n" , info->start_code);
-        fprintf(logfile, "start_data  0x%08lx\n" , info->start_data);
-        fprintf(logfile, "end_data    0x%08lx\n" , info->end_data);
-        fprintf(logfile, "start_stack 0x%08lx\n" , info->start_stack);
-        fprintf(logfile, "brk         0x%08lx\n" , info->brk);
-        fprintf(logfile, "entry       0x%08lx\n" , info->entry);
+        fprintf(logfile, "start_brk   0x" TARGET_FMT_lx "\n", info->start_brk);
+        fprintf(logfile, "end_code    0x" TARGET_FMT_lx "\n", info->end_code);
+        fprintf(logfile, "start_code  0x" TARGET_FMT_lx "\n",
+                info->start_code);
+        fprintf(logfile, "start_data  0x" TARGET_FMT_lx "\n",
+                info->start_data);
+        fprintf(logfile, "end_data    0x" TARGET_FMT_lx "\n", info->end_data);
+        fprintf(logfile, "start_stack 0x" TARGET_FMT_lx "\n",
+                info->start_stack);
+        fprintf(logfile, "brk         0x" TARGET_FMT_lx "\n", info->brk);
+        fprintf(logfile, "entry       0x" TARGET_FMT_lx "\n", info->entry);
     }
 
     target_set_brk(info->brk);
Index: linux-user/qemu.h
===================================================================
RCS file: /sources/qemu/qemu/linux-user/qemu.h,v
retrieving revision 1.37
diff -u -d -d -p -r1.37 qemu.h
--- linux-user/qemu.h	30 Sep 2007 14:32:45 -0000	1.37
+++ linux-user/qemu.h	4 Oct 2007 03:12:51 -0000
@@ -17,18 +17,18 @@
  * task_struct fields in the kernel
  */
 struct image_info {
-        target_ulong    load_addr;
-	unsigned long	start_code;
-	unsigned long	end_code;
-        unsigned long   start_data;
-	unsigned long	end_data;
-	unsigned long	start_brk;
-	unsigned long	brk;
-	unsigned long	start_mmap;
-	unsigned long	mmap;
-	unsigned long	rss;
-	unsigned long	start_stack;
-	unsigned long	entry;
+	target_ulong	load_addr;
+	target_ulong	start_code;
+	target_ulong	end_code;
+        target_ulong    start_data;
+	target_ulong	end_data;
+	target_ulong	start_brk;
+	target_ulong	brk;
+	target_ulong	start_mmap;
+	target_ulong	mmap;
+	target_ulong	rss;
+	target_ulong	start_stack;
+	target_ulong	entry;
         target_ulong    code_offset;
         target_ulong    data_offset;
         char            **host_argv;
@@ -105,7 +105,7 @@ extern const char *qemu_uname_release;
 struct linux_binprm {
         char buf[128];
         void *page[MAX_ARG_PAGES];
-        unsigned long p;
+        target_ulong p;
 	int fd;
         int e_uid, e_gid;
         int argc, envc;

Re: [Qemu-devel] RFC: cleanups in ELF loader

[Thiemo Seufer]

J. Mayer wrote:
[snip]
> The thing I see is different is that the n32 ABI redefines elf_greg_t
> and elf_caddr_t as 32 bits. Maybe I missed something but those types
> seem not to be used by the ELF loader (or maybe I should look in a more
> recent kernel ;-) ).
> Then, I have seen no apparent issue with the patch and I'm quite sure
> that, even if it's not correct for some specific ABI, it would bring no
> regression.

Agreed.


Thiemo

Re: [Qemu-devel] RFC: cleanups in ELF loader

[J. Mayer]

On Thu, 2007-10-04 at 12:20 +0100, Thiemo Seufer wrote:
> J. Mayer wrote:
> [snip]
> > The thing I see is different is that the n32 ABI redefines elf_greg_t
> > and elf_caddr_t as 32 bits. Maybe I missed something but those types
> > seem not to be used by the ELF loader (or maybe I should look in a more
> > recent kernel ;-) ).
> > Then, I have seen no apparent issue with the patch and I'm quite sure
> > that, even if it's not correct for some specific ABI, it would bring no
> > regression.
> 
> Agreed.

I'd really like to have the though of ARM developpers about it....
But if no one seem to report potential issues, I'll commit it.

-- 
J. Mayer <l_indien@magic.fr>
Never organized