From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mx.groups.io with SMTP id smtpd.web10.29975.1585191930192641328 for ; Wed, 25 Mar 2020 20:05:30 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.43, mailfrom: jiewen.yao@intel.com) IronPort-SDR: DIi15Tn3YJsx21PncScqvR8Ra/31j5TIx4e+NDJ7ygo1B3q/mQ18RbLpZiIBN/muH1JXS6Id2/ xemGMPjY126Q== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Mar 2020 20:05:29 -0700 IronPort-SDR: SrGe42rumHUkTHczxLkdybdJoQmNZCumMy/a7RVi77hIFttm6yWovrJvuxDnmXNkj/CCRmII5+ ZWZ1NcD3a7Mw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,306,1580803200"; d="scan'208";a="448479054" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga006.fm.intel.com with ESMTP; 25 Mar 2020 20:05:29 -0700 Received: from fmsmsx605.amr.corp.intel.com (10.18.126.85) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 25 Mar 2020 20:05:28 -0700 Received: from fmsmsx605.amr.corp.intel.com (10.18.126.85) by fmsmsx605.amr.corp.intel.com (10.18.126.85) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Wed, 25 Mar 2020 20:05:28 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx605.amr.corp.intel.com (10.18.126.85) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Wed, 25 Mar 2020 20:05:28 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.50]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.206]) with mapi id 14.03.0439.000; Thu, 26 Mar 2020 11:05:25 +0800 From: "Yao, Jiewen" To: "Zurcher, Christopher J" , "devel@edk2.groups.io" CC: "Wang, Jian J" , "Lu, XiaoyuX" , Ard Biesheuvel , "david.harris4@hp.com" , "Kinney, Michael D" Subject: Re: [edk2-devel] [PATCH 1/1] CryptoPkg/OpensslLib: Add native instruction support for IA32 and X64 Thread-Topic: [edk2-devel] [PATCH 1/1] CryptoPkg/OpensslLib: Add native instruction support for IA32 and X64 Thread-Index: AQHV/Ebi1gaDbPZSX0eeh8s0KfCdQqhaHaDggAAChCD//5ISgIAAh3DA Date: Thu, 26 Mar 2020 03:05:24 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503F99C86E@shsmsx102.ccr.corp.intel.com> References: <20200317102656.20032-1-christopher.j.zurcher@intel.com> <20200317102656.20032-2-christopher.j.zurcher@intel.com> <15FFB5A5A94CCE31.23217@groups.io> <74D8A39837DF1E4DA445A8C0B3885C503F99C64F@shsmsx102.ccr.corp.intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: jiewen.yao@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Thanks. Comment inline: > -----Original Message----- > From: Zurcher, Christopher J > Sent: Thursday, March 26, 2020 10:44 AM > To: devel@edk2.groups.io; Yao, Jiewen > Cc: Wang, Jian J ; Lu, XiaoyuX ; > Ard Biesheuvel ; david.harris4@hp.com; Kinney= , > Michael D > Subject: RE: [edk2-devel] [PATCH 1/1] CryptoPkg/OpensslLib: Add native > instruction support for IA32 and X64 >=20 > The specific performance improvement depends on the operation; the OS > provisioning I mentioned in the [Patch 0/1] thread removed the hashing a= s a > bottleneck and improved the overall operation speed over 4x (saving 2.5 > minutes of flashing time), but a direct SHA256 benchmark on the particul= ar > silicon I have available showed over 12x improvement. I have not benchma= rked > 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. :-) >=20 > I will look at unifying the INF files in the next patch-set and will als= o add the > OpensslLibCrypto.inf case. [Jiewen] Thanks! >=20 > I have not exercised the AVX code specifically, as it is coming directly= from > OpenSSL and includes checks against the CPUID capability flags before ex= ecuting. > I'm not entirely familiar with AVX requirements; is there a known enviro= nment > 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 s= ave/restore some FPU state. If a function containing the YMM register access and it is linked into Bas= eCryptoLib, I highly recommend you run some test to make sure it can work c= orrect. 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? >=20 > Regarding RNG, it looks like we already have architecture-specific varia= nts of > RdRand...? [Jiewen] Yes. That is in RngLib.=20 I ask this question because I see openssl wrapper is using PMC/TSC as nois= y.=20 https://github.com/tianocore/edk2/blob/master/CryptoPkg/Library/OpensslLib= /rand_pool.c Since this patch adds instruction dependency, why no use RNG instruction a= s well? >=20 > There was some off-list feedback regarding the number of files required = to be > checked in here. OpenSSL does not include assembler-specific implementat= ions > 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 g= enerate > the resulting .nasm files. The implementation I have shared here generat= es these > files as part of the OpensslLib maintainer process, similar to the exist= ing header > files which are also generated. Since process_files.pl already requires = the > package maintainer to have a Perl environment installed, this does not p= lace 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, an= d any > platform build which intended to utilize the native algorithms would req= uire a > local Perl environment as well as any underlying environment dependencie= s > (such as a version check against the NASM executable) for every develope= r, and > additional pre-build steps to run the generator scripts. >=20 > Are there any strong opinions here around adding Perl as a build environ= ment > 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 co= mmon include file. How do we handle that? I look for the recommendation as well.=20 >=20 > Thanks, > Christopher Zurcher >=20 > > -----Original Message----- > > From: devel@edk2.groups.io On Behalf Of Yao, > Jiewen > > Sent: Wednesday, March 25, 2020 18:23 > > To: devel@edk2.groups.io; Yao, Jiewen ; Zurcher, > > Christopher J > > Cc: Wang, Jian J ; Lu, XiaoyuX > ; > > Eugene Cohen ; Ard Biesheuvel > > 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 cu= rrent > > environment? > > > > > > > > > > > -----Original Message----- > > > From: 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 > > > > > > Cc: Wang, Jian J ; Lu, XiaoyuX > > ; > > > Eugene Cohen ; Ard Biesheuvel > > > > Subject: Re: [edk2-devel] [PATCH 1/1] CryptoPkg/OpensslLib: Add nati= ve > > > 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 OpensslLibX6= 4. > > > 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 On Behalf Of > Zurcher, > > > > Christopher J > > > > Sent: Tuesday, March 17, 2020 6:27 PM > > > > To: devel@edk2.groups.io > > > > Cc: Wang, Jian J ; Lu, XiaoyuX > > > ; > > > > Eugene Cohen ; Ard Biesheuvel > > > > > Subject: [edk2-devel] [PATCH 1/1] CryptoPkg/OpensslLib: Add native > > > instruction > > > > support for IA32 and X64 > > > > > > > > > > > > > > > > > > >=20 >=20