From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web10.7025.1602746055222344195 for ; Thu, 15 Oct 2020 00:14:15 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=VBoucOF2; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1602746054; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=RiycL3cid1rX2auIsLXxPcY7JMbJVqZXt0NqNsfxKvo=; b=VBoucOF2b/E2fjxhth+fc+ECiRwUKNJYXQnPZXpe8AbIMof2Xin5hJFKdpth22wCMSO30X zgcQbiXJ6fBRTaMxX975+/XjYwZOgrRg0YzvowtToRp8323MAUoUqsFzXEQ5Ngol3a5CL0 UxcP+2ObLUtZ1Doh5J56jKcepjp9+Zk= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-418-kqzODP75PVOHM9HwZsRc2Q-1; Thu, 15 Oct 2020 03:14:09 -0400 X-MC-Unique: kqzODP75PVOHM9HwZsRc2Q-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id E8B3218A8221; Thu, 15 Oct 2020 07:14:07 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-119.ams2.redhat.com [10.36.113.119]) by smtp.corp.redhat.com (Postfix) with ESMTP id E808C19C4F; Thu, 15 Oct 2020 07:14:05 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 1/2] CryptoPkg/OpensslLib: Add native instruction support for X64 To: "Zurcher, Christopher J" , "devel@edk2.groups.io" , "Yao, Jiewen" , "Jiang, Guomin" Cc: "Wang, Jian J" , "Lu, XiaoyuX" , "Ard Biesheuvel (ARM address)" References: <20200804002429.3897-1-christopher.j.zurcher@intel.com> <162C7E6ED8CEF542.12673@groups.io> <1ce6123c-7616-30ad-07a0-30b6a5b51dec@redhat.com> <18e31a95-85e4-9c76-bdc1-5ffa0d32d9cd@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 15 Oct 2020 09:14:05 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/09/20 21:27, Zurcher, Christopher J wrote: > Here is the error message: > [...]/OpensslLibX64/OUTPUT/X64/crypto/aes/aesni-mb-x86_64.iii:1746: error: symbol `..imagebase' undefined > [cut 18 lines] > [...]/OpensslLibX64/OUTPUT/X64/crypto/aes/aesni-mb-x86_64.iii:1775: error: symbol `..imagebase' undefined > GNUmakefile:3390: recipe for target '[...]OpensslLibX64/OUTPUT/X64/crypto/aes/aesni-mb-x86_64.obj' failed > make: *** [[...]/OpensslLibX64/OUTPUT/X64/crypto/aes/aesni-mb-x86_64.obj] Error 1 > > The functionality is described here in "7.6.1 win64: Writing Position-Independent Code" and "7.6.2 win64: Structured Exception Handling" > https://www.nasm.us/xdoc/2.13.02rc3/html/nasmdoc7.html > > The x86_64 implementation in OpenSSL seems to assume that building with NASM guarantees a Windows toolchain and Windows execution environment. Thank you. I have no more ideas or questions. Please proceed as you suggested. Laszlo >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of Laszlo Ersek >> Sent: Friday, October 9, 2020 04:37 >> To: Zurcher, Christopher J ; >> devel@edk2.groups.io; Yao, Jiewen ; Jiang, Guomin >> >> Cc: Wang, Jian J ; Lu, XiaoyuX ; >> Ard Biesheuvel (ARM address) >> Subject: Re: [edk2-devel] [PATCH v2 1/2] CryptoPkg/OpensslLib: Add native >> instruction support for X64 >> >> On 10/08/20 21:56, Zurcher, Christopher J wrote: >>> Laszlo, thanks for sharing this explanation and history. I have found that >> in addition to the "common" declaration, OpenSSL's Structured Exception >> Handling functionality also breaks the GCC build by including "wrt >> ..imagebase" statements. Since we cannot implement functional changes in the >> current 1.1.1x versions of OpenSSL, my proposal is to go ahead with this >> patch only supporting VS and LLVM toolchains for now. >> >> Could you include the error message with the "wrt ..imagebase" string? >> >> I found the string in "crypto/perlasm/x86_64-xlate.pl" but don't really >> understand what it's about. >> >> I'd just like us to make one attempt to resolve that problem; otherwise >> personally I'm OK if this new feature is not enabled for GCC at first. >> >> Thanks >> Laszlo >> >>> >>> Thanks, >>> Christopher Zurcher >>> >>>> -----Original Message----- >>>> From: Laszlo Ersek >>>> Sent: Thursday, October 1, 2020 05:58 >>>> To: devel@edk2.groups.io; Zurcher, Christopher J >>>> ; Yao, Jiewen ; >> Jiang, >>>> Guomin >>>> Cc: Wang, Jian J ; Lu, XiaoyuX >> ; >>>> Ard Biesheuvel (ARM address) >>>> Subject: Re: [edk2-devel] [PATCH v2 1/2] CryptoPkg/OpensslLib: Add native >>>> instruction support for X64 >>>> >>>> (refreshing Ard's address, comments below) >>>> >>>> On 09/29/20 23:08, Zurcher, Christopher J wrote: >>>>> The GCC build fails with this error: >>>>> >>>>> `OPENSSL_ia32cap_P' referenced in section `.text.OPENSSL_cpuid_setup' >>>>> of /tmp/ccIIRAYs.ltrans20.ltrans.o: defined in discarded section >>>>> `COMMON' of >>>>> >>>> >> /mnt/c/mssql/tiano/Build/OvmfX64/DEBUG_GCC5/X64/CryptoPkg/Library/OpensslLib/ >>>> OpensslLibX64/OUTPUT/OpensslLibX64.lib(x86_64cpuid.obj) >>>>> >>>>> The code in question is here: >>>>>> section .CRT$XCU rdata align=8 >>>>>> DQ OPENSSL_cpuid_setup >>>>>> >>>>>> common OPENSSL_ia32cap_P 16 >>>> >>>> For the X64 arch, OPENSSL_cpuid_setup() is implemented in >>>> >>>> CryptoPkg/Library/OpensslLib/openssl/crypto/cryptlib.c >>>> >>>> It makes references to: >>>> >>>> extern unsigned int OPENSSL_ia32cap_P[4]; >>>> >>>> The variable is defined in generated assembly source code. >>>> >>>> There seem to be multiple generators (for various assemblers): >>>> >>>> (1) crypto/perlasm/x86gas.pl -- likely for the GNU assembler: >>>> >>>>> my $tmp=".comm\t${nmdecor}OPENSSL_ia32cap_P,16"; >>>> >>>> (2) crypto/perlasm/x86nasm.pl -- likely for NASM: >>>> >>>>> ${drdecor}common ${nmdecor}OPENSSL_ia32cap_P 16 >>>> >>>> (3) crypto/x86_64cpuid.pl -- likely for... ??? >>>> >>>>> .comm OPENSSL_ia32cap_P,16,4 >>>> >>>> They all put the variable in the "common" section. >>>> >>>> Tracking the NASM generator through a number of "git blame" commands, >>>> I've ended up at historical commit 10e7d6d52650 ("Support for IA-32 SSE2 >>>> instruction set.", 2004-05-06). This commit introduced "OPENSSL_ia32cap" >>>> at once in the common section -- see "crypto/perlasm/x86unix.pl". >>>> >>>> Now, the NASM manual says the following about the common section: >>>> >>>>> 6.7. 'COMMON': Defining Common Data Areas >>>>> ========================================= >>>>> >>>>> The 'COMMON' directive is used to declare _common variables_. A common >>>>> variable is much like a global variable declared in the uninitialized >>>>> data section, so that >>>>> >>>>> common intvar 4 >>>>> >>>>> is similar in function to >>>>> >>>>> global intvar >>>>> section .bss >>>>> >>>>> intvar resd 1 >>>>> >>>>> The difference is that if more than one module defines the same >>>>> common variable, then at link time those variables will be _merged_, and >>>>> references to 'intvar' in all modules will point at the same piece of >>>>> memory. >>>> >>>> The common section is a *really* bad idea for C language projects, >>>> because if there are multiple external definitions of an object in a >>>> program, then that should (per C language standard) prevent the >>>> successful linking of the program, rather than undergo silent definition >>>> merging. >>>> >>>> This has caused actual, inexplicable bugs in edk2 -- identically named, >>>> but differently sized, and entirely independently inteded, variables >>>> with external linkage and static storage duration got silently merged, >>>> rather than breaking the build. In the end, we tracked those down and >>>> marked them all STATIC. But in order to prevent such nonsense in the >>>> future, we also forbade the common section altogether. Let me find that >>>> commit... >>>> >>>> Yes, please see 214a3b79417f ("BaseTools GCC: avoid the use of COMMON >>>> symbols", 2015-12-08). >>>> >>>> So, my guess is that this interferes with OpenSSL's placing of >>>> "OPENSSL_ia32cap_P" in the common section. >>>> >>>> Without knowing more, I'd hazard that this is a bug in OpenSSL. Unless >>>> they have a strong reason for it, I think we should try to contribute a >>>> patch that removes "common". >>>> >>>> The code should provide exactly one definition (in the generated >>>> assembly source), provide one central (extern) declaration too, in a >>>> header file, then let all users include the declaration via the header >>>> file. The object file built from the generated assembly source should be >>>> linked into each final executable. >>>> >>>> For example, "CryptoPkg/Library/OpensslLib/openssl/crypto/cryptlib.c" >>>> already correctly declares the variable as "extern". >>>> >>>> Otherwise, as last resort, I guess we could attempt working it around by >>>> adding back "-fcommon" to the OpensslLib build flags. :/ >>>> >>>> Thanks, >>>> Laszlo >>> >> >> >> >> >> >