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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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?


> 
> 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.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
> > > >
> > >
> > >
> > >
> >
> >
> > 
>