Re: [PATCH 2/7] Initial support for ARMv7 architecture

[Leif Lindholm]

On Mon, Apr 01, 2013 at 03:28:17AM +0200, Vladimir '??-coder/phcoder' 
Serbinenko wrote:
> > === added directory 'grub-core/kern/arm'
> 
> > === added file 'grub-core/kern/arm/cache.S'
> > --- grub-core/kern/arm/cache.S      1970-01-01 00:00:00 +0000
> > +++ grub-core/kern/arm/cache.S      2013-03-24 12:56:20 +0000
> > @@ -0,0 +1,251 @@
> > +/*
> > + *  GRUB  --  GRand Unified Bootloader
> > + *  Copyright (C) 2013  Free Software Foundation, Inc.
> > + *
> > + *  GRUB is free software: you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License as published by
> > + *  the Free Software Foundation, either version 3 of the License, or
> > + *  (at your option) any later version.
> > + *
> > + *  GRUB is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + *
> > + *  You should have received a copy of the GNU General Public License
> > + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <grub/symbol.h>
> > +#include <grub/dl.h>
> > +
> > +   .file   "cache.S"
> > +   .text
> > +   .syntax unified
> > +#if !defined (__thumb2__)
> > +   .arm
> > +#define ARM(x...)  x
> > +#define THUMB(x...)
> > +#else
> > +   .thumb
> > +#define THUMB(x...)        x
> > +#define ARM(x...)
> > +#endif
> 
> What are THUMB/ARM macros for? I don't see any use of them

Used for conditionals where one set of instructions is required when
assembling for ARM instruction set and another for Thumb.
(See lib/arm/setjmp.S for an example.)
Being lazy, I tend to stick it in as boilerplate.
Can drop for now in files not using it.

> > +
> > +   .align  2
> > +
> > +/*
> > + * Simple cache maintenance functions
> > + */
> > +
> > +@ r0 - *beg (inclusive)
> > +@ r1 - *end (exclusive)    
> > +clean_dcache_range:
> > +   @ Clean data cache range for range to point-of-unification
> > +   ldr     r2, dlinesz
> > +1: cmp     r0, r1
> > +   bge     2f
> > +#ifdef DEBUG
> > +   push    {r0-r2, lr}
> > +   mov     r1, r2
> > +   mov     r2, r0
> > +   ldr     r0, =dcstr
> > +   bl      EXT_C(grub_printf)
> > +   pop     {r0-r2, lr}
> > +#endif
> 
> Could you use grub_dprintf framework for debug messages which are still
> useful and remove the rest?

Sure - these are propably all a bit excessive for now, so will drop.

> > +#ifdef DEBUG
> > +   push    {r0-r2, lr}
> > +   mov     r1, r2
> > +   mov     r2, r0
> > +   ldr     r0, =icstr
> > +   bl      EXT_C(grub_printf)
> > +   pop     {r0-r2, lr}
> > +#endif
> 
> Ditto
> > address@hidden __wrap___clear_cache(char *beg, char *end);
> 
> > +FUNCTION(__wrap___clear_cache)
> 
> Now that we removed all nested functions would it be possible to remove
> this function in order to make linker fail if any nested functions appear?

Tested it, and it seems to work.
It won't actually fail at link time though, but when running grub-mkimage,
so will require an install test, not just a build test.

> Can't really review cache flushing due to my very limited ARM experience.
> 
> > +#ifdef GRUB_UTIL
> > +# include <grub/util/misc.h>
> > +#else
> 
> Ditto. Use grub_dprintf
> 
> > +# if !defined(__thumb2__)
> > +#  error "Relocations not implemented for A32 ("ARM") instruction set yet!"
> > +# endif
> > +
> > +grub_err_t reloc_jump24 (grub_uint32_t *addr, Elf32_Addr sym_addr);
> > +grub_err_t reloc_thm_call (grub_uint16_t *addr, Elf32_Addr sym_addr);
> > +grub_err_t reloc_thm_jump19 (grub_uint16_t *addr, Elf32_Addr sym_addr);
> > +
> > +#ifdef DL_DEBUG
> > +static const char *symstrtab;
> > +
> > +/*
> > + * This is a bit of a hack, setting the symstrtab pointer to the last 
> > STRTAB
> > + * section in the module (which is where symbol names are in the objects 
> > I've
> > + * inspected manually). 
> > + */
> > +static void
> > +set_symstrtab (Elf_Ehdr * e)
> > +{
> > +  int i;
> > +  Elf_Shdr *s;
> > +
> > +  symstrtab = NULL;
> > +
> > +  for (i = 0, s = (Elf_Shdr *) ((grub_uint32_t) e + e->e_shoff);
> > +       i < e->e_shnum;
> > +       i++, s = (Elf_Shdr *) ((grub_uint32_t) s + e->e_shentsize))
> > +    if (s->sh_type == SHT_STRTAB)
> > +      symstrtab = (void *) ((grub_addr_t) e + s->sh_offset);
> > +}
> > +
> > +static const char *
> > +get_symbolname (Elf_Sym * sym)
> > +{
> > +  const char *symbolname = symstrtab + sym->st_name;
> > +
> > +  return (*symbolname ? symbolname : NULL);
> > +}
> > +#endif /* DL_DEBUG */
> 
> This would need to be platform-independent. I don't see how this would
> be ARM-specific.

