From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by mx.groups.io with SMTP id smtpd.web11.30741.1585195097451960663 for ; Wed, 25 Mar 2020 20:58:17 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.120, mailfrom: jiewen.yao@intel.com) IronPort-SDR: SJYJ0j9+3MpuuYeQKuHYS7YNR7tXoMSnNoI6PXt0oyAjnB6NoNXdr19jRUf3/zTrdkzhpLdpro J+aacqutp9SA== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Mar 2020 20:58:17 -0700 IronPort-SDR: lI6pno29uCGS1T4NsWM1I1PSG2XWeUvlxm2ZP8/hXYn6PEe3m377i0huI+vWt0GnLgQOVbIQ0+ FpBJeqgJKh7w== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,306,1580803200"; d="scan'208";a="293519322" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by FMSMGA003.fm.intel.com with ESMTP; 25 Mar 2020 20:58:17 -0700 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) 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:58:16 -0700 Received: from shsmsx154.ccr.corp.intel.com (10.239.6.54) by fmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 25 Mar 2020 20:58:16 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.50]) by SHSMSX154.ccr.corp.intel.com ([169.254.7.145]) with mapi id 14.03.0439.000; Thu, 26 Mar 2020 11:58:13 +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//+FKwCAAIxksA== Date: Thu, 26 Mar 2020 03:58:12 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503F99CA3E@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> <74D8A39837DF1E4DA445A8C0B3885C503F99C86E@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 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 ad= ded 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 su= re 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 > Sent: Thursday, March 26, 2020 11:29 AM > To: Yao, Jiewen ; 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 >=20 > > -----Original Message----- > > From: Yao, Jiewen > > Sent: Wednesday, March 25, 2020 20:05 > > To: Zurcher, Christopher J ; > > devel@edk2.groups.io > > Cc: Wang, Jian J ; Lu, XiaoyuX > ; > > Ard Biesheuvel ; david.harris4@hp.com; Kinn= ey, > > Michael D > > 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 > > > 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; Ki= nney, > > > Michael D > > > Subject: RE: [edk2-devel] [PATCH 1/1] CryptoPkg/OpensslLib: Add nati= ve > > > instruction support for IA32 and X64 > > > > > > The specific performance improvement depends on the operation; the O= S > > > provisioning I mentioned in the [Patch 0/1] thread removed the hashi= ng 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 part= icular > > > 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 b= y 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 dire= ctly > > from > > > OpenSSL and includes checks against the CPUID capability flags befor= e > > 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, a= nd > > 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 ca= n > work > > correct. > > > > Maybe I should ask more generic question: Have you validated all impac= ted > > crypto lib API to make sure they still work well with this improvement= ? > > >=20 > I have not. Is there an existing test suite that was used initially, or = used with > each OpenSSL version update? >=20 > Thanks, > Christopher Zurcher >=20 > > > > > > > > Regarding RNG, it looks like we already have architecture-specific v= ariants > > 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/OpensslL= ib/ > ra > > nd_pool.c > > Since this patch adds instruction dependency, why no use RNG instructi= on as > > well? > > > > > > > > > > There was some off-list feedback regarding the number of files requi= red to > > be > > > checked in here. OpenSSL does not include assembler-specific > > implementations > > > of these files and instead relies on "perlasm" scripts which are par= sed 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 gen= erates > > these > > > files as part of the OpensslLib maintainer process, similar to the e= xisting > > header > > > files which are also generated. Since process_files.pl already requi= res the > > > package maintainer to have a Perl environment installed, this does n= ot > > 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 depende= ncies > > > (such as a version check against the NASM executable) for every deve= loper, > > 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 "intermedia= te" > > 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 On Behalf Of Yao= , > > > Jiewen > > > > Sent: Wednesday, March 25, 2020 18:23 > > > > To: devel@edk2.groups.io; Yao, Jiewen ; Zurc= her, > > > > Christopher J > > > > Cc: Wang, Jian J ; Lu, XiaoyuX > > > ; > > > > Eugene Cohen ; Ard Biesheuvel > > > > > Subject: Re: [edk2-devel] [PATCH 1/1] CryptoPkg/OpensslLib: Add na= tive > > > > 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 regi= ster. > > > > Have you validated that code, to make sure it can work correctly i= n > > current > > > > environment? > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > From: devel@edk2.groups.io On Behalf Of Y= ao, > > > 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 = 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 ge= t? > > > > > 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 OpensslL= ibX64. > > > > > 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 na= tive > > > > > instruction > > > > > > support for IA32 and X64 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >=20 > > > > > >=20