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.kin...@intel.com> > Sent: Thursday, March 26, 2020 11:23 > To: Yao, Jiewen <jiewen....@intel.com>; Zurcher, Christopher J > <christopher.j.zurc...@intel.com>; devel@edk2.groups.io; Kinney, Michael D > <michael.d.kin...@intel.com> > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX <xiaoyux...@intel.com>; > Ard Biesheuvel <ard.biesheu...@linaro.org>; david.harr...@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....@intel.com> > > Sent: Wednesday, March 25, 2020 8:58 PM > > To: Zurcher, Christopher J > > <christopher.j.zurc...@intel.com>; devel@edk2.groups.io > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX > > <xiaoyux...@intel.com>; Ard Biesheuvel > > <ard.biesheu...@linaro.org>; david.harr...@hp.com; > > Kinney, Michael D <michael.d.kin...@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.zurc...@intel.com> > > > Sent: Thursday, March 26, 2020 11:29 AM > > > To: Yao, Jiewen <jiewen....@intel.com>; > > devel@edk2.groups.io > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX > > <xiaoyux...@intel.com>; > > > Ard Biesheuvel <ard.biesheu...@linaro.org>; > > david.harr...@hp.com; Kinney, > > > Michael D <michael.d.kin...@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....@intel.com> > > > > Sent: Wednesday, March 25, 2020 20:05 > > > > To: Zurcher, Christopher J > > <christopher.j.zurc...@intel.com>; > > > > devel@edk2.groups.io > > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX > > > <xiaoyux...@intel.com>; > > > > Ard Biesheuvel <ard.biesheu...@linaro.org>; > > david.harr...@hp.com; Kinney, > > > > Michael D <michael.d.kin...@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.zurc...@intel.com> > > > > > Sent: Thursday, March 26, 2020 10:44 AM > > > > > To: devel@edk2.groups.io; Yao, Jiewen > > <jiewen....@intel.com> > > > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, > > XiaoyuX > > > > <xiaoyux...@intel.com>; > > > > > Ard Biesheuvel <ard.biesheu...@linaro.org>; > > david.harr...@hp.com; Kinney, > > > > > Michael D <michael.d.kin...@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....@intel.com>; Zurcher, > > > > > > Christopher J <christopher.j.zurc...@intel.com> > > > > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, > > XiaoyuX > > > > > <xiaoyux...@intel.com>; > > > > > > Eugene Cohen <eug...@hp.com>; Ard Biesheuvel > > > <ard.biesheu...@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.zurc...@intel.com> > > > > > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, > > XiaoyuX > > > > > > <xiaoyux...@intel.com>; > > > > > > > Eugene Cohen <eug...@hp.com>; Ard Biesheuvel > > > > > <ard.biesheu...@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.w...@intel.com>; Lu, > > XiaoyuX > > > > > > > <xiaoyux...@intel.com>; > > > > > > > > Eugene Cohen <eug...@hp.com>; Ard Biesheuvel > > > > > <ard.biesheu...@linaro.org> > > > > > > > > Subject: [edk2-devel] [PATCH 1/1] > > CryptoPkg/OpensslLib: Add native > > > > > > > instruction > > > > > > > > support for IA32 and X64 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56433): https://edk2.groups.io/g/devel/message/56433 Mute This Topic: https://groups.io/mt/72021063/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
