From: "Sukerkar, Amol N" <amol.n.sukerkar@intel.com>
To: "Kinney, Michael D" <michael.d.kinney@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
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>,
"Sukerkar, Amol N" <amol.n.sukerkar@intel.com>
Subject: Re: [PATCH v6 0/2] CryptoPkg/HashApiLib: Implement Unified Hash Calculation API
Date: Wed, 29 Jan 2020 23:09:44 +0000 [thread overview]
Message-ID: <MWHPR11MB0064D17B91E6BCB5D54387FEAC050@MWHPR11MB0064.namprd11.prod.outlook.com> (raw)
In-Reply-To: <E92EE9817A31E24EB0585FDF735412F5B9E80EB6@ORSMSX113.amr.corp.intel.com>
Hi Mike,
Question about point 4. Could you help me clear the confusion?
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.
Perhaps I am not very clear but it appears you are contradicting your earlier feedback:
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.
Thanks,
Amol
-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Wednesday, January 29, 2020 1:10 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 v6 0/2] CryptoPkg/HashApiLib: Implement Unified Hash Calculation API
Amol,
1) Typo in CryptoPkg.dec. Should be Crypto Package, not Security package.
[Guids]
## Security package token space guid.
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.
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.<ModuleType>] sections.
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.
5) In order to be consistent with other EDK II context typedefs, I recommend
typedef VOID *HASH_API_CONTEXT;
Also update APIs to use HashContext instead of *HashContext.
6) HashApiDuplicate() - The NewHashContext parameter should be type
HASH_API_CONTEXT.
7) HashApiLib.inf - I think you can remove MdeModulePkg.dec from [Packages]
Thanks,
Mike
> -----Original Message-----
> From: Sukerkar, Amol N <amol.n.sukerkar@intel.com>
> Sent: Tuesday, January 28, 2020 10:04 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 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=2151.
>
> 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
next prev parent reply other threads:[~2020-01-29 23:09 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-28 18:03 [PATCH v6 0/2] CryptoPkg/HashApiLib: Implement Unified Hash Calculation API Sukerkar, Amol N
2020-01-28 18:03 ` [PATCH v6 1/2] CryptoPkg: Add CryptoPkg Token Space GUID Sukerkar, Amol N
2020-01-28 18:03 ` [PATCH v6 2/2] CryptoPkg/HashApiLib: Implement Unified Hash Calculation API Sukerkar, Amol N
2020-01-29 20:10 ` [PATCH v6 0/2] " Michael D Kinney
2020-01-29 23:09 ` Sukerkar, Amol N [this message]
2020-01-29 23:16 ` Michael D Kinney
2020-01-29 23:23 ` Sukerkar, Amol N
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=MWHPR11MB0064D17B91E6BCB5D54387FEAC050@MWHPR11MB0064.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