Re: [PATCH v3 09/10] RISC-V: Add to build system

[Bin Meng]

Hi Alex,

On Mon, Nov 26, 2018 at 7:06 AM Alexander Graf <address@hidden> wrote:
>
>
>
> On 18.11.18 12:41, Bin Meng wrote:
> > Hi Alex,
> >
> > On Thu, Nov 15, 2018 at 1:27 AM Alexander Graf <address@hidden> wrote:
> >>
> >> This patch adds support for RISC-V to the grub build system. With this
> >> patch, I can successfully build grub on RISC-V as a UEFI application.
> >>
> >> Signed-off-by: Alexander Graf <address@hidden>
> >> Reviewed-by: Alistair Francis <address@hidden>
> >>
> >> ---
> >>
> >> v2 -> v3:
> >>
> >>   - Fix riscv32 target
> >> ---
> >>  configure.ac                 | 28 ++++++++++++++++++++++++++--
> >>  gentpl.py                    | 11 +++++++----
> >>  grub-core/Makefile.am        | 12 ++++++++++++
> >>  grub-core/Makefile.core.def  | 29 +++++++++++++++++++++++++++++
> >>  grub-core/commands/file.c    | 14 +++++++++++++-
> >>  grub-core/kern/compiler-rt.c |  6 ++++--
> >>  grub-core/kern/efi/mm.c      |  2 +-
> >>  grub-core/kern/emu/cache.c   |  6 ++++++
> >>  grub-core/kern/emu/cache_s.S |  1 +
> >>  grub-core/kern/emu/lite.c    |  2 ++
> >>  grub-core/lib/efi/halt.c     |  3 ++-
> >>  grub-core/lib/setjmp.S       |  2 ++
> >>  include/grub/compiler-rt.h   | 12 ++++++++----
> >>  include/grub/efi/api.h       |  3 ++-
> >>  include/grub/efi/efi.h       |  2 +-
> >>  include/grub/misc.h          |  3 ++-
> >>  include/grub/util/install.h  |  2 ++
> >>  util/grub-install-common.c   |  2 ++
> >>  util/grub-install.c          | 28 ++++++++++++++++++++++++++++
> >>  util/grub-mknetdir.c         |  4 +++-
> >>  util/grub-mkrescue.c         | 16 +++++++++++++++-
> >>  util/mkimage.c               | 32 ++++++++++++++++++++++++++++++++
> >>  22 files changed, 200 insertions(+), 20 deletions(-)
> >>
> >
> > Reviewed-by: Bin Meng <address@hidden>
> > Tested-by: Bin Meng <address@hidden>
> >
> > But please see one comment below:
> >
> >> diff --git a/configure.ac b/configure.ac
> >> index 5e63c4af3..2cc2ffb2c 100644
> >> --- a/configure.ac
> >> +++ b/configure.ac
> >> @@ -104,6 +104,12 @@ case "$target_cpu" in
> >>    aarch64*)
> >>                 target_cpu=arm64
> >>                 ;;
> >> +  riscv32*)
> >> +               target_cpu=riscv32
> >> +               ;;
> >> +  riscv64*)
> >> +               target_cpu=riscv64
> >> +               ;;
> >>  esac
> >>
> >>  # Specify the platform (such as firmware).
> >> @@ -127,6 +133,8 @@ if test "x$with_platform" = x; then
> >>      ia64-*) platform=efi ;;
> >>      arm-*) platform=uboot ;;
> >>      arm64-*) platform=efi ;;
> >> +    riscv32-*) platform=efi ;;
> >> +    riscv64-*) platform=efi ;;
> >>      *)
> >>        AC_MSG_WARN([unsupported CPU: "$target_cpu" - only building 
> >> utilities])
> >>        platform=none
> >> @@ -174,6 +182,8 @@ case "$target_cpu"-"$platform" in
> >>    arm-coreboot) ;;
> >>    arm-efi) ;;
> >>    arm64-efi) ;;
> >> +  riscv32-efi) ;;
> >> +  riscv64-efi) ;;
> >>    *-emu) ;;
> >>    *-none) ;;
> >>    *) AC_MSG_ERROR([platform "$platform" is not supported for target CPU 
> >> "$target_cpu"]) ;;
> >> @@ -826,6 +836,16 @@ if test x"$platform" != xemu ; then
> >>         AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[]], [[]])],
> >>                          
> >> [grub_cv_target_cc_soft_float="-mgeneral-regs-only"], [])
> >>      fi
> >> +    if test "x$target_cpu" = xriscv32; then
> >> +       CFLAGS="$TARGET_CFLAGS -march=rv32imac -mabi=ilp32 -Werror"
> >> +       AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[]], [[]])],
> >> +                        [grub_cv_target_cc_soft_float="-march=rv32imac 
> >> -mabi=ilp32"], [])
> >> +    fi
> >> +    if test "x$target_cpu" = xriscv64; then
> >> +       CFLAGS="$TARGET_CFLAGS -march=rv64imac -mabi=lp64 -Werror"
> >> +       AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[]], [[]])],
> >> +                        [grub_cv_target_cc_soft_float="-march=rv64imac 
> >> -mabi=lp64"], [])
> >> +    fi
> >>      if test "x$target_cpu" = xia64; then
> >>         CFLAGS="$TARGET_CFLAGS -mno-inline-float-divide -mno-inline-sqrt 
> >> -Werror"
> >>         AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[]], [[]])],
> >> @@ -1141,7 +1161,7 @@ AC_SUBST(TARGET_LDFLAGS_OLDMAGIC)
> >>
> >>  LDFLAGS="$TARGET_LDFLAGS"
> >>
> >> -if test "$target_cpu" = x86_64 || test "$target_cpu" = sparc64 ; then
> >> +if test "$target_cpu" = x86_64 || test "$target_cpu" = sparc64 || test 
> >> "$target_cpu" = riscv64 ; then
> >>    # Use large model to support 4G memory
> >>    AC_CACHE_CHECK([whether option -mcmodel=large works], 
> >> grub_cv_cc_mcmodel, [
> >>      CFLAGS="$TARGET_CFLAGS -mcmodel=large"
> >> @@ -1151,7 +1171,7 @@ if test "$target_cpu" = x86_64 || test "$target_cpu" 
> >> = sparc64 ; then
> >>    ])
> >>    if test "x$grub_cv_cc_mcmodel" = xyes; then
> >>      TARGET_CFLAGS="$TARGET_CFLAGS -mcmodel=large"
> >> -  elif test "$target_cpu" = sparc64; then
> >> +  elif test "$target_cpu" = sparc64 || test "$target_cpu" = riscv64; then
> >>      TARGET_CFLAGS="$TARGET_CFLAGS -mcmodel=medany"
> >
> > The patch does not indicate the code model explicitly for riscv32.
> > It's better we pass the -mcmodel=medlow (suitable for riscv32) to the
> > compiler in case the compiler is configured with a different code
> > model by default.
>
> I'm afraid there is no other model?
>
>   https://gcc.gnu.org/onlinedocs/gcc/RISC-V-Options.html
>

Correct. However my comments were to request we explicitly pass GCC
the code model for 32-bit build too. Your patch only did such for
64-bit. We should do the same for 32-bit.

Regards,
Bin