Re: [PATCH v3 07/10] RISC-V: Add awareness for RISC-V reloations

[Alexander Graf]

On 21.11.18 16:51, Daniel Kiper wrote:
> On Wed, Nov 14, 2018 at 06:27:36PM +0100, Alexander Graf wrote:
>> This patch adds awareness of RISC-V relocations throughout the grub tools
>> as well as dynamic linkage and elf->PE relocation conversion support.
>>
>> Signed-off-by: Alexander Graf <address@hidden>
>>
>> ---
>>
>> v2 -> v3:
>>
>>   - Fix riscv32 target
>> ---
>>  grub-core/kern/dl.c         |   6 +-
>>  grub-core/kern/riscv/dl.c   | 335 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  include/grub/dl.h           |   6 +-
>>  util/grub-mkimagexx.c       | 268 +++++++++++++++++++++++++++++++++++
>>  util/grub-module-verifier.c |  56 ++++++++
>>  5 files changed, 666 insertions(+), 5 deletions(-)
>>  create mode 100644 grub-core/kern/riscv/dl.c
>>
>> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
>> index f8d58f029..48eb5e7b6 100644
>> --- a/grub-core/kern/dl.c
>> +++ b/grub-core/kern/dl.c
>> @@ -225,7 +225,7 @@ grub_dl_load_segments (grub_dl_t mod, const Elf_Ehdr *e)
>>    unsigned i;
>>    const Elf_Shdr *s;
>>    grub_size_t tsize = 0, talign = 1;
>> -#if !defined (__i386__) && !defined (__x86_64__)
>> +#if !defined (__i386__) && !defined (__x86_64__) && !defined(__riscv)
> 
> Could not we use __riscv__?

Unfortunately __riscv__ is not defined in recent versions of gcc:


https://github.com/riscv/riscv-toolchain-conventions#cc-preprocessor-definitions

> 
>>    grub_size_t tramp;
>>    grub_size_t got;
>>    grub_err_t err;
>> @@ -241,7 +241,7 @@ grub_dl_load_segments (grub_dl_t mod, const Elf_Ehdr *e)
>>      talign = s->sh_addralign;
>>      }
>>
>> -#if !defined (__i386__) && !defined (__x86_64__)
>> +#if !defined (__i386__) && !defined (__x86_64__) && !defined(__riscv)
>>    err = grub_arch_dl_get_tramp_got_size (e, &tramp, &got);
>>    if (err)
>>      return err;
>> @@ -304,7 +304,7 @@ grub_dl_load_segments (grub_dl_t mod, const Elf_Ehdr *e)
>>        mod->segment = seg;
>>      }
>>      }
>> -#if !defined (__i386__) && !defined (__x86_64__)
>> +#if !defined (__i386__) && !defined (__x86_64__) && !defined(__riscv)
>>    ptr = (char *) ALIGN_UP ((grub_addr_t) ptr, GRUB_ARCH_DL_TRAMP_ALIGN);
>>    mod->tramp = ptr;
>>    mod->trampptr = ptr;
>> diff --git a/grub-core/kern/riscv/dl.c b/grub-core/kern/riscv/dl.c
>> new file mode 100644
>> index 000000000..503b67df1
>> --- /dev/null
>> +++ b/grub-core/kern/riscv/dl.c
>> @@ -0,0 +1,335 @@
>> +/* dl.c - arch-dependent part of loadable module support */
>> +/*
>> + *  GRUB  --  GRand Unified Bootloader
>> + *  Copyright (C) 2013  Free Software Foundation, Inc.
> 
> s/2013/2018/
> 
>> + *
>> + *  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/dl.h>
>> +#include <grub/elf.h>
>> +#include <grub/misc.h>
>> +#include <grub/err.h>
>> +#include <grub/mm.h>
>> +#include <grub/i18n.h>
>> +
>> +#define LDR 0x58000050
>> +#define BR 0xd61f0200
>> +
>> +
> 
> Please drop this empty line.
> 
>> +/*
>> + * Check if EHDR is a valid ELF header.
>> + */
>> +grub_err_t
>> +grub_arch_dl_check_header (void *ehdr)
>> +{
>> +  Elf_Ehdr *e = ehdr;
>> +
>> +  /* Check the magic numbers.  */
>> +  if (e->e_ident[EI_DATA] != ELFDATA2LSB || e->e_machine != EM_RISCV)
>> +    return grub_error (GRUB_ERR_BAD_OS,
>> +                   N_("invalid arch-dependent ELF magic"));
>> +
>> +  return GRUB_ERR_NONE;
>> +}
>> +
>> +#pragma GCC diagnostic ignored "-Wcast-align"
> 
> Why? Could you add a comment here?

