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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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/5f7c91f0d72efca3b53628163861265c89306f1f/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm#L244

https://github.com/tianocore/edk2/blob/5f7c91f0d72efca3b53628163861265c89306f1f/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nasm#L290

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