Re: new ELF loader module

[Marco Gerards]

Guillem Jover <address@hidden> writes:

Hi Guillem,

> This patch merges some of the ELF static image loading functionality
> into an independent elf module. Please test on PPC as I've only tested
> this on x86.

It does not work on the PPC, unfortunately.  it gives the error:

error: Segment doesn't fit in memory reserved for the image

I will get back to this later in this email.

>
> 2005-02-01  Guillem Jover  <address@hidden>
>
>       * conf/i386-pc.rmk (pkg_MODULES): Add `elf.mod'.

I would prefer elf32.mod because we need something similar for 64 bits
ELFs.  Perhaps some macro magic can be used to use the same sourcecode
for 64 bits ELFs...?

> diff -Naupr -x CVS grub2.dl/loader/elf.c grub2.elf/loader/elf.c
> --- grub2.dl/loader/elf.c     1970-01-01 01:00:00.000000000 +0100
> +++ grub2.elf/loader/elf.c    2005-02-01 05:52:12.000000000 +0100

[...]

> +#if GRUB_HOST_SIZEOF_VOID_P == 4
> +
> +typedef Elf32_Addr Elf_Addr;
> +typedef Elf32_Ehdr Elf_Ehdr;
> +typedef Elf32_Phdr Elf_Phdr;
> +
> +#elif GRUB_HOST_SIZEOF_VOID_P == 8
> +
> +typedef Elf64_Addr Elf_Addr;
> +typedef Elf64_Ehdr Elf_Ehdr;
> +typedef Elf64_Phdr Elf_Phdr;
> +
> +#endif

I think the type of loader should not be dependent of the processor.
Perhaps we want a 64 bits ELF loader on the PPC.  Or the other way
around.

> +
> +Elf_Phdr *
> +grub_elf_get_phdr (grub_file_t file, Elf_Ehdr *ehdr)
> +{
> +  Elf_Phdr *phdr = 0;
> +
> +  phdr = (Elf_Phdr *) grub_malloc (ehdr->e_phnum * ehdr->e_phentsize);

You did not check if the allocation was successful.  This can be done
using:

if (!phdr)
  return 0;

> +grub_err_t
> +grub_elf_load_static (grub_file_t file, Elf_Ehdr *ehdr, Elf_Phdr *phdrs,
> +                   Elf_Addr base_offs,
> +                   grub_addr_t base_addr, grub_size_t base_size)

Can you explain what all arguments do and which effect they will have?
I tried fixing this function so it would work for me, but I got
confused and stopped.

On the PPC base_addr and base_offs are the same.  They are set to the
address the kernel should be loaded to.  Here you just load to what is
requested in the ELF, but this is not always the right thing to do.
For example on the PPC you have to claim memory.  If the memory can
not be claimed, the linux loader just uses another address to load
linux to.

Hopefully we can work on this so we can define the right interface
that can handle all weird requirements we could have and still be
flexible enough.  It looks nice already so it should not be too much
work. :)

> --- grub2.dl/loader/powerpc/ieee1275/linux.c  2005-02-01 04:03:26.000000000 
> +0100
> +++ grub2.elf/loader/powerpc/ieee1275/linux.c 2005-02-01 04:41:31.000000000 
> +0100

[...]

> +  if (grub_elf_load_static (file, ehdr, phdrs, entry, linux_addr, 
> linux_size))
> +    goto fail;

linux_addr and entry are set to the same values at this point.



When compiling GRUB 2 after committing this patch the following
warnings are shown:

loader/powerpc/ieee1275/linux.c: In function `grub_rescue_cmd_linux':
loader/powerpc/ieee1275/linux.c:150: warning: implicit declaration of function 
`grub_elf_get_phdr'
loader/powerpc/ieee1275/linux.c:150: warning: assignment makes pointer from 
integer without a cast
loader/powerpc/ieee1275/linux.c:185: warning: implicit declaration of function 
`grub_elf_load_static'
loader/powerpc/ieee1275/linux.c:104: warning: unused variable `offset'

Perhaps this an be fixed by adding prototypes.  I am not sure if elf.h
would be the right file to add this because it looks like a copy from
glibc.

Hopefully we can cooperate to fix these remaining issues.  In the
linux loader on the PPC works and after thinking about how the elf64
loader can be implemented.  no need to implement it already, I just
don't want this to make it hard to implement one later.

Thanks,
Marco