Re: [PATCH 6/7] mkimage: support images which require full relocation at mkimage time.

[Ian Campbell]

On Tue, 2013-12-31 at 15:19 +0100, Francesco Lavra wrote:
> On 12/29/2013 07:47 PM, Ian Campbell wrote:
> > diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> > index b4216ff..186d259 100644
> > --- a/util/grub-mkimagexx.c
> > +++ b/util/grub-mkimagexx.c
> > @@ -378,6 +378,7 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr 
> > *sections,
> >                        Elf_Shdr *symtab_section, Elf_Addr 
> > *section_addresses,
> >                        Elf_Half section_entsize, Elf_Half num_sections,
> >                        void *jumpers, Elf_Addr jumpers_addr,
> > +                      Elf_Addr bss_addr, size_t bss_size,
> >                        const struct grub_install_image_target_desc 
> > *image_target)
> >  {
> >    Elf_Word symtab_size, sym_size, num_syms;
> > @@ -416,10 +417,14 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr 
> > *sections,
> >          }
> >        else if (cur_index == STN_UNDEF)
> >     {
> > -     if (sym->st_name)
> > +     if (strcmp (name, "__bss_start") == 0 && bss_addr)
> > +         sym->st_value = bss_addr;
> > +     else if (strcmp (name, "_end") == 0 && bss_addr)
> > +         sym->st_value = bss_addr + bss_size;
> > +     else if (sym->st_name)
> >         grub_util_error ("undefined symbol %s", name);
> > -     else
> > -       continue;
> > +
> > +     continue;
> >     }
> >        else if (cur_index >= num_sections)
> >     grub_util_error ("section %d does not exist", cur_index);
> > @@ -584,7 +589,7 @@ static void
> >  SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr *sections,
> >                          Elf_Addr *section_addresses,
> >                          Elf_Half section_entsize, Elf_Half num_sections,
> > -                        const char *strtab,
> > +                        const char *strtab, grub_uint64_t target_address,
> >                          char *pe_target, Elf_Addr tramp_off,
> >                          Elf_Addr got_off,
> >                          const struct grub_install_image_target_desc 
> > *image_target)
> > @@ -867,6 +872,7 @@ SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr 
> > *sections,
> >                {
> >                case R_ARM_ABS32:
> >                  {
> > +                  sym_addr += target_address;
> >                    grub_util_info ("  
> > ABS32:\ttarget=0x%08lx\toffset=(0x%08x)",
> >                                    (unsigned long) ((char *) target
> >                                                     - (char *) e),
> > @@ -928,7 +934,7 @@ SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr 
> > *sections,
> >                        grub_uint32_t tr_addr;
> >                        grub_int32_t new_offset;
> >                        tr_addr = (char *) tr - (char *) pe_target
> > -                        - target_section_addr;
> > +                        - (target_address - target_section_addr);
> 
> target_section_addr was being subtracted from tr before, now it's being
> added; I guess this is not intentional.

Hrm, yes, it does look suspicious. Good spot.

I added some debug prints to all of this and it came out correct, i.e.
with this change tr_addr (which is actually an offset) is 0x9290 and
without it tr_addr is 0xb8009290, which is clearly nonsense. The
trampolines are placed at 0x9290-0x92a0 (n.b. .bss, which is supposed to
be right after the trampolines, is from offset 0x92a0, which matches).

I suppose something elsewhere accounts for this offset the other way
around and everything comes out in the wash. I'll investigate properly
in the new year.

The R_ARM_THM_{CALL,JUMP*} case is also unchanged, but I don't actually
see any interworking in that direction so that case never triggers, it
should certainly be changed to be the same in both cases, whatever that
turns out to be.

Ian.

> 
> --
> Francesco
>