[edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 24/29] BaseTools: BaseTools changes for RISC-V platform.
Abner Chang
abner.chang at hpe.com
Wed Oct 16 05:06:51 UTC 2019
> -----Original Message-----
> From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of
> Leif Lindholm
> Sent: Tuesday, October 15, 2019 6:57 PM
> To: devel at edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> <abner.chang at hpe.com>
> Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 24/29]
> BaseTools: BaseTools changes for RISC-V platform.
>
> On Tue, Oct 15, 2019 at 06:18:29AM +0000, Abner Chang wrote:
> > > -----Original Message-----
> > > From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf
> > > Of Leif Lindholm
> > > Sent: Friday, September 27, 2019 6:10 AM
> > > To: devel at edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> > > <abner.chang at hpe.com>
> > > Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 24/29]
> > > BaseTools: BaseTools changes for RISC-V platform.
> > >
> > > On Mon, Sep 23, 2019 at 08:31:50AM +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.
> > >
> > > Thank you, this is much cleaner.
> > > There are however some issues in this patch that prevent building on
> > > any platform. Please ensure to give a local build test before
> > > submitting a 3.
> > >
> > > First of all, this still does not contain the addition to
> > > BaseTools/Source/Python/Common/buildoptions.py that I mentioned in
> > > INVALID URI REMOVED
> > >
> 3A__edk2.groups.io_g_devel_message_47036&d=DwIBAg&c=C5b8zRQO1mi
> > >
> GmBeVZ2LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=
> > > YclXVT-
> > >
> dumczX_RwFNv_GDdWAp1gvJXUN0KRfNaGEtw&s=Gp1kHhT9Z6PR93PmPN
> > > ZD-_0h0rPDXLsODbhLWyQs8NA&e= - meaning that attempting to build
> > > anything for RISCV64 gives an error.
> >
> > I thought you were saying to use ENV(GCC5_RISCV64_PREFIX) to point to
> > build tool binaries, no?
>
> That is unrelated.
>
> I am talking about that the build command needs to be aware of the
> existence of the RISCV64 architecture. The version of the build command
> included in v2 cannot be used to build the tree. Just like I commented on,
> and provided the patch for, for v1. In the message linked to above.
>
> How *have* you tested the build without a working build command?
Ah I see.
You were saying -a in build command line. That was fixed in v3.
>
> > >
> > > Other minor issues reviewed inline:
> > >
> > > > Signed-off-by: Abner Chang <abner.chang at hpe.com>
> > > > ---
> > > > BaseTools/Conf/build_rule.template | 62 ++---
> > > > BaseTools/Conf/tools_def.template | 64 ++++-
> > > > BaseTools/Source/C/Common/BasePeCoff.c | 15 +-
> > > > BaseTools/Source/C/Common/PeCoffLoaderEx.c | 95 ++++++++
> > > > BaseTools/Source/C/GenFv/GenFvInternalLib.c | 128 +++++++++-
> > > > BaseTools/Source/C/GenFw/Elf32Convert.c | 5 +-
> > > > BaseTools/Source/C/GenFw/Elf64Convert.c | 260
> > > ++++++++++++++++++++-
> > > > BaseTools/Source/C/GenFw/elf_common.h | 62 +++++
> > > > .../Source/C/Include/IndustryStandard/PeImage.h | 6 +
> > > > BaseTools/Source/Python/Common/DataType.py | 7 +-
> > > > 10 files changed, 659 insertions(+), 45 deletions(-)
> > > >
> > > > diff --git a/BaseTools/Conf/build_rule.template
> > > b/BaseTools/Conf/build_rule.template
> > > > index db06d3a..fab3926 100755
> > > > --- a/BaseTools/Conf/build_rule.template
> > > > +++ b/BaseTools/Conf/build_rule.template
> > > > @@ -321,6 +314,21 @@
> > > > "$(OBJCOPY)" $(OBJCOPY_FLAGS) ${dst}
> > > >
> > > >
> > > > +[Static-Library-File.COMMON.RISCV64, Static-Library-
> > > File.COMMON.RISCV32]
> > > > + <InputFile>
> > > > + *.lib
> > > > +
> > > > + <ExtraDependency>
> > > > + $(MAKE_FILE)
> > > > +
> > > > + <OutputFile>
> > > > + $(DEBUG_DIR)(+)$(MODULE_NAME).dll
> > > > +
> > > > + <Command.GCC>
> > > > + "$(DLINK)" -o ${dst} $(DLINK_FLAGS) --start-group
> > > > + $(DLINK_SPATH)
> > > @$(STATIC_LIBRARY_FILES_LIST) --end-group $(DLINK2_FLAGS)
> > >
> > > This line looks to me like the only thing that is actually changed
> > > here, and I am not convinced it is necessary.
> > > "$(DLINK)" -o ${dst} $(DLINK_FLAGS) -Wl,--start-
> > > group,@$(STATIC_LIBRARY_FILES_LIST),--end-group $(CC_FLAGS)
> > > $(DLINK2_FLAGS)
> > >
> > > On the ARM/AARCH64 side, we use gcc as the DLINK, and pass the
> > > required flags through to the linker with -Wl. Please have a look
> > > and try to rework at that end rather than fundamentally revamping
> > > the basic build rules differently for RISCV than other architectures.
> > >
> > > Basically, please discard all changes to this file, apply the below
> > > diff, and rework the flags to resolve the builds. (Basically, add a
> > > bunch of -Wl,)
> >
> > I got build error when use -Wl with the specific version of RISC-V GCC
> > toolchain (the old and workable one). I will revisit this when I
> > investigate the issue caused by latest RISC-V build tool.
>
> OK.
>
> > > > diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c
> > > b/BaseTools/Source/C/GenFw/Elf64Convert.c
> > > > index 3d6319c..2aa09fd 100644
> > > > --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
> > > > +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
> > > > @@ -3,6 +3,7 @@ Elf64 convert solution
> > > >
> > > > Copyright (c) 2010 - 2018, Intel Corporation. All rights
> > > > reserved.<BR> Portions copyright (c) 2013-2014, ARM Ltd. All
> > > > rights reserved.<BR>
> > > > +Portions Copyright (c) 2016 - 2019 Hewlett Packard Enterprise
> > > Development LP. All rights reserved.<BR>
> > > >
> > > > SPDX-License-Identifier: BSD-2-Clause-Patent
> > > >
> > > > @@ -31,6 +32,12 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > #include "ElfConvert.h"
> > > > #include "Elf64Convert.h"
> > > >
> > > > +#define RV_X(x, s, n) (((x) >> (s)) & ((1<<(n))-1)) #define
> > > > +RISCV_IMM_BITS 12 #define RISCV_IMM_REACH
> (1LL<<RISCV_IMM_BITS)
> > > > +#define RISCV_CONST_HIGH_PART(VALUE) \
> > > > + (((VALUE) + (RISCV_IMM_REACH/2)) & ~(RISCV_IMM_REACH-1))
> > > > +
> > > > STATIC
> > > > VOID
> > > > ScanSections64 (
> > > > @@ -153,8 +160,8 @@ InitializeElf64 (
> > > > Error (NULL, 0, 3000, "Unsupported", "ELF e_type not ET_EXEC
> > > > or
> > > ET_DYN");
> > > > return FALSE;
> > > > }
> > > > - if (!((mEhdr->e_machine == EM_X86_64) || (mEhdr->e_machine ==
> > > EM_AARCH64))) {
> > > > - Error (NULL, 0, 3000, "Unsupported", "ELF e_machine not
> EM_X86_64 or
> > > EM_AARCH64");
> > > > + if (!((mEhdr->e_machine == EM_X86_64) || (mEhdr->e_machine ==
> > > EM_AARCH64) || (mEhdr->e_machine == EM_RISCV64))) {
> > > > + Error (NULL, 0, 3000, "Unsupported", "ELF e_machine is not
> > > > + Elf64
> > > machine.");
> > > > return FALSE;
> > > > }
> > > > if (mEhdr->e_version != EV_CURRENT) { @@ -481,6 +488,7 @@
> > > > ScanSections64 (
> > > > switch (mEhdr->e_machine) {
> > > > case EM_X86_64:
> > > > case EM_AARCH64:
> > > > + case EM_RISCV64:
> > > > mCoffOffset += sizeof (EFI_IMAGE_NT_HEADERS64);
> > > > break;
> > > > default:
> > > > @@ -690,6 +698,11 @@ ScanSections64 (
> > > > NtHdr->Pe32Plus.FileHeader.Machine =
> > > EFI_IMAGE_MACHINE_AARCH64;
> > > > NtHdr->Pe32Plus.OptionalHeader.Magic =
> > > EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
> > > > break;
> > > > + case EM_RISCV64:
> > > > + NtHdr->Pe32Plus.FileHeader.Machine =
> > > EFI_IMAGE_MACHINE_RISCV64;
> > > > + NtHdr->Pe32Plus.OptionalHeader.Magic =
> > > EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
> > > > + break;
> > > > +
> > > > default:
> > > > VerboseMsg ("%s unknown e_machine type. Assume X64",
> > > (UINTN)mEhdr->e_machine);
> > > > NtHdr->Pe32Plus.FileHeader.Machine = EFI_IMAGE_MACHINE_X64;
> > > > @@ -769,6 +782,11 @@ WriteSections64 (
> > > > Elf_Shdr *SecShdr;
> > > > UINT32 SecOffset;
> > > > BOOLEAN (*Filter)(Elf_Shdr *);
> > > > + UINT32 Value;
> > > > + UINT32 Value2;
> > > > + UINT8 *Pass1Targ = NULL;
> > > > + Elf_Shdr *Pass1Sym = NULL;
> > > > + Elf64_Half Pass1SymSecIndex = 0;
> > > > Elf64_Addr GOTEntryRva;
> > > >
> > > > //
> > > > @@ -893,13 +911,14 @@ WriteSections64 (
> > > > if (SymName == NULL) {
> > > > SymName = (const UINT8 *)"<unknown>";
> > > > }
> > > > + if (mEhdr->e_machine != EM_RISCV64) {
> > >
> > > This needs a comment explaining why this does not apply to RISCV.
> >
> > >
> > > > + Error (NULL, 0, 3000, "Invalid",
> > > > + "%s: Bad definition for symbol '%s'@%#llx or
> > > > + unsupported
> > > symbol type. "
> > > > + "For example, absolute and undefined symbols
> > > > + are not
> > > supported.",
> > > > + mInImageName, SymName, Sym->st_value);
> > > >
> > > > - Error (NULL, 0, 3000, "Invalid",
> > > > - "%s: Bad definition for symbol '%s'@%#llx or unsupported
> symbol
> > > type. "
> > > > - "For example, absolute and undefined symbols are not
> > > supported.",
> > > > - mInImageName, SymName, Sym->st_value);
> > > > -
> > > > - exit(EXIT_FAILURE);
> > > > + exit(EXIT_FAILURE);
> > > > + }
> > > > }
> > > > SymShdr = GetShdrByIndex(Sym->st_shndx);
> > > >
> > > > @@ -1114,6 +1133,128 @@ WriteSections64 (
> > > > default:
> > > > Error (NULL, 0, 3000, "Invalid", "WriteSections64():
> > > > %s unsupported
> > > ELF EM_AARCH64 relocation 0x%x.", mInImageName, (unsigned)
> > > ELF_R_TYPE(Rel->r_info));
> > > > }
> > > > + } else if (mEhdr->e_machine == EM_RISCV64) {
> > >
> > > Yeah, this code block is just *waaaay* too big.
> > > Please break it out into its own helper function.
> >
> > Leif, I am not going to address this issue this time. I just follow
> > what other archs done in this function. I agree with you this
> > function is way too long. I could create a task to refine this
> > function once RISC-V part is reviewed and pushed to the mainstream.
>
> I don't understand this logic.
> Breaking this out into a helper function would take no more time than you
> spent on typing the above response that you don't intend to do.
>
> Yes, the code for the other architectures is also bad, and we should revisit
> and fix it. But that doesn't mean we should keep making the file worse.
>
> I am already giving a pass on how even if you break this hunk out into its own
> helper function, that one is itself way too long and needs to be broken up.
> But at least if we move it out, we've compartmentalised the problem.
>
> Merging something bad in order to fix it later is never the answer.
> (And not only because in 95% in cases, that later never happens.)
>
> Best Regards,
>
> Leif
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#49049): https://edk2.groups.io/g/devel/message/49049
Mute This Topic: https://groups.io/mt/34258222/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list