Re: [edk2-devel] [PATCH 1/1] CryptoPkg/OpensslLib: Add native instruction support for IA32 and X64

["Zurcher, Christopher J" <christopher.j.zurcher@intel.com>]

During the Perlasm generation step, the script attempts to execute NASM from the user's path to see if it's an appropriate version to support AVX instructions:
https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/crypto/sha/asm/sha512-x86_64.pl#L129
For now, we could force this check to fail during generation. I'm going to reduce this patch to only cover AES and SHA, and the OS-provisioning use case for the SHA acceleration has enough dependency on I/O operations that the AVX speed increase is already reaching diminishing returns. I don't mind leaving AVX support to the future.

--
Christopher Zurcher

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Thursday, March 26, 2020 11:23
> To: Yao, Jiewen <jiewen.yao@intel.com>; Zurcher, Christopher J
> <christopher.j.zurcher@intel.com>; devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>;
> Ard Biesheuvel <ard.biesheuvel@linaro.org>; david.harris4@hp.com
> Subject: RE: [edk2-devel] [PATCH 1/1] CryptoPkg/OpensslLib: Add native
> instruction support for IA32 and X64
> 
> The main issue with use of extended registers is
> that the default interrupt/exception handler for
> IA32/X64 does FXSAVE/FXRSTOR.
> 
> https://github.com/tianocore/edk2/blob/5f7c91f0d72efca3b53628163861265c89306f
> 1f/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm#L24
> 4
> 
> https://github.com/tianocore/edk2/blob/5f7c91f0d72efca3b53628163861265c89306f
> 1f/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nasm#L2
> 90
> 
> This covers FP/MMX/XMM registers, but no more.
> 
> Any code that uses more than this has 2 options:
> 1) Expand the state saved in interrupts/exception handlers
>    for the entire platform.
> 2) Disable interrupts when calling code that uses
>    additional register state.
> 
> Mike
> 
> > -----Original Message-----
> > From: Yao, Jiewen <jiewen.yao@intel.com>
> > Sent: Wednesday, March 25, 2020 8:58 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>; Ard Biesheuvel
> > <ard.biesheuvel@linaro.org>; david.harris4@hp.com;
> > Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: RE: [edk2-devel] [PATCH 1/1]
> > CryptoPkg/OpensslLib: Add native instruction support for
> > IA32 and X64
> >
> > There is an old shell based test application. It is
> > removed from edk2-repo, and it should be in edk2-test
> > repo in the future.
> >
> > Before the latter is ready, you can find the archive at
> > https://github.com/jyao1/edk2/tree/edk2-
> > cryptest/CryptoTestPkg/Cryptest
> >
> > But I am not sure if it covers the latest interface,
> > especially the new added one. As far as I know, some SM3
> > is not added there.
> >
> > I still recommend you double check which functions use
> > the YMM and make sure these functions are tested.
> >
> > I have no concern on HASH, since you already provided the
> > data.
> > But other algo, such as AES/RSA/HMAC, etc.
> >
> >
> > Thank you
> > Yao Jiewen
> >
> > > -----Original Message-----
> > > From: Zurcher, Christopher J
> > <christopher.j.zurcher@intel.com>
> > > Sent: Thursday, March 26, 2020 11:29 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@linaro.org>;
> > david.harris4@hp.com; Kinney,
> > > Michael D <michael.d.kinney@intel.com>
> > > Subject: RE: [edk2-devel] [PATCH 1/1]
> > CryptoPkg/OpensslLib: Add native
> > > instruction support for IA32 and X64
> > >
> > > > -----Original Message-----
> > > > From: Yao, Jiewen <jiewen.yao@intel.com>
> > > > Sent: Wednesday, March 25, 2020 20:05
> > > > 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@linaro.org>;
> > david.harris4@hp.com; Kinney,
> > > > Michael D <michael.d.kinney@intel.com>
> > > > Subject: RE: [edk2-devel] [PATCH 1/1]
> > CryptoPkg/OpensslLib: Add native
> > > > instruction support for IA32 and X64
> > > >
> > > > Thanks. Comment inline:
> > > >
> > > > > -----Original Message-----
> > > > > From: Zurcher, Christopher J
> > <christopher.j.zurcher@intel.com>
> > > > > Sent: Thursday, March 26, 2020 10:44 AM
> > > > > To: devel@edk2.groups.io; Yao, Jiewen
> > <jiewen.yao@intel.com>
> > > > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu,
> > XiaoyuX
> > > > <xiaoyux.lu@intel.com>;
> > > > > Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> > david.harris4@hp.com; Kinney,
> > > > > Michael D <michael.d.kinney@intel.com>
> > > > > Subject: RE: [edk2-devel] [PATCH 1/1]
> > CryptoPkg/OpensslLib: Add native
> > > > > instruction support for IA32 and X64
> > > > >
> > > > > The specific performance improvement depends on the
> > operation; the OS
> > > > > provisioning I mentioned in the [Patch 0/1] thread
> > removed the hashing as a
> > > > > bottleneck and improved the overall operation speed
> > over 4x (saving 2.5
> > > > > minutes of flashing time), but a direct SHA256
> > benchmark on the particular
> > > > > silicon I have available showed over 12x
> > improvement. I have not
> > > > benchmarked
> > > > > the improvements to boot time. I do not know the
> > use case targeted by BZ
> > > > 2507
> > > > > so I don’t know what benefit will be seen there.
> > > > [Jiewen] I guess there might be some improvement on
> > HTTPS boot because of
> > > > AES-NI for TLS session.
> > > > I am just curious on the data. :-)
> > > >
> > > >
> > > > >
> > > > > I will look at unifying the INF files in the next
> > patch-set and will also
> > > > add the
> > > > > OpensslLibCrypto.inf case.
> > > > [Jiewen] Thanks!
> > > >
> > > >
> > > > >
> > > > > I have not exercised the AVX code specifically, as
> > it is coming directly
> > > > from
> > > > > OpenSSL and includes checks against the CPUID
> > capability flags before
> > > > executing.
> > > > > I'm not entirely familiar with AVX requirements; is
> > there a known
> > > > environment
> > > > > restriction against AVX instructions in EDK2?
> > > > [Jiewen] Yes. UEFI spec only requires to set env for
> > XMM register.
> > > > Using other registers such as YMM or ZMM may requires
> > special setup, and
> > > > save/restore some FPU state.
> > > > If a function containing the YMM register access and
> > it is linked into
> > > > BaseCryptoLib, I highly recommend you run some test
> > to make sure it can
> > > work
> > > > correct.
> > > >
> > > > Maybe I should ask more generic question: Have you
> > validated all impacted
> > > > crypto lib API to make sure they still work well with
> > this improvement?
> > > >
> > >
> > > I have not. Is there an existing test suite that was
> > used initially, or used with
> > > each OpenSSL version update?
> > >
> > > Thanks,
> > > Christopher Zurcher
> > >
> > > >
> > > > >
> > > > > Regarding RNG, it looks like we already have
> > architecture-specific variants
> > > > of
> > > > > RdRand...?
> > > > [Jiewen] Yes. That is in RngLib.
> > > > I ask this question because I see openssl wrapper is
> > using PMC/TSC as noisy.
> > > >
> > >
> > https://github.com/tianocore/edk2/blob/master/CryptoPkg/L
> > ibrary/OpensslLib/
> > > ra
> > > > nd_pool.c
> > > > Since this patch adds instruction dependency, why no
> > use RNG instruction as
> > > > well?
> > > >
> > > >
> > > > >
> > > > > There was some off-list feedback regarding the
> > number of files required to
> > > > be
> > > > > checked in here. OpenSSL does not include
> > assembler-specific
> > > > implementations
> > > > > of these files and instead relies on "perlasm"
> > scripts which are parsed by
> > > > a
> > > > > translation script at build time (in the normal
> > OpenSSL build flow) to
> > > > generate
> > > > > the resulting .nasm files. The implementation I
> > have shared here generates
> > > > these
> > > > > files as part of the OpensslLib maintainer process,
> > similar to the existing
> > > > header
> > > > > files which are also generated. Since
> > process_files.pl already requires the
> > > > > package maintainer to have a Perl environment
> > installed, this does not
> > > > place any
> > > > > additional burden on them.
> > > > > An alternative implementation has been proposed
> > which would see only a
> > > > > listing/script of the required generator operations
> > to be checked in, and
> > > > any
> > > > > platform build which intended to utilize the native
> > algorithms would
> > > > require a
> > > > > local Perl environment as well as any underlying
> > environment dependencies
> > > > > (such as a version check against the NASM
> > executable) for every developer,
> > > > and
> > > > > additional pre-build steps to run the generator
> > scripts.
> > > > >
> > > > > Are there any strong opinions here around adding
> > Perl as a build
> > > > environment
> > > > > dependency vs. checking in maintainer-generated
> > assembly "intermediate"
> > > > build
> > > > > files?
> > > > [Jiewen] Good question. For tool, maybe Mike or
> > Liming can answer.
> > > > And I did get similar issue with you.
> > > > I got a submodule code need using CMake and pre-
> > processor to generate a
> > > > common include file.
> > > > How do we handle that? I look for the recommendation
> > as well.
> > > >
> > > >
> > > > >
> > > > > Thanks,
> > > > > Christopher Zurcher
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io>
> > On Behalf Of Yao,
> > > > > Jiewen
> > > > > > Sent: Wednesday, March 25, 2020 18:23
> > > > > > 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>;
> > > > > > Eugene Cohen <eugene@hp.com>; Ard Biesheuvel
> > > <ard.biesheuvel@linaro.org>
> > > > > > Subject: Re: [edk2-devel] [PATCH 1/1]
> > CryptoPkg/OpensslLib: Add native
> > > > > > instruction support for IA32 and X64
> > > > > >
> > > > > > Some more comment:
> > > > > >
> > > > > > 3) Do you consider to enable RNG instruction as
> > well?
> > > > > >
> > > > > > 4) I saw you added some code for AVX instruction,
> > such as YMM register.
> > > > > > Have you validated that code, to make sure it can
> > work correctly in
> > > > current
> > > > > > environment?
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: devel@edk2.groups.io
> > <devel@edk2.groups.io> On Behalf Of Yao,
> > > > > Jiewen
> > > > > > > Sent: Thursday, March 26, 2020 9:15 AM
> > > > > > > To: devel@edk2.groups.io; Zurcher, Christopher
> > J
> > > > > > > <christopher.j.zurcher@intel.com>
> > > > > > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu,
> > XiaoyuX
> > > > > > <xiaoyux.lu@intel.com>;
> > > > > > > Eugene Cohen <eugene@hp.com>; Ard Biesheuvel
> > > > > <ard.biesheuvel@linaro.org>
> > > > > > > Subject: Re: [edk2-devel] [PATCH 1/1]
> > CryptoPkg/OpensslLib: Add native
> > > > > > > instruction support for IA32 and X64
> > > > > > >
> > > > > > > HI Christopher
> > > > > > > Thanks for the contribution. I think it is good
> > enhancement.
> > > > > > >
> > > > > > > Do you have any data show what performance
> > improvement we can get?
> > > > > > > Did the system boot faster with the this? Which
> > feature ?
> > > > > > > UEFI Secure Boot? TCG Measured Boot? HTTPS
> > boot?
> > > > > > >
> > > > > > >
> > > > > > > Comment for the code:
> > > > > > > 1) I am not sure if we need separate
> > OpensslLibIa32 and OpensslLibX64.
> > > > > > > Can we just define single INF, such as
> > OpensslLibHw.inf ?
> > > > > > >
> > > > > > > 2) Do we also need add a new version for
> > OpensslLibCrypto.inf ?
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Thank you
> > > > > > > Yao Jiewen
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: devel@edk2.groups.io
> > <devel@edk2.groups.io> On Behalf Of
> > > > > Zurcher,
> > > > > > > > Christopher J
> > > > > > > > Sent: Tuesday, March 17, 2020 6:27 PM
> > > > > > > > To: devel@edk2.groups.io
> > > > > > > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu,
> > XiaoyuX
> > > > > > > <xiaoyux.lu@intel.com>;
> > > > > > > > Eugene Cohen <eugene@hp.com>; Ard Biesheuvel
> > > > > <ard.biesheuvel@linaro.org>
> > > > > > > > Subject: [edk2-devel] [PATCH 1/1]
> > CryptoPkg/OpensslLib: Add native
> > > > > > > instruction
> > > > > > > > support for IA32 and X64
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > > 
> > > > >
> > > >
> > >
>