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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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
> > >
> > >
> > >
> > > 
> > >