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? > > 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/Library/OpensslLib/rand_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 (#56355): https://edk2.groups.io/g/devel/message/56355 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] -=-=-=-=-=-=-=-=-=-=-=-
