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.23.1580339764616212782 for ; Wed, 29 Jan 2020 15:16:04 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.120, mailfrom: michael.d.kinney@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Jan 2020 15:16:04 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,379,1574150400"; d="scan'208";a="229770532" Received: from orsmsx110.amr.corp.intel.com ([10.22.240.8]) by orsmga003.jf.intel.com with ESMTP; 29 Jan 2020 15:16:03 -0800 Received: from orsmsx126.amr.corp.intel.com (10.22.240.126) by ORSMSX110.amr.corp.intel.com (10.22.240.8) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 29 Jan 2020 15:16:03 -0800 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.57]) by ORSMSX126.amr.corp.intel.com ([169.254.4.210]) with mapi id 14.03.0439.000; Wed, 29 Jan 2020 15:16:02 -0800 From: "Michael D Kinney" To: "Sukerkar, Amol N" , "devel@edk2.groups.io" , "Kinney, Michael D" CC: "Yao, Jiewen" , "Wang, Jian J" , "Agrawal, Sachin" , "Musti, Srinivas" , "Lakkimsetti, Subash" Subject: Re: [PATCH v6 0/2] CryptoPkg/HashApiLib: Implement Unified Hash Calculation API Thread-Topic: [PATCH v6 0/2] CryptoPkg/HashApiLib: Implement Unified Hash Calculation API Thread-Index: AQHV1gVRMo3qASUng0qpuAl+3tncSKgB5WFwgABfN6CAAAJecA== Date: Wed, 29 Jan 2020 23:16:02 +0000 Message-ID: References: <20200128180340.15136-1-amol.n.sukerkar@intel.com> In-Reply-To: Accept-Language: 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.22.254.139] MIME-Version: 1.0 Return-Path: michael.d.kinney@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Amol, The feedback I received was for the use of the term "Base" in library class names. You did the correct change by changing the class name from "BaseHashLib" to "HashApiLib". It is correct to use the term "Base" in the name of=20 a library instance if the library as implemented is compatible with all module types. The one implementation of the HashApiLib class is compatible with all module types, so the library instance name should be "BaseHashApiLib". Mike > -----Original Message----- > From: Sukerkar, Amol N > Sent: Wednesday, January 29, 2020 3:10 PM > To: Kinney, Michael D ; > devel@edk2.groups.io > Cc: Yao, Jiewen ; Wang, Jian J > ; Agrawal, Sachin > ; Musti, Srinivas > ; Lakkimsetti, Subash > ; Sukerkar, Amol N > > Subject: RE: [PATCH v6 0/2] CryptoPkg/HashApiLib: > Implement Unified Hash Calculation API >=20 > Hi Mike, >=20 > Question about point 4. Could you help me clear the > confusion? >=20 > 4) The name of the HashApiLib instance should be > "BaseHashApiLib" and the > should be in the CryptoPkg/Library/BaseHashApiLib > directory with > files BashHashApiLib.inf, BaseHashApiLib.c, and > BaseHashApiLib.uni. > BASE_NAME in BaseHashApiLib.iunf should also be > BaseHashApiLib. >=20 > Perhaps I am not very clear but it appears you are > contradicting your earlier feedback: >=20 > I have received feedback before against the use of the > term "Base" in the name of a library class. It causes > confusion because the term "Base" usually applies to > the library implementation to describe the module type > compatibility of the lib instance. Take BaseCryptLib > class as an example. There are instances of this > library that are specific to PEI, DXE, SMM, and > Runtime. If we look at the entire edk2 repo, there are > only 3 lib classes that start with the term "Base": >=20 > BaseLib - OK. Single instance of type BASE. > BaseMemoryLib - Confusing. BASE, PEI, DXE versions > BaseCryptLib - Confusing. PEI, DXE, SMM, RT > versions >=20 > I also see the BaseHashLib service names use "HashApi". > In order to address both the use of the term "Base" and > The inconsistency between the lib class name and the > lib service names, I recommend the lib class be changed > from "BaseHashLib" to "HashApiLib" along with a few > other name changes to scope defines and types to the > HashApiLib: >=20 > BaseHashLib -> HashApiLib > HASH_HANDLE -> HASH_API_CONTEXT > HASH_INVALID -> HASH_API_ALGORITHM_INVALID > HASH_MD4 -> HASH_API_ALGORITHM_MD4 > HASH_MD5 -> HASH_API_ALGORITHM_MD5 > HASH_SHA1 -> HASH_API_ALGORITHM_SHA1 > HASH_SHA256 -> HASH_API_ALGORITHM_SHA256 > HASH_SHA384 -> HASH_API_ALGORITHM_SHA384 > HASH_SHA512 -> HASH_API_ALGORITHM_SHA512 > HASH_SM3_256 -> HASH_API_ALGORITHM_SM3_256 > HASH_MAX -> Remove. Not used. >=20 > Some file name a directory names changes would also be > required to follow this same pattern. >=20 > Thanks, > Amol >=20 > -----Original Message----- > From: Kinney, Michael D > Sent: Wednesday, January 29, 2020 1:10 PM > To: Sukerkar, Amol N ; > devel@edk2.groups.io; Kinney, Michael D > > Cc: Yao, Jiewen ; Wang, Jian J > ; Agrawal, Sachin > ; Musti, Srinivas > ; Lakkimsetti, Subash > > Subject: RE: [PATCH v6 0/2] CryptoPkg/HashApiLib: > Implement Unified Hash Calculation API >=20 > Amol, >=20 >=20 > 1) Typo in CryptoPkg.dec. Should be Crypto Package, > not Security package. >=20 > [Guids] > ## Security package token space guid. >=20 > 2) CryptoPkg.dec/uni. I see the default value for > PcdHashApiLibPolicy > is 0x04. This is documented to be SHA256. The > DEC/UNI file > descriptions of this PCD should state that the > default policy is > SHA256. This makes it clear to platform developers > that maintain > DSC files what the default policy is. >=20 > 3) CryptoPkg.dsc: The same HashApiLib instance is used > for all module types > so a single mapping can be moved to [LibraryClasses] > section and the > DSC file and removed from the > [LibraryClass.common.] sections. >=20 > 4) The name of the HashApiLib instance should be > "BaseHashApiLib" and the > should be in the CryptoPkg/Library/BaseHashApiLib > directory with > files BashHashApiLib.inf, BaseHashApiLib.c, and > BaseHashApiLib.uni. > BASE_NAME in BaseHashApiLib.iunf should also be > BaseHashApiLib. >=20 > 5) In order to be consistent with other EDK II context > typedefs, I recommend >=20 > typedef VOID *HASH_API_CONTEXT; >=20 > Also update APIs to use HashContext instead of > *HashContext. >=20 > 6) HashApiDuplicate() - The NewHashContext parameter > should be type > HASH_API_CONTEXT. >=20 > 7) HashApiLib.inf - I think you can remove > MdeModulePkg.dec from [Packages] >=20 > Thanks, >=20 > Mike >=20 > > -----Original Message----- > > From: Sukerkar, Amol N > > Sent: Tuesday, January 28, 2020 10:04 AM > > To: devel@edk2.groups.io > > Cc: Kinney, Michael D ; > Yao, Jiewen > > ; Wang, Jian J > ; Agrawal, > > Sachin ; Musti, Srinivas > > ; Lakkimsetti, Subash > > > > Subject: [PATCH v6 0/2] CryptoPkg/HashApiLib: > Implement Unified Hash > > Calculation API > > > > Currently, the UEFI drivers using the SHA/SM3 hashing > algorithms use > > hard-coded API to calculate the hash, for instance, > sha_256(...), etc. > > Since SHA384 and/or > > SM3_256 are being increasingly adopted for > robustness, it becomes > > cumbersome to modify each driver that calls into hash > calculating API. > > > > To better achieve this, we are proposing a Unified > API, which can be > > used by UEFI drivers, that provides the drivers with > flexibility to > > use the desired hashing algorithm based on the > required robnustness. > > > > Alternatively, the design document is also attached > to Bugzilla, > > https://bugzilla.tianocore.org/show_bug.cgi?id=3D2151. > > > > Sukerkar, Amol N (2): > > CryptoPkg: Add CryptoPkg Token Space GUID > > CryptoPkg/HashApiLib: Implement Unified Hash > Calculation API > > > > CryptoPkg/Library/HashApiLib/HashApiLib.c | 333 > > ++++++++++++++++++++ > > CryptoPkg/CryptoPkg.dec | 27 +- > > CryptoPkg/CryptoPkg.dsc | 7 +- > > CryptoPkg/CryptoPkg.uni | 17 + > > CryptoPkg/Include/Library/HashApiLib.h | 122 > > +++++++ > > CryptoPkg/Library/HashApiLib/HashApiLib.inf | 45 > +++ > > CryptoPkg/Library/HashApiLib/HashApiLib.uni | 17 + > > 7 files changed, 566 insertions(+), 2 deletions(-) > create mode > > 100644 CryptoPkg/Library/HashApiLib/HashApiLib.c > > create mode 100644 > > CryptoPkg/Include/Library/HashApiLib.h > > create mode 100644 > > CryptoPkg/Library/HashApiLib/HashApiLib.inf > > create mode 100644 > > CryptoPkg/Library/HashApiLib/HashApiLib.uni > > > > -- > > 2.16.2.windows.1 >=20 >=20