Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v1 11/22]: BaseTools: BaseTools changes for RISC-V platform.

["Leif Lindholm" <leif.lindholm@linaro.org>]

On Mon, Sep 16, 2019 at 07:46:26AM +0000, Abner Chang wrote:
> > -----Original Message-----
> > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > Sent: Monday, September 9, 2019 7:37 PM
> > To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> > <abner.chang@hpe.com>
> > Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v1 11/22]:
> > BaseTools: BaseTools changes for RISC-V platform.
> > 
> > Hi Abner,
> > 
> > Having actually tried to build things, I have come across a bunch of
> > issues with this patch I missed on my (very cursory) ocular review.
> > 
> > On Wed, Sep 04, 2019 at 06:43:06PM +0800, Abner Chang wrote:
> > > BaseTools changes for building EDK2 RISC-V platform.
> > > The changes made to build_rule.template is to avoid build errors cause by
> > GCC711RISCV tool chain.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > Signed-off-by: Abner Chang <abner.chang@hpe.com>
> > > ---
> > >  BaseTools/Conf/build_rule.template                 |   23 +-
> > >  BaseTools/Conf/tools_def.template                  |  108 +-
> > >  BaseTools/Source/C/Common/BasePeCoff.c             |   19 +-
> > >  BaseTools/Source/C/Common/PeCoffLoaderEx.c         |   96 ++
> > >  BaseTools/Source/C/GenFv/GenFvInternalLib.c        |  281 ++++-
> > >  BaseTools/Source/C/GenFw/Elf32Convert.c            |    6 +-
> > >  BaseTools/Source/C/GenFw/Elf64Convert.c            |  273 ++++-
> > >  BaseTools/Source/C/GenFw/elf_common.h              |   63 ++
> > >  .../Source/C/Include/IndustryStandard/PeImage.h    |   10 +
> > >  BaseTools/Source/Python/Common/DataType.py         | 1075
> > ++++++++++----------
> > >  10 files changed, 1393 insertions(+), 561 deletions(-)
> > >
> > > diff --git a/BaseTools/Conf/tools_def.template
> > b/BaseTools/Conf/tools_def.template
> > > index 8f0e6cb..36a301a 100755
> > > --- a/BaseTools/Conf/tools_def.template
> > > +++ b/BaseTools/Conf/tools_def.template
> > > @@ -3,7 +3,7 @@
> > >  #  Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
> > >  #  Portions copyright (c) 2011 - 2014, ARM Ltd. All rights reserved.<BR>
> > >  #  Copyright (c) 2015, Hewlett-Packard Development Company, L.P.<BR>
> > > -#  (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> > > +#  (C) Copyright 2016-2019 Hewlett Packard Enterprise Development
> > LP<BR>
> > >  #
> > >  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> > >  #
> > > @@ -114,6 +114,12 @@ DEFINE GCC49_X64_PREFIX        = ENV(GCC49_BIN)
> > >  DEFINE GCC5_IA32_PREFIX        = ENV(GCC5_BIN)
> > >  DEFINE GCC5_X64_PREFIX         = ENV(GCC5_BIN)
> > >  DEFINE GCC_HOST_PREFIX         = ENV(GCC_HOST_BIN)
> > > +#
> > > +# RISC-V GCC toolchain
> > > +# This is the default directory used when install official riscv-tools.
> > > +#
> > > +DEFINE GCCRISCV_RISCV32_PREFIX = ENV(GCC_RISCV32_BIN)
> > > +DEFINE GCCRISCV_RISCV64_PREFIX = ENV(GCC_RISCV64_BIN)
> > 
> > If at all possible, I would strongly recommend *not* following the x86
> > _BIN example, and instead using ENV(<toolchain>_RISCV64_PREFIX) like
> > the ARM/AARCH64 profiles.
> > 
> > >  DEFINE UNIX_IASL_BIN           = ENV(IASL_PREFIX)iasl
> > >  DEFINE WIN_IASL_BIN            = ENV(IASL_PREFIX)iasl.exe
> > > @@ -236,6 +242,15 @@ DEFINE DTC_BIN                 = ENV(DTC_PREFIX)dtc
> > >  #                             Required to build platforms or ACPI tables:
> > >  #                               Intel(r) ACPI Compiler from
> > >  #                               https://acpica.org/downloads
> > > +#   GCCRISCV    - Linux       -  Requires:
> > > +#                             RISC-V official release of RISC-V GNU toolchain,
> > > +#                               https://github.com/riscv/riscv-gnu-toolchain @64879b24
> > > +#                               The commit ID 64879b24 is the one can build RISC-V
> > platform and boo to EFI shell.
> > > +#                               Follow the instructions mentioned in README.md to
> > build RISC-V tool change.
> > > +#                             Set below environment variables to the RISC-V tool chain
> > binaries before building RISC-V EDK2 port.
> > > +#                               - GCC_RISCV32_BIN
> > > +#                               - GCC_RISCV64_BIN
> > > +#
> > >  #   CLANG35     -Linux,Windows-  Requires:
> > >  #                             Clang v3.5 or later, and GNU binutils targeting aarch64-
> > linux-gnu or arm-linux-gnueabi
> > >  #                        Optional:
> > > @@ -1806,6 +1821,26 @@ DEFINE GCC5_ARM_ASLDLINK_FLAGS       =
> > DEF(GCC49_ARM_ASLDLINK_FLAGS)
> > >  DEFINE GCC5_AARCH64_ASLDLINK_FLAGS   =
> > DEF(GCC49_AARCH64_ASLDLINK_FLAGS)
> > >  DEFINE GCC5_ASLCC_FLAGS              = DEF(GCC49_ASLCC_FLAGS) -fno-lto
> > >
> > > +DEFINE GCC_RISCV_ALL_CC_FLAGS                    = -g -fshort-wchar -fno-
> > strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-
> > sections -c -include AutoGen.h -fno-common -
> > DSTRING_ARRAY_NAME=$(BASE_NAME)Strings
> > > +DEFINE GCC_RISCV_ALL_DLINK_COMMON                = -nostdlib -n -q --gc-
> > sections -z common-page-size=0x40
> > > +DEFINE GCC_RISCV_ALL_DLINK_FLAGS                 =
> > DEF(GCC_RISCV_ALL_DLINK_COMMON) --entry $(IMAGE_ENTRY_POINT) -u
> > $(IMAGE_ENTRY_POINT) -Map $(DEST_DIR_DEBUG)/$(BASE_NAME).map
> > > +DEFINE GCC_RISCV_ALL_DLINK2_FLAGS                = --
> > defsym=PECOFF_HEADER_SIZE=0x220 --
> > script=$(EDK_TOOLS_PATH)/Scripts/GccBaseRiscV.lds
> > > +DEFINE GCC_RISCV_ALL_ASM_FLAGS                   = -c -x assembler -imacros
> > $(DEST_DIR_DEBUG)/AutoGen.h
> > > +DEFINE GCC_RISCV_RISCV32_DLINK2_FLAGS            = --
> > defsym=PECOFF_HEADER_SIZE=0x220 DEF(GCC_DLINK2_FLAGS_COMMON)
> > > +
> > > +DEFINE GCCRISCV_RISCV32_ARCH                   = rv32imafdc
> > > +DEFINE GCCRISCV_RISCV64_ARCH                   = rv64imafdc
> > > +DEFINE GCCRISCV_CC_FLAGS_WARNING_DISABLE       = -Wno-
> > tautological-compare -Wno-pointer-compare
> > > +DEFINE GCCRISCV_RISCV32_CC_FLAGS               =
> > DEF(GCC_RISCV_ALL_CC_FLAGS)
> > DEF(GCCRISCV_CC_FLAGS_WARNING_DISABLE) -
> > march=DEF(GCCRISCV_RISCV32_ARCH) -malign-double -fno-stack-protector
> > -D EFI32 -fno-asynchronous-unwind-tables -Wno-address -Wno-unused-but-
> > set-variable -fpack-struct=8
> > > +DEFINE GCCRISCV_RISCV64_CC_FLAGS               =
> > DEF(GCC_RISCV_ALL_CC_FLAGS)
> > DEF(GCCRISCV_CC_FLAGS_WARNING_DISABLE) -
> > march=DEF(GCCRISCV_RISCV64_ARCH) -fno-builtin -fno-builtin-memcpy -
> > fno-stack-protector -Wno-address -fno-asynchronous-unwind-tables -Wno-
> > unused-but-set-variable -fpack-struct=8 -mcmodel=medany -mabi=lp64
> > > +DEFINE GCCRISCV_RISCV32_RISCV64_DLINK_COMMON   = -nostdlib -n -q
> > --gc-sections -z common-page-size=0x40
> > > +DEFINE GCCRISCV_RISCV32_RISCV64_ASLDLINK_FLAGS =
> > DEF(GCC_RISCV_ALL_DLINK_COMMON) --entry ReferenceAcpiTable -u
> > ReferenceAcpiTable
> > > +DEFINE GCCRISCV_RISCV32_RISCV64_DLINK_FLAGS    =
> > DEF(GCC_RISCV_ALL_DLINK_COMMON) --entry $(IMAGE_ENTRY_POINT) -u
> > $(IMAGE_ENTRY_POINT) -Map $(DEST_DIR_DEBUG)/$(BASE_NAME).map
> > > +DEFINE GCCRISCV_RISCV32_DLINK2_FLAGS           =
> > DEF(GCC_RISCV_RISCV32_DLINK2_FLAGS)
> > > +DEFINE GCCRISCV_RISCV64_DLINK_FLAGS            =
> > DEF(GCC_RISCV_ALL_DLINK_FLAGS)  -melf64lriscv --oformat=elf64-littleriscv
> > --no-relax
> > > +DEFINE GCCRISCV_RISCV64_DLINK2_FLAGS           =
> > DEF(GCC_RISCV_ALL_DLINK2_FLAGS)
> > > +DEFINE GCCRISCV_ASM_FLAGS                      =
> > DEF(GCC_RISCV_ALL_ASM_FLAGS) -march=DEF(GCCRISCV_RISCV64_ARCH)
> > -mcmodel=medany -mabi=lp64
> > > +
> > >
> > ##########################################################
> > ##########################
> > >  #
> > >  # GCC 4.8 - This configuration is used to compile under Linux to produce
> > > @@ -2247,6 +2282,77 @@ RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS = -z
> > common-page-size=0x20
> > >    NOOPT_GCC5_AARCH64_DLINK_FLAGS =
> > DEF(GCC5_AARCH64_DLINK_FLAGS) -O0
> > >    NOOPT_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
> > -O0
> > >
> > >
> > +#########################################################
> > ##########################
> > >
> > +#########################################################
> > ###########################
> > > +#
> > > +# GCC RISC-V This configuration is used to compile under Linux to produce
> > > +#             PE/COFF binaries using GCC RISC-V tool chain
> > > +#         https://github.com/riscv/riscv-gnu-toolchain @64879b24
> > > +# The commit ID 64879b24 is the one can build RISC-V platform and boo to
> > EFI shell.
> > > +#
> > >
> > +#########################################################
> > ###########################
> > 
> > Please don't do this. This mistake was made for the ARM port, and it
> > caused us nothing but pain.
> > 
> > Please add the requisite support to the GCC5 profile instead. (Which
> > is not actually for gcc 5, but is effectively GCC5+ - we are still
> > successfully using it with gcc 9.)
> 
> I can try to use GCC5 profile but the toolchain still has to be
> stick on https://github.com/riscv/riscv-gnu-toolchain @64879b24. We
> got problem on the version higher than this, system hangs at SEC to
> PEI transition if use GCC version higher than @64879b24.
>
> We will figure it out later.

I suppose this is fine as long as this is specifically on the
edk2-staging branch. We will need to resolve it before we bring the
port to edk2 master.

I would still like this support to be tweaked to the point where I can
build with either the Fedora or the Debian packaged cross compiler.

> So how can I mention this restrictions in tool_def?

I would suggest the following:
- The above information in the top-level Readme.md on the staging branch.
- A single line in the "GCC5 RISCV64 definitions" comment block.
- Adding the above to the commit message.

Best Regards,

Leif