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

[Vladimir 'φ-coder/phcoder' Serbinenko]

> === 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

> +
> +     .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?

> +#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?

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.

> +  tmp = *target;
> +  tmp += sym_addr;
> +  *target = tmp;


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

> +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

> +  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?

> +
> +  /* 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

> +  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?

> +/*
> + * 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.

> +
> +#if defined (__arm__)
> +void EXPORT_FUNC (__aeabi_idiv) (void);
> +void EXPORT_FUNC (__aeabi_idivmod) (void);

Are these for all or only 64-bit divisions?

> === 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?

signature.asc
Description: OpenPGP digital signature