From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by mx.groups.io with SMTP id smtpd.web10.1899.1571137020233192886 for ; Tue, 15 Oct 2019 03:57:00 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=jmY4ze+O; spf=pass (domain: linaro.org, ip: 209.85.221.66, mailfrom: leif.lindholm@linaro.org) Received: by mail-wr1-f66.google.com with SMTP id p4so7392542wrm.8 for ; Tue, 15 Oct 2019 03:56:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=sWhds/Q7MTvZVb79nECZ34ay68HMTeg76x1A6nPOS4g=; b=jmY4ze+OYvUMHwovg6rBBTH0CiVsXQ3ti5s/SCij+lnvVheNE7xGrYbOi12+GPt/6H 8uNudlPwi9Sn1OCMbi0UL3D2ga8q13vCvrFECRjTGAka5KsXXmEmByppJrVx9rVD1Bji e5ZJKtjkJkwFjehP2N+OIPrF3NbGzqOIOMui3oq+neKhBtAMw07vwn5BmEPXurYsaOBT a4T9vwaYsGqWDObUwwm+lCtQtFordTmXzAINGY5yUqETuQRHZhQ6kT02v8+j2CAgkkDZ w0YaIn5TCe9/WNnFQLr1cBf+5stVLshqfmHFK+nSv8NqCVAmh6IgG7zYS8KyW7SLC4OU p9+w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=sWhds/Q7MTvZVb79nECZ34ay68HMTeg76x1A6nPOS4g=; b=E8RX4pn+QXVD9eWzIdMVMJ6DFD5ME5YGeq7LzFWV9VstC15+gG3Iiql13u17xiHJ5d 23HdNTYuWbxa+RusXixZDfph+vcDJxIdoO7CysKuhkXoIphKBZ3nhSXA3L7KEp/dVVuS of+3TTikpsW6m7jco/bSbqZ/UOI3OC7S65qNBcGGWYuSy0FnD8t7mpJt8iLBiHWXSH3f Cz4TL3RGkksldHgLJHDnKRfXPxBJjVfpRp9J8Ac3i7ra5vGXkxDs5z6PbSJUBmr2VGg/ dI2Bs9NiyvZwRIUgoMy1Mm6pHA/71WA1/DzmPz8fFnoP3pNryQBslXE6FCPi3uIJPepy 9lPw== X-Gm-Message-State: APjAAAUe4NmXL6eHuF9efdqzqWVQsXPEUMiqrU5hLie8yPJOh0c28x2M k6+MSc8YdaBeqZNwdD2Fra5SDJPnl4M= X-Google-Smtp-Source: APXvYqxhN4yoMZ+hIy4VuIcL4pOHTuLf3MvcibXjsCbdWlLNK8PKzReCRnakgjgWU/3qu/1X2R0Hpg== X-Received: by 2002:a5d:5743:: with SMTP id q3mr30501609wrw.394.1571137018211; Tue, 15 Oct 2019 03:56:58 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id b12sm16408666wrt.21.2019.10.15.03.56.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 15 Oct 2019 03:56:57 -0700 (PDT) Date: Tue, 15 Oct 2019 11:56:56 +0100 From: "Leif Lindholm" To: devel@edk2.groups.io, abner.chang@hpe.com Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 24/29] BaseTools: BaseTools changes for RISC-V platform. Message-ID: <20191015105656.GK25504@bivouac.eciton.net> References: <1569198715-31552-1-git-send-email-abner.chang@hpe.com> <1569198715-31552-26-git-send-email-abner.chang@hpe.com> <20190926220946.GV28454@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Oct 15, 2019 at 06:18:29AM +0000, Abner Chang wrote: > > -----Original Message----- > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > > Leif Lindholm > > Sent: Friday, September 27, 2019 6:10 AM > > To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist) > > > > 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? > > > > Other minor issues reviewed inline: > > > > > Signed-off-by: Abner Chang > > > --- > > > 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] > > > + > > > + *.lib > > > + > > > + > > > + $(MAKE_FILE) > > > + > > > + > > > + $(DEBUG_DIR)(+)$(MODULE_NAME).dll > > > + > > > + > > > + "$(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.
> > > Portions copyright (c) 2013-2014, ARM Ltd. All rights reserved.
> > > +Portions Copyright (c) 2016 - 2019 Hewlett Packard Enterprise > > Development LP. All rights reserved.
> > > > > > 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< > > +#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 *)""; > > > } > > > + 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