Fair enough - I may just strip it out.

> > +  tmp = *target;
> > +  tmp += sym_addr;
> > +  *target = tmp;
> 
> 
> Why not *target += sym_addr and put it inline?

In a development version I actually extended all of these operations to use
grub_host_to_target/grub_target_to_host, but decided against including that
in my patchset since it ends up being fairly invasive around grub-mkimage
and the util headers. Ended up keeping the logic in place.

It's a simple enough function, so I can put it inline, but I opted to put
it separately since the other relocation handlers are.

> > +grub_err_t
> > +reloc_thm_call (grub_uint16_t *target, Elf32_Addr sym_addr)
> > +{
> > +  grub_int32_t offset, offset_low, offset_high;
> > +  grub_uint32_t sign, j1, j2, is_blx;
> > +  grub_uint32_t insword, insmask;
> > +
> > +  /* Extract instruction word in alignment-safe manner */
> > +  insword = (*target << 16) | (*(target + 1));
> 
> Why not use grub_get_unaligned32

Like Francesco said, this is endianess as well as alignment, so I should
probable edit the comment to reflect.
 
> > +  insmask = 0xf800d000;
> > +
> > +  /* B.W/BL or BLX? Affects range and expected target state */
> > +  if (((insword >> 12) & 0xd) == 0xc)
> > +    is_blx = 1;
> > +  else
> > +    is_blx = 0;
> > +
> > +  /* If BLX, target symbol must be ARM (target address LSB == 0) */
> > +  if (is_blx && (sym_addr & 1))
> > +    {
> > +#ifndef GRUB_UTIL
> > +      return grub_error
> > +   (GRUB_ERR_BUG, N_("Relocation targeting wrong execution state"));
> 
> This would be GRUB_ERR_BAD_MODULE
> 
> > +
> > +  offset_low = -16777216;
> > +  offset_high = is_blx ? 16777212 : 16777214;
> 
> Why not write these values in hex?
 
They are written in decimal in the ARM Architecture Reference Manual.
I could change them if you wish.

> > +
> > +  /* Extract bitfields from instruction words */
> > +  sign = (insword >> 26) & 1;
> > +  j1 = (insword >> 13) & 1;
> > +  j2 = (insword >> 11) & 1;
> > +  offset = (sign << 24) | ((~(j1 ^ sign) & 1) << 23) |
> > +    ((~(j2 ^ sign) & 1) << 22) |
> > +    ((insword & 0x03ff0000) >> 4) | ((insword & 0x000007ff) << 1);
> > +
> > +  /* Sign adjust and calculate offset */
> > +  if (offset & (1 << 24))
> > +    offset -= (1 << 25);
> > +#ifdef GRUB_UTIL
> > +  grub_util_info ("    sym_addr = 0x%08x", sym_addr);
> > +#endif
> > +#ifdef GRUB_UTIL
> > +  offset += sym_addr;
> > +#else
> > +  offset += sym_addr - (grub_uint32_t) target;
> > +#endif
> 
> What is this target? Why is it dependent whether we're in util or not
 
In the util case, this ends up being called from the EFI relocation code,
so cannot be fully resolved. This may end up being true also for any future
relocatable ELF image for U-Boot.

> > +  if ((offset < offset_low) || (offset > offset_high))
> > +    {
> > +#ifdef GRUB_UTIL
> > +      grub_util_error ("Relocation out of range");
> > +#else
> > +      return grub_error
> > +   (GRUB_ERR_OUT_OF_RANGE, N_("THM_CALL Relocation out of range."));
> 
> ERR_BAD_MODULE ditto
> 
> > +#ifdef GRUB_UTIL
> > +#pragma GCC diagnostic ignored "-Wcast-align"
> > +  grub_util_info ("    *target = 0x%08x", *((unsigned int *)target));
> > +#endif
> 
> grub_get_unaligned.
> 
> > +  *addr = insword >> 16;
> > +  *(addr + 1) = insword & 0xffff;
> 
> Use grub_set_unaligned+
> 
> > +/*************************************************
> > + * Runtime dynamic linker with helper functions. *
> > + *************************************************/
> > +#ifndef GRUB_UTIL
> > +/*
> > + * find_segment(): finds a module segment matching sh_info
> > + */
> > +static grub_dl_segment_t
> > +find_segment (grub_dl_segment_t seg, Elf32_Word sh_info)
> > +{
> > +  for (; seg; seg = seg->next)
> > +    if (seg->section == sh_info)
> > +      return seg;
> > +
> > +  return NULL;
> > +}
> > +
> 
> Could you just put this directly in the code?

I can, if preferred.

> > +/*
> > + * grub_arch_dl_relocate_symbols():
> > + *   Only externally visible function in this file.
> > + *   Locates the relocations section of the ELF object, and calls
> > + *   do_relocations() to deal with it.
> > + */
> > +grub_err_t
> > +grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr)
> > +{
> > +  Elf_Ehdr *e = ehdr;
> > +  Elf_Shdr *s;
> > +  unsigned i;
> > +
> > +  if (!has_symtab (e))
> > +    return grub_error (GRUB_ERR_BAD_MODULE, N_("no symbol table"));
> > +
> > +#ifdef DL_DEBUG
> > +  set_symstrtab (e);
> > +#endif
> > +
> > +#define FIRST_SHDR(x) ((Elf_Shdr *) ((grub_addr_t)(x) + (x)->e_shoff))
> > +#define NEXT_SHDR(x, y) ((Elf_Shdr *) ((grub_addr_t)(y) + 
> > (x)->e_shentsize))
> > +
> > +  for (i = 0, s = FIRST_SHDR (e); i < e->e_shnum; i++, s = NEXT_SHDR (e, 
> > s))
> > +    {
> > +      grub_err_t ret;
> > +
> > +      switch (s->sh_type)
> > +   {
> > +   case SHT_REL:
> > +     {
> > +       /* Relocations, no addends */
> > +       ret = do_relocations (s, e, mod);
> > +       if (ret != GRUB_ERR_NONE)
> > +         return ret;
> > +     }
> > +     break;
> > +   case SHT_NULL:
> > +   case SHT_PROGBITS:
> > +   case SHT_SYMTAB:
> > +   case SHT_STRTAB:
> > +   case SHT_NOBITS:
> > +   case SHT_ARM_ATTRIBUTES:
> > +     break;
> > +   case SHT_RELA:
> > +   default:
> > +     {
> > +       grub_printf ("unhandled section_type: %d (0x%08x)\n",
> > +                    s->sh_type, s->sh_type);
> > +       return GRUB_ERR_NOT_IMPLEMENTED_YET;
> > +     };
> > +   }
> > +    }
> 
> 
> Why is this file much bigger and very different from similar file for
> other architectures? Perhaps you should make dl.c malloc in single chunk
> like already done for ia64 and ppc. It may simplify the code but
> additionally will solve few problems with relocation overflows.
 
Three reasons:
- It contains (potentially redundant) additional debug functionality.
- Thumb2 relocations are a bit fiddly.
- I implement also code used for the EFI case, which for ia64 is inlined
  (causing duplication) in util/grub-mkimagexx.c.

> > +
> > +#if defined (__arm__)
> > +void EXPORT_FUNC (__aeabi_idiv) (void);
> > +void EXPORT_FUNC (__aeabi_idivmod) (void);
> 
> Are these for all or only 64-bit divisions?

All.
 
> > === modified file 'include/grub/symbol.h'
> > --- include/grub/symbol.h   2012-05-28 15:49:18 +0000
> > +++ include/grub/symbol.h   2013-03-24 12:56:20 +0000
> > @@ -29,7 +29,11 @@
> >  
> >  #if HAVE_ASM_USCORE
> >  #ifdef ASM_FILE
> > -# define EXT_C(sym)        _ ## sym
> > +# ifndef (__arm__)
> > +#  define EXT_C(sym)       _ ## sym
> > +# else
> > +#  define EXT_C(sym)       % ## sym
> 
> Why % ? Shouldn't configure tests adjusted as well?
 
Empirical tests showed it worked :)
Yes, probably - in acinclude.m4?

/
    Leif