Re: [PATCH v5 0/2] CryptoPkg/BaseHashLib: Implement Unified Hash Calculation API

["Sukerkar, Amol N" <amol.n.sukerkar@intel.com>]

Thanks, Mike! I agree with the feedback below. I will upload the changes soon.

~ Amol

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com> 
Sent: Sunday, January 26, 2020 12:48 PM
To: Sukerkar, Amol N <amol.n.sukerkar@intel.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Agrawal, Sachin <sachin.agrawal@intel.com>; Musti, Srinivas <srinivas.musti@intel.com>; Lakkimsetti, Subash <subash.lakkimsetti@intel.com>
Subject: RE: [PATCH v5 0/2] CryptoPkg/BaseHashLib: Implement Unified Hash Calculation API

Amol,

Thank your for providing patches for this feature.
You have done a very good job on following the design patterns from similar feature types in the EDK II and following the EDK II coding style.  I do have some recommended changes below and have attempted to provide reasons for these recommendations.
Please let me know if you have any questions or need any clarifications

I am thinking of the use case where a developer is updating an existing module that uses the BaseCryptLib to use the BaseHashLib.  The BaseHashLib does not provide that same set of services per hash algorithm which means there may be module that would like to switch to BaseHashLib, but can't.  

Can we update BaseHashLib to provide the same set of
services:

	HashApiGetContextSize()
	HashApiInit()
	HashApiDuplicate()
	HashApiUpdate()
	HashApiFinal()
	HashApiHashAll()

The BaseHashLib uses the term "Handle" where the BaseCryptLib uses the term "Context".  This may confuse developers that are porting from BaseCryptLib usage to BaseHashLib usage.  I recommend we continue the use of the term "Context" in BasehHashLib.

The BaseCryptLib supports PEI, DXE, SMM, and Runtime.
is there a reason that BaseHashLib only supports PEI and DXE INFs?  Upon further review, the implementation of this library only uses the services of other library classes and is not phase specific at all.  I think this could be a single INF/UNI file  with a MODULE_TYPE of BASE and include <Base.h> at the top of BaseHashLib.c.
This lib instance would be compatible with all module types.

I see the PCD name " PcdSystemHashPolicy".  This PCD can be set in the context of a single module if different modules in a platform FW build use different hash algorithms.
As a result, this PCD can be used for both system wide policy settings using a Dynamic/DynamicEx PCD type or a module scoped PCD setting using a FixedAtBuild or PatchableInModule PCD type.  This PCD is scoped for use by the BaseHashLib.  Perhaps a better name would be "PcdHashApiLibPolicy".

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

  BaseLib        - OK.  Single instance of type BASE.
  BaseMemoryLib  - Confusing.  BASE, PEI, DXE versions
  BaseCryptLib   - Confusing.  PEI, DXE, SMM, RT versions

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:

  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.

Some file name a directory names changes would also be required to follow this same pattern.

The include file "CryptoPkgTokenSpace.h" can be removed.  I know the pattern in the existing packages contains this include file, but there was a change in the autogen that supports defining a GUID value in the DEC file without requiring an associated .h file.  This new pattern only applies to GUIDs that have no associated data structures, Protocols, or PPIs.  A token space GUID is a good example of a GUID value with no associated structures.

BaseHashLib.h includes the file <Uefi.h>.  This can be removed.  Modules and library instances that use a library class will include a top level include file that matches the MODULE_TYPE before including any library class .h files or protocols/ppi/guid .h files. 

  <Base.h>
  <PiPei.h>
  <PiDxe.h>
  <PiSmm.h>
  <PiMm.h>
  <Uefi.h>

Thanks,

Mike

> -----Original Message-----
> From: Sukerkar, Amol N <amol.n.sukerkar@intel.com>
> Sent: Thursday, January 23, 2020 8:47 AM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen 
> <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Agrawal, 
> Sachin <sachin.agrawal@intel.com>; Musti, Srinivas 
> <srinivas.musti@intel.com>; Lakkimsetti, Subash 
> <subash.lakkimsetti@intel.com>
> Subject: [PATCH v5 0/2] CryptoPkg/BaseHashLib: 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=2151.
> 
> Sukerkar, Amol N (2):
>   CryptoPkg: Add CryptoPkg Token Space GUID
>   CryptoPkg/BaseHashLib: Implement Unified Hash Calculation API
> 
>  CryptoPkg/Library/BaseHashLib/BaseHashLib.c      | 228
> ++++++++++++++++++++
>  CryptoPkg/CryptoPkg.dec                          |  28
> ++-
>  CryptoPkg/CryptoPkg.dsc                          |   6
> +-
>  CryptoPkg/CryptoPkg.uni                          |  17
> ++
>  CryptoPkg/Include/Guid/CryptoPkgTokenSpace.h     |  19
> ++
>  CryptoPkg/Include/Library/BaseHashLib.h          |  81
> +++++++
>  CryptoPkg/Library/BaseHashLib/BaseHashLibDxe.inf |  44
> ++++
>  CryptoPkg/Library/BaseHashLib/BaseHashLibDxe.uni |  17
> ++
>  CryptoPkg/Library/BaseHashLib/BaseHashLibPei.inf |  45
> ++++
>  CryptoPkg/Library/BaseHashLib/BaseHashLibPei.uni |  17
> ++
>  10 files changed, 500 insertions(+), 2 deletions(-)  create mode 
> 100644 CryptoPkg/Library/BaseHashLib/BaseHashLib.c
>  create mode 100644
> CryptoPkg/Include/Guid/CryptoPkgTokenSpace.h
>  create mode 100644
> CryptoPkg/Include/Library/BaseHashLib.h
>  create mode 100644
> CryptoPkg/Library/BaseHashLib/BaseHashLibDxe.inf
>  create mode 100644
> CryptoPkg/Library/BaseHashLib/BaseHashLibDxe.uni
>  create mode 100644
> CryptoPkg/Library/BaseHashLib/BaseHashLibPei.inf
>  create mode 100644
> CryptoPkg/Library/BaseHashLib/BaseHashLibPei.uni
> 
> --
> 2.16.2.windows.1