From: "Zurcher, Christopher J" <christopher.j.zurcher@intel.com>
To: "Yao, Jiewen" <jiewen.yao@intel.com>,
"devel@edk2.groups.io" <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" <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
Date: Thu, 26 Mar 2020 03:29:22 +0000 [thread overview]
Message-ID: <MWHPR1101MB2125F3DA40C63B25C56B98EBB3CF0@MWHPR1101MB2125.namprd11.prod.outlook.com> (raw)
In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503F99C86E@shsmsx102.ccr.corp.intel.com>
> -----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/Library/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
> > > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> >
>
next prev parent reply other threads:[~2020-03-26 3:29 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-17 10:26 [PATCH 0/1] CryptoPkg/OpensslLib: Add native instruction support for IA32 and X64 Zurcher, Christopher J
2020-03-17 10:26 ` [PATCH 1/1] " Zurcher, Christopher J
2020-03-26 1:15 ` [edk2-devel] " Yao, Jiewen
[not found] ` <15FFB5A5A94CCE31.23217@groups.io>
2020-03-26 1:23 ` Yao, Jiewen
2020-03-26 2:44 ` Zurcher, Christopher J
2020-03-26 3:05 ` Yao, Jiewen
2020-03-26 3:29 ` Zurcher, Christopher J [this message]
2020-03-26 3:58 ` Yao, Jiewen
2020-03-26 18:23 ` Michael D Kinney
2020-03-27 0:52 ` Zurcher, Christopher J
2020-03-23 12:59 ` [edk2-devel] [PATCH 0/1] " Laszlo Ersek
2020-03-25 18:40 ` Ard Biesheuvel
2020-03-26 1:04 ` [edk2-devel] " Zurcher, Christopher J
2020-03-26 7:49 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MWHPR1101MB2125F3DA40C63B25C56B98EBB3CF0@MWHPR1101MB2125.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox