回复: [edk2-devel] [PATCH v5 0/2] CryptoPkg/OpensslLib: Add native instruction support for X64
gaoliming
gaoliming at byosoft.com.cn
Mon Nov 9 07:03:57 UTC 2020
Zurcher:
I reply the mail to mention this is the expected behavior in nasm compiler.
For this patch, I suggest to generate GAS assembly for GCC tool chain.
For ECC and License checker issues, you can ignore them in
CryptoPkg.ci.yaml if they are not real issue.
Thanks
Liming
> -----邮件原件-----
> 发件人: bounce+27952+67117+4905953+8761045 at groups.io
> <bounce+27952+67117+4905953+8761045 at groups.io> 代表 Zurcher,
> Christopher J
> 发送时间: 2020年11月7日 10:03
> 收件人: Yao, Jiewen <jiewen.yao at intel.com>; devel at edk2.groups.io; Laszlo
> Ersek <lersek at redhat.com>; gaoliming <gaoliming at byosoft.com.cn>
> 抄送: Wang, Jian J <jian.j.wang at intel.com>; Lu, XiaoyuX
> <xiaoyux.lu at intel.com>; Kinney, Michael D <michael.d.kinney at intel.com>;
> Ard Biesheuvel <ard.biesheuvel at arm.com>
> 主题: Re: [edk2-devel] [PATCH v5 0/2] CryptoPkg/OpensslLib: Add native
> instruction support for X64
>
> 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 at intel.com>
> > Sent: Friday, November 6, 2020 15:07
> > To: Zurcher, Christopher J <christopher.j.zurcher at intel.com>;
> > devel at edk2.groups.io; Laszlo Ersek <lersek at redhat.com>; gaoliming
> > <gaoliming at byosoft.com.cn>
> > Cc: Wang, Jian J <jian.j.wang at intel.com>; Lu, XiaoyuX
> <xiaoyux.lu at intel.com>;
> > Kinney, Michael D <michael.d.kinney at intel.com>; Ard Biesheuvel
> > <ard.biesheuvel at 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 at intel.com>
> > > Sent: Saturday, November 7, 2020 3:36 AM
> > > To: Yao, Jiewen <jiewen.yao at intel.com>; devel at edk2.groups.io; Laszlo
> > > Ersek <lersek at redhat.com>; gaoliming <gaoliming at byosoft.com.cn>
> > > Cc: Wang, Jian J <jian.j.wang at intel.com>; Lu, XiaoyuX
> > > <xiaoyux.lu at intel.com>; Kinney, Michael D
> <michael.d.kinney at intel.com>;
> > > Ard Biesheuvel <ard.biesheuvel at 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 at intel.com>
> > > > Sent: Friday, November 6, 2020 02:23
> > > > To: Zurcher, Christopher J <christopher.j.zurcher at intel.com>;
> > > > devel at edk2.groups.io
> > > > Cc: Wang, Jian J <jian.j.wang at intel.com>; Lu, XiaoyuX
> > > <xiaoyux.lu at intel.com>;
> > > > Kinney, Michael D <michael.d.kinney at intel.com>; Ard Biesheuvel
> > > > <ard.biesheuvel at 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 at intel.com>
> > > > > Sent: Friday, November 6, 2020 5:50 PM
> > > > > To: Yao, Jiewen <jiewen.yao at intel.com>; devel at edk2.groups.io
> > > > > Cc: Wang, Jian J <jian.j.wang at intel.com>; Lu, XiaoyuX
> > > > > <xiaoyux.lu at intel.com>; Kinney, Michael D
> > > <michael.d.kinney at intel.com>;
> > > > > Ard Biesheuvel <ard.biesheuvel at 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 at intel.com>
> > > > > > Sent: Thursday, November 5, 2020 22:14
> > > > > > To: devel at edk2.groups.io; Yao, Jiewen <jiewen.yao at intel.com>;
> > > Zurcher,
> > > > > > Christopher J <christopher.j.zurcher at intel.com>
> > > > > > Cc: Wang, Jian J <jian.j.wang at intel.com>; Lu, XiaoyuX
> > > > > <xiaoyux.lu at intel.com>;
> > > > > > Kinney, Michael D <michael.d.kinney at intel.com>; Ard Biesheuvel
> > > > > > <ard.biesheuvel at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf
> Of
> > > Yao,
> > > > > > > Jiewen
> > > > > > > Sent: Friday, November 6, 2020 1:56 PM
> > > > > > > To: Zurcher, Christopher J <christopher.j.zurcher at intel.com>;
> > > > > > > devel at edk2.groups.io
> > > > > > > Cc: Wang, Jian J <jian.j.wang at intel.com>; Lu, XiaoyuX
> > > > > > > <xiaoyux.lu at intel.com>; Kinney, Michael D
> > > > > <michael.d.kinney at intel.com>;
> > > > > > > Ard Biesheuvel <ard.biesheuvel at 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 at intel.com>
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Christopher J Zurcher
<christopher.j.zurcher at intel.com>
> > > > > > > > Sent: Wednesday, November 4, 2020 5:59 AM
> > > > > > > > To: devel at edk2.groups.io
> > > > > > > > Cc: Yao, Jiewen <jiewen.yao at intel.com>; Wang, Jian J
> > > > > > > > <jian.j.wang at intel.com>; Lu, XiaoyuX <xiaoyux.lu at intel.com>;
> > > Kinney,
> > > > > > > > Michael D <michael.d.kinney at intel.com>; Ard Biesheuvel
> > > > > > > > <ard.biesheuvel at 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 at intel.com>
> > > > > > > > Cc: Jian J Wang <jian.j.wang at intel.com>
> > > > > > > > Cc: Xiaoyu Lu <xiaoyux.lu at intel.com>
> > > > > > > > Cc: Mike Kinney <michael.d.kinney at intel.com>
> > > > > > > > Cc: Ard Biesheuvel <ard.biesheuvel at 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
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67161): https://edk2.groups.io/g/devel/message/67161
Mute This Topic: https://groups.io/mt/78130889/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list