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

[Daniel Kiper]

On Mon, Nov 26, 2018 at 12:38:12AM +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>

Except two nitpicks
  Reviewed-by: Daniel Kiper <address@hidden>

> ---
>
> v2 -> v3:
>
>   - Fix riscv32 target
>
> v3 -> v4:
>
>   - Change copyright from 2013 to 2018
>   - Add spec reference
> ---
>  grub-core/kern/dl.c         |   6 +-
>  grub-core/kern/riscv/dl.c   | 340 
> ++++++++++++++++++++++++++++++++++++++++++++
>  include/grub/dl.h           |   6 +-
>  util/grub-mkimagexx.c       | 268 ++++++++++++++++++++++++++++++++++
>  util/grub-module-verifier.c |  56 ++++++++
>  5 files changed, 671 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)
>    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..6fb8385ef
> --- /dev/null
> +++ b/grub-core/kern/riscv/dl.c
> @@ -0,0 +1,340 @@
> +/* dl.c - arch-dependent part of loadable module support */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2018  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/dl.h>
> +#include <grub/elf.h>
> +#include <grub/misc.h>
> +#include <grub/err.h>
> +#include <grub/mm.h>
> +#include <grub/i18n.h>
> +
> +/*
> + * Instructions and instruction encoding are documented in the RISC-V
> + * specification. This file is based on version 2.2:
> + *
> + * 
> https://github.com/riscv/riscv-isa-manual/blob/master/release/riscv-spec-v2.2.pdf
> + */

May I ask you to add the same comment to util/grub-mkimagexx.c in the
relevant places?

> +#define LDR 0x58000050
> +#define BR 0xd61f0200
> +
> +/*
> + * 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;
> +}

[...]

> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> index a483c674c..a94d03540 100644
> --- a/util/grub-mkimagexx.c
> +++ b/util/grub-mkimagexx.c
> @@ -1188,6 +1188,205 @@ SUFFIX (relocate_addrs) (Elf_Ehdr *e, struct 
> section_metadata *smd,
>                break;
>              }
>  #endif /* MKIMAGE_ELF32 */
> +          case EM_RISCV:
> +            {
> +              grub_uint64_t *t64 = (grub_uint64_t *) target;
> +              grub_uint32_t *t32 = (grub_uint32_t *) target;
> +              grub_uint16_t *t16 = (grub_uint16_t *) target;
> +              grub_uint8_t *t8 = (grub_uint8_t *) target;
> +              grub_int64_t off = (long)sym_addr - target_section_addr - 
> offset
> +                                 - image_target->vaddr_offset;
> +
> +              sym_addr += addend;
> +
> +              switch (ELF_R_TYPE (info))
> +                {
> +                case R_RISCV_ADD8:
> +                  {
> +                    *t8 = *t8 + sym_addr;
> +                  }

I think that you can drop all curly braces in cases like that.

> +                  break;
> +                case R_RISCV_ADD16:
> +                  {
> +                    *t16 = grub_host_to_target16 (grub_target_to_host16 
> (*t16) + sym_addr);
> +                  }

Ditto.

> +                  break;
> +                case R_RISCV_32:
> +                case R_RISCV_ADD32:
> +                  {
> +                    *t32 = grub_host_to_target32 (grub_target_to_host32 
> (*t32) + sym_addr);
> +                  }

Ditto and below.

Daniel