Re: [PATCH v3 0/2] CryptoPkg/OpensslLib: Add native instruction support for X64

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

> -----Original Message-----
> From: Zurcher, Christopher J <christopher.j.zurcher@intel.com>
> Sent: Wednesday, October 21, 2020 5:10 AM
> 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>; Ard Biesheuvel <ard.biesheuvel@arm.com>
> Subject: RE: [PATCH v3 0/2] CryptoPkg/OpensslLib: Add native instruction
> support for X64
> 
> Comments inline.
> 
> > -----Original Message-----
> > From: Yao, Jiewen <jiewen.yao@intel.com>
> > Sent: Saturday, October 17, 2020 18:26
> > 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>;
> > Ard Biesheuvel <ard.biesheuvel@arm.com>
> > Subject: RE: [PATCH v3 0/2] CryptoPkg/OpensslLib: Add native instruction
> > support for X64
> >
> > Thanks Zurcher.
> >
> > Comment:
> > 1) I do not see the copy right header and license header for uefi-asm.conf.
> > Please add it.
> >
> > 2) Is "ApiHooks.c" only for Visual Studio? Do we need it for GCC and LLVM?
> > If it is only for visual studio, I suggest we rename to MsftApiHooks.c, and
> > add "| MSFT" in INF.
> 
> ApiHooks.c is for any compiler building the assembly files, not only Visual
> Studio.
[Jiewen] Got it. Thanks for the clarification.
It is only needed for X64 assembly. Right?
If so, I recommend to put it to [Source.X64]

> 
> >
> > 3) Since nasm is only for X64¸ please use [Source.X64] for them.
> 
> Since the .c file set can change depending on which assembly target is
> selected (X64 vs. IA32 etc.) and since this .inf file is only for X64 targets, I will
> move all sources under the [Sources.X64] header. Is that OK? Leaving the
> current [Sources] adding a second autogen section under [Sources.X64] for
> only the .nasm files would add another layer of complexity to
> process_files.pl.
[Jiewen] I think it is OK. You can put all X64 released C file under [Sources.X64].

> 
> >
> > Please refer to
> >
> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseLib/Ba
> seLib.
> > inf, on 2) and 3).
> >
> > I want to get ACK from Mike Kinney, to see if it is right way to manually
> add
> > nasm and ApiHooks.c. Or if we need some extra documentation for that.
> > Please do add him as reviewer in next version patch.
> 
> I have added Mike Kinney for the next patch set but I have already had
> multiple discussions with him on this topic and the current implementation
> was agreed upon.
[Jiewen] Sounds good. Then, please add his Acked-by.


> 
> Thanks,
> Christopher Zurcher
> 
> >
> > I treat the ApiHooks.c is a work-around and it should be removed in the
> > future, once openssl resolve it. Maybe we need a Bugzilla to track this.
> >
> >
> > Thank you
> > Yao Jiewen
> >
> >
> > > -----Original Message-----
> > > From: Christopher J Zurcher <christopher.j.zurcher@intel.com>
> > > Sent: Thursday, October 15, 2020 4:49 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>; Ard
> > > Biesheuvel <ard.biesheuvel@arm.com>
> > > Subject: [PATCH v3 0/2] CryptoPkg/OpensslLib: Add native instruction
> > > support for X64
> > >
> > > 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: 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                       |
> > 657 +++
> > >  CryptoPkg/Library/Include/CrtLibSupport.h                            |
> > 2 +
> > >  CryptoPkg/Library/Include/openssl/opensslconf.h                      |
> > 3 -
> > >  CryptoPkg/Library/OpensslLib/ApiHooks.c                              |
> > 18 +
> > >  CryptoPkg/Library/OpensslLib/OpensslLibConstructor.c                 |
> > 34 +
> > >  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                        |
> > 223 +-
> > >  CryptoPkg/Library/OpensslLib/uefi-asm.conf                           |
> > 15 +
> > >  22 files changed, 26735 insertions(+), 50 deletions(-)
> > >  create mode 100644 CryptoPkg/Library/OpensslLib/OpensslLibX64.inf
> > >  create mode 100644 CryptoPkg/Library/OpensslLib/ApiHooks.c
> > >  create mode 100644
> CryptoPkg/Library/OpensslLib/OpensslLibConstructor.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