Thanks Mike. I understand the *private protocol* now. I think it is OK to put CryptoProtocol into Private dir to sever that purpose. And for private API, I don’t have concern on the data structure, since it is invisible.
Siyuan provided below usage and concern: "The goal to provide a driver version of crypto service is to support modulization FW update, which means the crypto driver may NOT be updated together with its consumer. A platform may choose to update the crypto service driver to a new version with this patch, then all the BaseCryptoLib consumers will be messed." This usage might become a problem, when we want to deprecate an API and keep binary compatibility at same time. (To be specific, I am not worried about source compatibility, because we can update both producer and consumer. I am not worried about adding API, because there will be no issue on appending a function at the end.) Take below as an example: Firmware Version 100 uses Crypto Version 100. We want to deprecate a private API and change to a new one. So, we upgrade Crypto to Version 101 and update Firmware to Version 101. Of course, we need change *all other consumers* and rebuild everything make sure it works correctly. However, it is hard to support this in "modulization FW update", because we have no chance to update the binary of firmware version 100. If we have to keep *permanent binary compatibility*, then we cannot deprecate any old API, just because that will break old consumer. That brings much validation burden, because you have to test every update in master with old binaries, besides the latest binaries. That also brings maintenance burden for the unused old API. The only consumer is in the old binary and invisible. I don’t believe that is what we want. Modulization FW update is good feature. And we can have different strategy for that besides keeping permanent binary compatibility. 1) Modulization FW update can be limited a range of version. At some point, you have to update the whole FW, because there are too many changes or incompatible binary changes. The cadence of full update can be longer than the one of partial update. For example, Linux or windows are making incompatible change in major version and only keep compatibility in minor version. 2) A project can branch the production launch firmware, and only keep binary compatibility and support the modulization FW update within this branch. As such, the big update in master won't impact this branch. If a production may choose to resync to master, at that time a full firmware update is required. I guess most people are using this way in a real production. Thought? Thank you Yao Jiewen > -----Original Message----- > From: Kinney, Michael D <michael.d.kin...@intel.com> > Sent: Saturday, March 28, 2020 12:38 AM > To: devel@edk2.groups.io; Yao, Jiewen <jiewen....@intel.com>; Fu, Siyuan > <siyuan...@intel.com>; Gao, Zhichao <zhichao....@intel.com>; Kinney, > Michael D <michael.d.kin...@intel.com> > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX <xiaoyux...@intel.com>; > Maciej Rabeda <maciej.rab...@linux.intel.com>; Wu, Jiaxin > <jiaxin...@intel.com> > Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire the deprecate function > > Jiewen, > > The purpose of private includes is to keep modules/lib > in *other* packages from using interfaces that are the > package with the private interface does not want other > packages to use and does not want to have to coordinate > with other packages if that package owner decides to > make changes to those private interfaces. > > For modules/libs within package that do use private > includes, the package owner gets to decide how to > maintain the interfaces in the private includes to > support those modules/libs. > > For example, the CryptoPkg has modules that are > intended to be used as pre-built binaries, so the > CryptoPkg owner needs to make sure the maintenance > of the private includes supports both the source and > binary use cases. > > The private Protocol/PPI interfaces in the CryptoPkg > were designed to support future expansion. The first > API in the Protocol/PPI is GetVersion(). The version > value returned can be used to have different layouts > of fields in the Protocol/PPI. In order to support > backwards compatibility, APIs are added to the end > of the Protocol/PPI structure as the version value > is increased. You will notice that there is an X509 > service that was added further down than the initial > grouping. This is just an example of how the CryptoPkg > is maintaining a private interface for binary use cases. > Other packages may choose alternate techniques. > > Thanks, > > Mike > > > -----Original Message----- > > From: devel@edk2.groups.io <devel@edk2.groups.io> On > > Behalf Of Yao, Jiewen > > Sent: Thursday, March 26, 2020 9:59 PM > > To: Fu, Siyuan <siyuan...@intel.com>; > > devel@edk2.groups.io; Gao, Zhichao > > <zhichao....@intel.com> > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX > > <xiaoyux...@intel.com>; Maciej Rabeda > > <maciej.rab...@linux.intel.com>; Wu, Jiaxin > > <jiaxin...@intel.com> > > Subject: Re: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire > > the deprecate function > > > > Thanks Siyun. > > I think probably we need discuss this more. > > > > 1) About private v.s. public. > > > > The benefit for private include is to isolate external > > interface and internal interface. > > A package can keep updating its private interface without > > impact any other packages. > > However, in this case, a private interface update will > > bring binary compatibility issue with other package. > > I am not sure it is acceptable or not. > > > > Mike > > Do you have any comment? Is that the design goal of > > private interface - just keep source code compatibility, > > but not binary compatiblity? > > > > 2) About the protocol itself. > > > > One concern I have is that we *hardcode* the algorithm in > > protocol. > > > > We keeps adding new algorithm and removing old one. That > > means this protocol interface is unstable. > > > > Today, we have defined SHA2 set, and deprecating SHA1 and > > older one. Tomorrow we may need add SHA3 set. > > Today, we have RSAPKCS1_15. Tomorrow we will have RSAPSS. > > Some other new set of algorithms might be added later, > > such as AEAD, GMAC. > > > > For a protocol definition, I think we need *abstract the > > action*, but not *algorithm*. > > One good example is the UEFI HASH2 Protocol. > > https://github.com/tianocore/edk2/blob/master/MdePkg/Incl > > ude/Protocol/Hash2.h > > It just tells you do the hash. You may add new algorithm > > GUID. > > > > Another good example is inside of openssl. Now it is > > using EVP style cipher algo. > > For example, > > https://www.openssl.org/docs/man1.1.1/man3/EVP_EncryptIni > > t_ex.html > > https://www.openssl.org/docs/man1.1.0/man3/EVP_CIPHER_CTX > > _ctrl.html > > The cipher itself is input as parameter. > > > > The benefit is that, if we want to deprecate an > > algorithm, the interface can be unchanged. > > Just the internal implementation can be changed. > > The current PCD mechanism can still be applied to > > internal implementation. > > > > Can we get a chance to revisit/redesign the protocol API, > > when we move to public include? > > > > Thank you > > Yao Jiewen > > > > > -----Original Message----- > > > From: Fu, Siyuan <siyuan...@intel.com> > > > Sent: Friday, March 27, 2020 11:07 AM > > > To: Yao, Jiewen <jiewen....@intel.com>; > > devel@edk2.groups.io; Gao, Zhichao > > > <zhichao....@intel.com> > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX > > <xiaoyux...@intel.com>; > > > Maciej Rabeda <maciej.rab...@linux.intel.com>; Wu, > > Jiaxin > > > <jiaxin...@intel.com> > > > Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire > > the deprecate function > > > > > > Hi, Jiewen > > > > > > Although the protocol is private, a corresponding > > BaseCryptoLib instance is > > > not private, like PeiCryptLib.inf, RuntimeCryptLib, > > etc. These library instances > > > will be static linked to the consumer driver, for > > example an iSCSI network driver. > > > So actually it's not a "private" change inside > > CryptoPkg. > > > > > > The goal to provide a driver version of crypto service > > is to support modulization > > > FW update, which means the crypto driver may NOT be > > updated together with > > > its consumer. A platform may choose to update the > > crypto service driver to a > > > new version with this patch, then all the BaseCryptoLib > > consumers will be > > > messed. > > > > > > Best Regards > > > Siyuan > > > > > > > -----Original Message----- > > > > From: Yao, Jiewen <jiewen....@intel.com> > > > > Sent: 2020年3月27日 10:58 > > > > To: devel@edk2.groups.io; Fu, Siyuan > > <siyuan...@intel.com>; Gao, Zhichao > > > > <zhichao....@intel.com> > > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX > > > <xiaoyux...@intel.com>; > > > > Maciej Rabeda <maciej.rab...@linux.intel.com>; Wu, > > Jiaxin > > > > <jiaxin...@intel.com> > > > > Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg: > > Retire the deprecate > > > function > > > > > > > > EDKII_CRYPTO_PROTOCOL is *private*. > > > > > > > > > https://github.com/tianocore/edk2/blob/master/CryptoPkg/P > > rivate/Protocol/C > > > > rypto.h > > > > > > > > Why we cannot change? > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> > > On Behalf Of Siyuan, > > > Fu > > > > > Sent: Friday, March 27, 2020 10:47 AM > > > > > To: Gao, Zhichao <zhichao....@intel.com>; > > devel@edk2.groups.io > > > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, > > XiaoyuX > > > > <xiaoyux...@intel.com>; > > > > > Maciej Rabeda <maciej.rab...@linux.intel.com>; Wu, > > Jiaxin > > > > > <jiaxin...@intel.com> > > > > > Subject: Re: [edk2-devel] [PATCH 0/8] CryptoPkg: > > Retire the deprecate > > > > function > > > > > > > > > > Hi, Zhichao > > > > > > > > > > We should never move/delete a member field of a > > previous defined protocol > > > > > Interface. Instead, these protocol APIs shall be > > kept and return an error code > > > > > If the function is retired. Otherwise the consumer > > driver may call into an > > > > > Incorrect function if it's build with different > > codebase/PCD settings with the > > > > > Crypto PEI/DXE/SMM driver. > > > > > This comment applies to all the > > EDKII_CRYPTO_PROTOCOL related changes > > > in > > > > > your patch set. > > > > > > > > > > Best Regards > > > > > Siyuan > > > > > > > > > > > -----Original Message----- > > > > > > From: Gao, Zhichao <zhichao....@intel.com> > > > > > > Sent: 2020年3月27日 9:56 > > > > > > To: devel@edk2.groups.io > > > > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, > > XiaoyuX > > > > > <xiaoyux...@intel.com>; > > > > > > Maciej Rabeda <maciej.rab...@linux.intel.com>; > > Wu, Jiaxin > > > > > > <jiaxin...@intel.com>; Fu, Siyuan > > <siyuan...@intel.com> > > > > > > Subject: [PATCH 0/8] CryptoPkg: Retire the > > deprecate function > > > > > > > > > > > > REF: > > https://bugzilla.tianocore.org/show_bug.cgi?id=1682 > > > > > > REF: > > https://bugzilla.tianocore.org/show_bug.cgi?id=1898 > > > > > > > > > > > > MD4, AR4, Tdes, Aes Ecb mode, MD5 and SHA1 is not > > secure any longer. > > > > > > They are all deprecated. Edk2 would not support > > them any longer. > > > > > > So remove them. > > > > > > But uefi spec want to keep MD5 and SHA1 for > > backwards compatibility. > > > > > > So add two pcds to control the MD5 and SHA1 > > enablement. Set the pcds > > > > > > default value to false to indicate they are > > deprecated. > > > > > > > > > > > > NetWorkPkg's iSCSI driver would consume the MD5 > > function, so change > > > > > > the md5 pcd to TURE when iSCSI is enabled. > > > > > > > > > > > > Cc: Jian J Wang <jian.j.w...@intel.com> > > > > > > Cc: Xiaoyu Lu <xiaoyux...@intel.com> > > > > > > Cc: Maciej Rabeda <maciej.rab...@linux.intel.com> > > > > > > Cc: Jiaxin Wu <jiaxin...@intel.com> > > > > > > Cc: Siyuan Fu <siyuan...@intel.com> > > > > > > Signed-off-by: Zhichao Gao > > <zhichao....@intel.com> > > > > > > > > > > > > Zhichao Gao (8): > > > > > > CryptoPkg/BaseCrpytLib: Retire MD4 algorithm > > > > > > CryptoPkg/BaseCryptLib: Retire ARC4 algorithm > > > > > > CryptoPkg/BaseCryptLib: Retire the Tdes > > algorithm > > > > > > CryptoPkg/BaseCryptLib: Retire Aes Ecb mode > > algorithm > > > > > > CryptoPkg/dec: Add pcds to avoid building the > > deprecated function > > > > > > NetWorkPkg/Pcd.inc: Enable the MD5 for iSCSI > > > > > > Crypto/BaseCryptLib: Using pcd to control MD5 > > enablement > > > > > > CryptoPkg/BaseCryptLib: Use Pcd to control the > > SHA1 enablement > > > > > > > > > > > > CryptoPkg/CryptoPkg.dec | > > 11 + > > > > > > CryptoPkg/CryptoPkg.uni | > > 11 + > > > > > > CryptoPkg/Driver/Crypto.c | > > 634 +----------------- > > > > > > CryptoPkg/Include/Library/BaseCryptLib.h | > > 548 --------------- > > > > > > .../Library/BaseCryptLib/BaseCryptLib.inf | > > 9 +- > > > > > > .../Library/BaseCryptLib/Cipher/CryptAes.c | > > 114 ---- > > > > > > .../BaseCryptLib/Cipher/CryptAesNull.c | > > 52 -- > > > > > > .../Library/BaseCryptLib/Cipher/CryptArc4.c | > > 205 ------ > > > > > > .../BaseCryptLib/Cipher/CryptArc4Null.c | > > 124 ---- > > > > > > .../Library/BaseCryptLib/Cipher/CryptTdes.c | > > 364 ---------- > > > > > > .../BaseCryptLib/Cipher/CryptTdesNull.c | > > 160 ----- > > > > > > .../Library/BaseCryptLib/Hash/CryptMd4.c | > > 223 ------ > > > > > > .../Library/BaseCryptLib/Hash/CryptMd4Null.c | > > 143 ---- > > > > > > .../Library/BaseCryptLib/Hash/CryptMd5.c | > > 5 +- > > > > > > .../Library/BaseCryptLib/Hmac/CryptHmacMd5.c | > > 3 + > > > > > > .../BaseCryptLib/Hmac/CryptHmacMd5Null.c | > > 3 + > > > > > > .../Library/BaseCryptLib/Hmac/CryptHmacSha1.c | > > 3 + > > > > > > .../BaseCryptLib/Hmac/CryptHmacSha1Null.c | > > 3 + > > > > > > .../Library/BaseCryptLib/PeiCryptLib.inf | > > 13 +- > > > > > > .../BaseCryptLib/Pk/CryptPkcs5Pbkdf2.c | > > 3 + > > > > > > .../Library/BaseCryptLib/Pk/CryptRsaBasic.c | > > 5 + > > > > > > .../Library/BaseCryptLib/Pk/CryptRsaExt.c | > > 5 + > > > > > > .../Library/BaseCryptLib/RuntimeCryptLib.inf | > > 13 +- > > > > > > .../Library/BaseCryptLib/SmmCryptLib.inf | > > 13 +- > > > > > > .../BaseCryptLibNull/BaseCryptLibNull.inf | > > 3 - > > > > > > .../BaseCryptLibNull/Cipher/CryptAesNull.c | > > 54 +- > > > > > > .../BaseCryptLibNull/Cipher/CryptArc4Null.c | > > 124 ---- > > > > > > .../BaseCryptLibNull/Cipher/CryptTdesNull.c | > > 160 ----- > > > > > > .../BaseCryptLibNull/Hash/CryptMd4Null.c | > > 143 ---- > > > > > > .../BaseCryptLibNull/Hash/CryptMd5Null.c | > > 3 + > > > > > > .../BaseCryptLibNull/Hmac/CryptHmacMd5Null.c | > > 3 + > > > > > > .../BaseCryptLibNull/Hmac/CryptHmacSha1Null.c | > > 4 +- > > > > > > .../BaseCryptLibOnProtocolPpi/CryptLib.c | > > 604 +---------------- > > > > > > .../Library/BaseHashApiLib/BaseHashApiLib.c | > > 12 + > > > > > > .../Library/BaseHashApiLib/BaseHashApiLib.inf | > > 1 + > > > > > > CryptoPkg/Private/Protocol/Crypto.h | > > 583 +--------------- > > > > > > NetworkPkg/NetworkPcds.dsc.inc | > > 5 +- > > > > > > 37 files changed, 145 insertions(+), 4221 > > deletions(-) > > > > > > delete mode 100644 > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptArc4.c > > > > > > delete mode 100644 > > > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptArc4Null.c > > > > > > delete mode 100644 > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptTdes.c > > > > > > delete mode 100644 > > > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptTdesNull.c > > > > > > delete mode 100644 > > CryptoPkg/Library/BaseCryptLib/Hash/CryptMd4.c > > > > > > delete mode 100644 > > > CryptoPkg/Library/BaseCryptLib/Hash/CryptMd4Null.c > > > > > > delete mode 100644 > > > > > > > > CryptoPkg/Library/BaseCryptLibNull/Cipher/CryptArc4Null.c > > > > > > delete mode 100644 > > > > > > > > CryptoPkg/Library/BaseCryptLibNull/Cipher/CryptTdesNull.c > > > > > > delete mode 100644 > > > > > > > CryptoPkg/Library/BaseCryptLibNull/Hash/CryptMd4Null.c > > > > > > > > > > > > -- > > > > > > 2.21.0.windows.1 > > > > > > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56520): https://edk2.groups.io/g/devel/message/56520 Mute This Topic: https://groups.io/mt/72579461/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
