Re: [edk2-devel] [PATCH v5 0/2] CryptoPkg/OpensslLib: Add native instruction support for X64

["Michael D Kinney" <michael.d.kinney@intel.com>]

Are you using the latest version of NASM in the Linux environment?  You may have to build it from sources.

Mike

> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Friday, November 6, 2020 6:24 PM
> To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>
> Cc: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; gaoliming <gaoliming@byosoft.com.cn>; Wang, Jian J
> <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Ard
> Biesheuvel <ard.biesheuvel@arm.com>
> Subject: Re: [edk2-devel] [PATCH v5 0/2] CryptoPkg/OpensslLib: Add native instruction support for X64
> 
> The reason we choose NASM is that we can use same assembly in windows build and Linux build. However if this NASM cannot
> be used in Linux, then the benefit does not exist any more. You can generate GAS to support GCC build, and check in .S
> file.
> 
> thank you!
> Yao, Jiewen
> 
> 
> > 在 2020年11月7日，上午10:02，Zurcher, Christopher J <christopher.j.zurcher@intel.com> 写道：
> >
> > Their options would be:
> > 1) File a bug against OpenSSL that their NASM code is too advanced for GCC toolchain (unlikely to result in any change
> since OpenSSL would say GCC build should be using GAS instead of NASM and building outside of their makefile build system
> is bad practice in the first place).
> > 2) File a bug against GCC that their toolchain can't handle NASM correctly.
> > 3) Use CLANGPDB toolchain instead.
> >
> > I am not familiar enough with the GCC toolchain to know if there is any way to make it interpret NASM files correctly,
> so there is a chance that someone who knows more about it could provide some configuration change that could resolve this.
> But I do not think we should say that VS and CLANGPDB toolchain users can't have OpenSSL acceleration because GCC can't
> handle it.
> >
> > Thanks,
> > Christopher Zurcher
> >
> >> -----Original Message-----
> >> From: Yao, Jiewen <jiewen.yao@intel.com>
> >> Sent: Friday, November 6, 2020 15:07
> >> To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>;
> >> devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; gaoliming
> >> <gaoliming@byosoft.com.cn>
> >> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>;
> >> Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel
> >> <ard.biesheuvel@arm.com>
> >> Subject: RE: [edk2-devel] [PATCH v5 0/2] CryptoPkg/OpensslLib: Add native
> >> instruction support for X64
> >>
> >> So, if someone wants to use this accelerator in Linux with GCC, what he/she
> >> need to do?
> >>
> >>> -----Original Message-----
> >>> From: Zurcher, Christopher J <christopher.j.zurcher@intel.com>
> >>> Sent: Saturday, November 7, 2020 3:36 AM
> >>> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Laszlo
> >>> Ersek <lersek@redhat.com>; gaoliming <gaoliming@byosoft.com.cn>
> >>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> >>> <xiaoyux.lu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> >>> Ard Biesheuvel <ard.biesheuvel@arm.com>
> >>> Subject: RE: [edk2-devel] [PATCH v5 0/2] CryptoPkg/OpensslLib: Add native
> >>> instruction support for X64
> >>>
> >>> Yes, in response to your request, it was discussed on the mailing list that
> >> the
> >>> OpenSSL assembly code is not compatible with GCC in this format. It is
> >> fully
> >>> functional with VS toolchain and CLANGPDB toolchain. The GCC toolchain
> >>> appears to be unable to handle some aspects of NASM code, particularly the
> >>> COMMON section and the "wrt ..imagebase" style of position-independent
> >>> code.
> >>> See 7.6.1 here: https://www.nasm.us/xdoc/2.13.02rc3/html/nasmdoc7.html
> >>>
> >>> Mike/Laszlo/Liming,
> >>> Can you help me resolve the CI failures?
> >>>
> >>> Thanks,
> >>> Christopher Zurcher
> >>>
> >>>> -----Original Message-----
> >>>> From: Yao, Jiewen <jiewen.yao@intel.com>
> >>>> Sent: Friday, November 6, 2020 02:23
> >>>> To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>;
> >>>> devel@edk2.groups.io
> >>>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> >>> <xiaoyux.lu@intel.com>;
> >>>> Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel
> >>>> <ard.biesheuvel@arm.com>
> >>>> Subject: RE: [edk2-devel] [PATCH v5 0/2] CryptoPkg/OpensslLib: Add native
> >>>> instruction support for X64
> >>>>
> >>>> Hi Zurcher
> >>>> I am not CI person, so I recommend you have a discussion with Mike,
> >>> Laszlo,
> >>>> or Liming to see what is the best way to resolve the coding style issue.
> >> Or
> >>>> how to change CI rule to add exception somewhere.
> >>>>
> >>>> However, I do have concern, when you say: "the module is not compatible
> >>> with
> >>>> GCC builds."
> >>>>
> >>>> In previous review, I already gave the comment to pass build with GCC and
> >>>> CLANG besides MSVC. Do you mean this patch cannot pass GCC build?
> >>>>
> >>>> Thank you
> >>>> Yao Jiewen
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Zurcher, Christopher J <christopher.j.zurcher@intel.com>
> >>>>> Sent: Friday, November 6, 2020 5:50 PM
> >>>>> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> >>>>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> >>>>> <xiaoyux.lu@intel.com>; Kinney, Michael D
> >>> <michael.d.kinney@intel.com>;
> >>>>> Ard Biesheuvel <ard.biesheuvel@arm.com>
> >>>>> Subject: RE: [edk2-devel] [PATCH v5 0/2] CryptoPkg/OpensslLib: Add
> >>> native
> >>>>> instruction support for X64
> >>>>>
> >>>>> I think some of these errors are not relevant based on the nature of
> >> the
> >>>>> commit:
> >>>>>
> >>>>> One of the errors reported is that the added typedefs (ptrdiff_t,
> >> wchar_t)
> >>>> do
> >>>>> not follow coding guidelines, but the typedefs are required for OpenSSL
> >>>>> compatibility and they match the already-existing style:
> >>>>>
> >>>>> typedef UINTN          size_t;
> >>>>> typedef UINTN          u_int;
> >>>>> +typedef INTN           ptrdiff_t;
> >>>>> typedef INTN           ssize_t;
> >>>>> typedef INT32          time_t;
> >>>>> typedef UINT8          __uint8_t;
> >>>>> typedef UINT8          sa_family_t;
> >>>>> typedef UINT8          u_char;
> >>>>> typedef UINT32         uid_t;
> >>>>> typedef UINT32         gid_t;
> >>>>> +typedef CHAR16         wchar_t;
> >>>>>
> >>>>> Another error is that the auto-generated assembly files do not start
> >> with
> >>>>> capital letters, but these filenames come from OpenSSL with lowercase
> >>>>> filenames, and we already have opensslconf.h in the Include folder
> >> which
> >>>>> has a lowercase filename.
> >>>>>
> >>>>> Another error type reported is that the auto-generated assembly files
> >> do
> >>>> not
> >>>>> have "SPDX-License-Identifier: BSD-2-Clause-Patent" but it was already
> >>>>> discussed on the list that these would be checked in with the OpenSSL
> >>>> header
> >>>>> similar to opensslconf.h:
> >>>>>
> >>> https://github.com/tianocore/edk2/blob/master/CryptoPkg/Library/Include/
> >>>>> openssl/opensslconf.h
> >>>>>
> >>>>> Additionally, there is an error that OpensslLibX64.inf is not in
> >>>> CryptoPkg.dsc,
> >>>>> but I am not sure if it is appropriate to include as the module is not
> >>>>> compatible with GCC builds.
> >>>>>
> >>>>> How should I proceed here?
> >>>>>
> >>>>> Thanks,
> >>>>> Christopher Zurcher
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Yao, Jiewen <jiewen.yao@intel.com>
> >>>>>> Sent: Thursday, November 5, 2020 22:14
> >>>>>> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>;
> >>> Zurcher,
> >>>>>> Christopher J <christopher.j.zurcher@intel.com>
> >>>>>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> >>>>> <xiaoyux.lu@intel.com>;
> >>>>>> Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel
> >>>>>> <ard.biesheuvel@arm.com>
> >>>>>> Subject: RE: [edk2-devel] [PATCH v5 0/2] CryptoPkg/OpensslLib: Add
> >>> native
> >>>>>> instruction support for X64
> >>>>>>
> >>>>>> Hi Zurcher
> >>>>>> I created https://github.com/tianocore/edk2/pull/1092
> >>>>>>
> >>>>>> However, there are failures in CI test. So this patch is NOT merged.
> >>>>>>
> >>>>>> Please take a look and resolve it.
> >>>>>>
> >>>>>> Thank you
> >>>>>> Yao Jiewen
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> >>> Yao,
> >>>>>>> Jiewen
> >>>>>>> Sent: Friday, November 6, 2020 1:56 PM
> >>>>>>> To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>;
> >>>>>>> devel@edk2.groups.io
> >>>>>>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> >>>>>>> <xiaoyux.lu@intel.com>; Kinney, Michael D
> >>>>> <michael.d.kinney@intel.com>;
> >>>>>>> Ard Biesheuvel <ard.biesheuvel@arm.com>
> >>>>>>> Subject: Re: [edk2-devel] [PATCH v5 0/2] CryptoPkg/OpensslLib: Add
> >>>>> native
> >>>>>>> instruction support for X64
> >>>>>>>
> >>>>>>> Patch 1/2 reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Christopher J Zurcher <christopher.j.zurcher@intel.com>
> >>>>>>>> Sent: Wednesday, November 4, 2020 5:59 AM
> >>>>>>>> To: devel@edk2.groups.io
> >>>>>>>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> >>>>>>>> <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>;
> >>> Kinney,
> >>>>>>>> Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel
> >>>>>>>> <ard.biesheuvel@arm.com>
> >>>>>>>> Subject: [PATCH v5 0/2] CryptoPkg/OpensslLib: Add native
> >>> instruction
> >>>>>>>> support for X64
> >>>>>>>>
> >>>>>>>> V5 Changes:
> >>>>>>>>  Move ApiHooks.c into X64 folder
> >>>>>>>>  Update process_files.pl to clean architecture-specific
> >> subfolders
> >>>>>> without
> >>>>>>>>    removing them
> >>>>>>>>  Rebased INF file to merge latest changes regarding RngLib vs.
> >>>> TimerLib
> >>>>>>>>
> >>>>>>>> V4 Changes:
> >>>>>>>>  Add copyright header to uefi-asm.conf
> >>>>>>>>  Move [Sources.X64] block to cover entire X64-specific config
> >>>>>>>>
> >>>>>>>> V3 Changes:
> >>>>>>>>  Added definitions for ptrdiff_t and wchar_t to CrtLibSupport.h
> >> for
> >>>>>>>>    LLVM/Clang build support.
> >>>>>>>>  Added -UWIN32 to GCC Flags for LLVM/Clang build support.
> >>>>>>>>  Added missing AES GCM assembly file.
> >>>>>>>>
> >>>>>>>> V2 Changes:
> >>>>>>>>  Limit scope of assembly config to SHA and AES functions.
> >>>>>>>>  Removed IA32 native support (reduced config was causing build
> >>>> failure
> >>>>>>> and
> >>>>>>>>    can be added in a later patch).
> >>>>>>>>  Removed XMM instructions from assembly generation.
> >>>>>>>>  Added automatic copyright header porting for generated assembly
> >>>>> files.
> >>>>>>>>
> >>>>>>>> This patch adds support for building the native instruction
> >>>> algorithms
> >>>>>> for
> >>>>>>>> the X64 architecture in OpensslLib. The process_files.pl script
> >> was
> >>>>>> modified
> >>>>>>>> to parse the .asm file targets from the OpenSSL build config data
> >>>> struct,
> >>>>>> and
> >>>>>>>> generate the necessary assembly files for the EDK2 build
> >>> environment.
> >>>>>>>>
> >>>>>>>> For the X64 variant, OpenSSL includes calls to a Windows error
> >>>> handling
> >>>>>> API,
> >>>>>>>> and that function has been stubbed out in ApiHooks.c.
> >>>>>>>>
> >>>>>>>> For all variants, a constructor is added to call the required
> >> CPUID
> >>>>>> function
> >>>>>>>> within OpenSSL to facilitate processor capability checks in the
> >>>> native
> >>>>>>>> algorithms.
> >>>>>>>>
> >>>>>>>> Additional native architecture variants should be simple to add
> >> by
> >>>>>> following
> >>>>>>>> the changes made for this architecture.
> >>>>>>>>
> >>>>>>>> The OpenSSL assembly files are traditionally generated at build
> >> time
> >>>>>> using a
> >>>>>>>> perl script. To avoid that burden on EDK2 users, these end-result
> >>>>>> assembly
> >>>>>>>> files are generated during the configuration steps performed by
> >> the
> >>>>>>> package
> >>>>>>>> maintainer (through process_files.pl). The perl generator scripts
> >>>> inside
> >>>>>>>> OpenSSL do not parse file comments as they are only meant to
> >>> create
> >>>>>>>> intermediate build files, so process_files.pl contains additional
> >>>> hooks
> >>>>>> to
> >>>>>>>> preserve the copyright headers as well as clean up tabs and line
> >>>> endings
> >>>>>> to
> >>>>>>>> comply with EDK2 coding standards. The resulting file headers
> >> align
> >>>>> with
> >>>>>>>> the generated .h files which are already included in the EDK2
> >>>> repository.
> >>>>>>>>
> >>>>>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >>>>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
> >>>>>>>> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> >>>>>>>> Cc: Mike Kinney <michael.d.kinney@intel.com>
> >>>>>>>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> >>>>>>>>
> >>>>>>>> Christopher J Zurcher (2):
> >>>>>>>>  CryptoPkg/OpensslLib: Add native instruction support for X64
> >>>>>>>>  CryptoPkg/OpensslLib: Commit the auto-generated assembly files
> >>> for
> >>>>> X64
> >>>>>>>>
> >>>>>>>> CryptoPkg/Library/OpensslLib/OpensslLib.inf
> >>>> |
> >>>>>> 2 +-
> >>>>>>>> CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> >>>> |
> >>>>>> 2 +-
> >>>>>>>> CryptoPkg/Library/OpensslLib/OpensslLibX64.inf
> >>>> |
> >>>>>> 653 +++
> >>>>>>>> CryptoPkg/Library/Include/CrtLibSupport.h
> >>>> |
> >>>>>> 2 +
> >>>>>>>> CryptoPkg/Library/Include/openssl/opensslconf.h
> >>>> |
> >>>>>> 3 -
> >>>>>>>> CryptoPkg/Library/OpensslLib/OpensslLibConstructor.c
> >>>> |
> >>>>>> 34 +
> >>>>>>>> CryptoPkg/Library/OpensslLib/X64/ApiHooks.c
> >>>> |
> >>>>>> 18 +
> >>>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/aes/aesni-mb-
> >>> x86_64.nasm
> >>>>> |
> >>>>>>>> 732 +++
> >>>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/aes/aesni-sha1-
> >>>>> x86_64.nasm   |
> >>>>>>>> 1916 ++++++++
> >>>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/aes/aesni-sha256-
> >>>>> x86_64.nasm
> >>>>>>> |
> >>>>>>>> 78 +
> >>>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/aes/aesni-x86_64.nasm
> >>>>> |
> >>>>>>>> 5103 ++++++++++++++++++++
> >>>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/aes/vpaes-x86_64.nasm
> >>>>> |
> >>>>>>>> 1173 +++++
> >>>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/modes/aesni-gcm-
> >>>>> x86_64.nasm
> >>>>>>>> |   34 +
> >>>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/modes/ghash-
> >>> x86_64.nasm
> >>>>> |
> >>>>>>>> 1569 ++++++
> >>>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/sha/sha1-mb-
> >>> x86_64.nasm
> >>>>> |
> >>>>>>>> 3137 ++++++++++++
> >>>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/sha/sha1-x86_64.nasm
> >>>>> |
> >>>>>>>> 2884 +++++++++++
> >>>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/sha/sha256-mb-
> >>>>> x86_64.nasm
> >>>>>>> |
> >>>>>>>> 3461 +++++++++++++
> >>>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/sha/sha256-
> >>> x86_64.nasm
> >>>>> |
> >>>>>>>> 3313 +++++++++++++
> >>>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/sha/sha512-
> >>> x86_64.nasm
> >>>>> |
> >>>>>>>> 1938 ++++++++
> >>>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/x86_64cpuid.nasm
> >>>> |
> >>>>>> 491
> >>>>>>>> ++
> >>>>>>>> CryptoPkg/Library/OpensslLib/process_files.pl
> >>>> |
> >>>>>> 232 +-
> >>>>>>>> CryptoPkg/Library/OpensslLib/uefi-asm.conf
> >>>> |
> >>>>>> 21 +
> >>>>>>>> 22 files changed, 26746 insertions(+), 50 deletions(-)
> >>>>>>>> create mode 100644
> >>> CryptoPkg/Library/OpensslLib/OpensslLibX64.inf
> >>>>>>>> create mode 100644
> >>>>>>> CryptoPkg/Library/OpensslLib/OpensslLibConstructor.c
> >>>>>>>> create mode 100644 CryptoPkg/Library/OpensslLib/X64/ApiHooks.c
> >>>>>>>> create mode 100644
> >>>>> CryptoPkg/Library/OpensslLib/X64/crypto/aes/aesni-
> >>>>>>>> mb-x86_64.nasm
> >>>>>>>> create mode 100644
> >>>>> CryptoPkg/Library/OpensslLib/X64/crypto/aes/aesni-
> >>>>>>>> sha1-x86_64.nasm
> >>>>>>>> create mode 100644
> >>>>> CryptoPkg/Library/OpensslLib/X64/crypto/aes/aesni-
> >>>>>>>> sha256-x86_64.nasm
> >>>>>>>> create mode 100644
> >>>>> CryptoPkg/Library/OpensslLib/X64/crypto/aes/aesni-
> >>>>>>>> x86_64.nasm
> >>>>>>>> create mode 100644
> >>>>> CryptoPkg/Library/OpensslLib/X64/crypto/aes/vpaes-
> >>>>>>>> x86_64.nasm
> >>>>>>>> create mode 100644
> >>>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/modes/aesni-gcm-
> >>>>> x86_64.nasm
> >>>>>>>> create mode 100644
> >>>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/modes/ghash-
> >>> x86_64.nasm
> >>>>>>>> create mode 100644
> >>>>> CryptoPkg/Library/OpensslLib/X64/crypto/sha/sha1-
> >>>>>>>> mb-x86_64.nasm
> >>>>>>>> create mode 100644
> >>>>> CryptoPkg/Library/OpensslLib/X64/crypto/sha/sha1-
> >>>>>>>> x86_64.nasm
> >>>>>>>> create mode 100644
> >>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/sha/sha256-
> >>>>>>>> mb-x86_64.nasm
> >>>>>>>> create mode 100644
> >>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/sha/sha256-
> >>>>>>>> x86_64.nasm
> >>>>>>>> create mode 100644
> >>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/sha/sha512-
> >>>>>>>> x86_64.nasm
> >>>>>>>> create mode 100644
> >>>>>>>> CryptoPkg/Library/OpensslLib/X64/crypto/x86_64cpuid.nasm
> >>>>>>>> create mode 100644 CryptoPkg/Library/OpensslLib/uefi-asm.conf
> >>>>>>>>
> >>>>>>>> --
> >>>>>>>> 2.28.0.windows.1
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> 
> >>>>>>>
> >