Mostly because all other targets have that as well?

I assume it's to silence warnings that the alignment could be mismatched
now, because offsets to elf data pointers could potentially be odd. In
practice, it never is though.

If I remove it, I get the following compile errors:

kern/riscv/dl.c: In function ‘grub_arch_dl_relocate_symbols’:
kern/riscv/dl.c:59:14: error: cast increases required alignment of
target type [-Werror=cast-align]
   for (rel = (Elf_Rel *) ((char *) ehdr + s->sh_offset),
              ^
kern/riscv/dl.c:60:9: error: cast increases required alignment of target
type [-Werror=cast-align]
   max = (Elf_Rel *) ((char *) rel + s->sh_size);
         ^
kern/riscv/dl.c:62:14: error: cast increases required alignment of
target type [-Werror=cast-align]
        rel = (Elf_Rel *) ((char *) rel + s->sh_entsize))
              ^
kern/riscv/dl.c:72:13: error: cast increases required alignment of
target type [-Werror=cast-align]
       sym = (Elf_Sym *) ((char *) mod->symtab
             ^
kern/riscv/dl.c:256:18: error: cast increases required alignment of
target type [-Werror=cast-align]
      for (rel2 = (Elf_Rela *) ((char *) rel - s->sh_entsize);
                  ^
kern/riscv/dl.c:258:14: error: cast increases required alignment of
target type [-Werror=cast-align]
       rel2 = (Elf_Rela *) ((char *) rel2 - s->sh_entsize))
              ^
kern/riscv/dl.c:275:14: error: cast increases required alignment of
target type [-Werror=cast-align]
       sym2 = (Elf_Sym *) ((char *) mod->symtab
              ^

> 
>> +/* Relocate symbols. */
>> +grub_err_t
>> +grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
>> +                           Elf_Shdr *s, grub_dl_segment_t seg)
>> +{
>> +  Elf_Rel *rel, *max;
>> +
>> +  for (rel = (Elf_Rel *) ((char *) ehdr + s->sh_offset),
>> +     max = (Elf_Rel *) ((char *) rel + s->sh_size);
>> +       rel < max;
>> +       rel = (Elf_Rel *) ((char *) rel + s->sh_entsize))
>> +    {
>> +      Elf_Sym *sym;
>> +      void *place;
>> +      grub_size_t sym_addr;
>> +
>> +      if (rel->r_offset >= seg->size)
>> +    return grub_error (GRUB_ERR_BAD_MODULE,
>> +                       "reloc offset is out of the segment");
>> +
>> +      sym = (Elf_Sym *) ((char *) mod->symtab
>> +                     + mod->symsize * ELF_R_SYM (rel->r_info));
>> +
>> +      sym_addr = sym->st_value;
>> +      if (s->sh_type == SHT_RELA)
>> +    sym_addr += ((Elf_Rela *) rel)->r_addend;
>> +
>> +      place = (void *) ((grub_addr_t) seg->addr + rel->r_offset);
>> +
>> +      switch (ELF_R_TYPE (rel->r_info))
>> +    {
>> +    case R_RISCV_32:
>> +      {
>> +        grub_uint32_t *abs_place = place;
>> +
>> +        grub_dprintf ("dl", "  reloc_abs32 %p => 0x%016llx\n",
>> +                      place, (unsigned long long) sym_addr);
>> +
>> +        *abs_place = (grub_uint32_t) sym_addr;
>> +      }
>> +      break;
>> +    case R_RISCV_64:
>> +      {
>> +        grub_size_t *abs_place = place;
>> +
>> +        grub_dprintf ("dl", "  reloc_abs64 %p => 0x%016llx\n",
>> +                      place, (unsigned long long) sym_addr);
>> +
>> +        *abs_place = (grub_size_t) sym_addr;
>> +      }
>> +      break;
>> +
>> +    case R_RISCV_ADD8:
>> +      {
>> +        grub_uint8_t *abs_place = place;
>> +
>> +        *abs_place += (grub_uint8_t) sym_addr;
>> +      }
>> +      break;
>> +    case R_RISCV_ADD16:
>> +      {
>> +        grub_uint16_t *abs_place = place;
>> +
>> +        *abs_place += (grub_uint16_t) sym_addr;
>> +      }
>> +      break;
>> +    case R_RISCV_ADD32:
>> +      {
>> +        grub_uint32_t *abs_place = place;
>> +
>> +        *abs_place += (grub_uint32_t) sym_addr;
>> +      }
>> +      break;
>> +    case R_RISCV_ADD64:
>> +      {
>> +        grub_size_t *abs_place = place;
>> +
>> +        *abs_place += (grub_size_t) sym_addr;
>> +      }
>> +      break;
>> +
>> +    case R_RISCV_SUB8:
>> +      {
>> +        grub_uint8_t *abs_place = place;
>> +
>> +        *abs_place -= (grub_uint8_t) sym_addr;
>> +      }
>> +      break;
>> +    case R_RISCV_SUB16:
>> +      {
>> +        grub_uint16_t *abs_place = place;
>> +
>> +        *abs_place -= (grub_uint16_t) sym_addr;
>> +      }
>> +      break;
>> +    case R_RISCV_SUB32:
>> +      {
>> +        grub_uint32_t *abs_place = place;
>> +
>> +        *abs_place -= (grub_uint32_t) sym_addr;
>> +      }
>> +      break;
>> +    case R_RISCV_SUB64:
>> +      {
>> +        grub_size_t *abs_place = place;
>> +
>> +        *abs_place -= (grub_size_t) sym_addr;
>> +      }
>> +      break;
>> +
>> +    case R_RISCV_BRANCH:
>> +      {
>> +        grub_uint32_t *abs_place = place;
>> +        grub_ssize_t off = sym_addr - (grub_addr_t) place;
>> +        grub_uint32_t imm12 = (off & 0x1000) << (31 - 12);
>> +        grub_uint32_t imm11 = (off & 0x800) >> (11 - 7);
>> +        grub_uint32_t imm10_5 = (off & 0x7e0) << (30 - 10);
>> +        grub_uint32_t imm4_1 = (off & 0x1e) << (11 - 4);
>> +        *abs_place = (*abs_place & 0x1fff07f)
>> +                     | imm12 | imm11 | imm10_5 | imm4_1;
> 
> Could not we use some constants instead of numbers here?
> If this does not make sense than please name the source of this numbers.

As Andreas already mentioned, these are just taken from the spec. I
agree that it looks quite unreadable, but putting the numbers into
constants would only make things worse I guess.

I'll add the spec reference into the head of this file.

> 
>> +      }
>> +      break;
>> +
>> +    case R_RISCV_JAL:
>> +      {
>> +        grub_uint32_t *abs_place = place;
>> +        grub_ssize_t off = sym_addr - (grub_addr_t) place;
>> +        grub_uint32_t imm20 = (off & 0x100000) << (31 - 20);
>> +        grub_uint32_t imm19_12 = (off & 0xff000);
>> +        grub_uint32_t imm11 = (off & 0x800) << (20 - 11);
>> +        grub_uint32_t imm10_1 = (off & 0x7fe) << (30 - 10);
>> +        *abs_place = (*abs_place & 0xfff)
>> +                     | imm20 | imm19_12 | imm11 | imm10_1;
> 
> Ditto.
> 
>> +      }
>> +      break;
>> +
>> +    case R_RISCV_CALL:
>> +      {
>> +        grub_uint32_t *abs_place = place;
>> +        grub_ssize_t off = sym_addr - (grub_addr_t) place;
>> +        grub_uint32_t hi20, lo12;
>> +
>> +        if (off != (grub_int32_t) off)
>> +          return grub_error (GRUB_ERR_BAD_MODULE, "relocation overflow");
>> +
>> +        hi20 = (off + 0x800) & 0xfffff000;
>> +        lo12 = (off - hi20) & 0xfff;
>> +        abs_place[0] = (abs_place[0] & 0xfff) | hi20;
>> +        abs_place[1] = (abs_place[1] & 0xfffff) | (lo12 << 20);
> 
> Ditto and below...
> 
>> +      }
>> +      break;
>> +
>> +    case R_RISCV_RVC_BRANCH:
>> +      {
>> +        grub_uint16_t *abs_place = place;
>> +        grub_ssize_t off = sym_addr - (grub_addr_t) place;
>> +        grub_uint16_t imm8 = (off & 0x100) << (12 - 8);
>> +        grub_uint16_t imm7_6 = (off & 0xc0) >> (6 - 5);
>> +        grub_uint16_t imm5 = (off & 0x20) >> (5 - 2);
>> +        grub_uint16_t imm4_3 = (off & 0x18) << (12 - 5);
>> +        grub_uint16_t imm2_1 = (off & 0x6) << (12 - 10);
>> +        *abs_place = (*abs_place & 0xe383)
>> +                     | imm8 | imm7_6 | imm5 | imm4_3 | imm2_1;
>> +      }
>> +      break;
>> +
>> +    case R_RISCV_RVC_JUMP:
>> +      {
>> +        grub_uint16_t *abs_place = place;
>> +        grub_ssize_t off = sym_addr - (grub_addr_t) place;
>> +        grub_uint16_t imm11 = (off & 0x800) << (12 - 11);
>> +        grub_uint16_t imm10 = (off & 0x400) >> (10 - 8);
>> +        grub_uint16_t imm9_8 = (off & 0x300) << (12 - 11);
>> +        grub_uint16_t imm7 = (off & 0x80) >> (7 - 6);
>> +        grub_uint16_t imm6 = (off & 0x40) << (12 - 11);
>> +        grub_uint16_t imm5 = (off & 0x20) >> (5 - 2);
>> +        grub_uint16_t imm4 = (off & 0x10) << (12 - 5);
>> +        grub_uint16_t imm3_1 = (off & 0xe) << (12 - 10);
>> +        *abs_place = ((*abs_place & 0xe003)
>> +                      | imm11 | imm10 | imm9_8 | imm7 | imm6
>> +                      | imm5 | imm4 | imm3_1);
>> +      }
>> +      break;
>> +
>> +    case R_RISCV_PCREL_HI20:
>> +      {
>> +        grub_uint32_t *abs_place = place;
>> +        grub_ssize_t off = sym_addr - (grub_addr_t) place;
>> +        grub_int32_t hi20;
>> +
>> +        if (off != (grub_int32_t)off)
>> +          return grub_error (GRUB_ERR_BAD_MODULE, "relocation overflow");
>> +
>> +        hi20 = (off + 0x800) & 0xfffff000;
>> +        *abs_place = (*abs_place & 0xfff) | hi20;
>> +      }
>> +    break;
>> +
>> +    case R_RISCV_PCREL_LO12_I:
>> +    case R_RISCV_PCREL_LO12_S:
>> +      {
>> +        grub_uint32_t *t32 = place;
>> +        Elf_Rela *rel2;
>> +        /* Search backwards for matching HI20 reloc.  */
> 
> Why "backwards"?

Because then the chance is higher that we hit a matching relocation
quickly :).


Alex