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


Zurcher, Christopher J
 

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



Join devel@edk2.groups.io to automatically receive all group messages.