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

[J. Mayer]

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 <address@hidden>
Never organized

elfload.diff
Description: Text Data