* [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c @ 2020-10-28 18:42 Zurcher, Christopher J 2020-10-28 18:42 ` [PATCH 1/1] " Zurcher, Christopher J 2020-10-31 1:10 ` [PATCH 0/1] " Yao, Jiewen 0 siblings, 2 replies; 10+ messages in thread From: Zurcher, Christopher J @ 2020-10-28 18:42 UTC (permalink / raw) To: devel Cc: Jiewen Yao, Jian J Wang, Xiaoyu Lu, Guomin Jiang, Sung-Uk Bin, Ard Biesheuvel BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2507 This patch provides a drop-in replacement for CryptAes.c which utilizes the EVP interface to OpenSSL. This enables access to the assembly-optimized algorithms. This patch has been unit-tested in both VS and CLANG build environments using both an X64 assembly-optimized implementation of OpensslLib and the standard implementation. Usage of this file does not require an assembly-optimized implementation of OpensslLib to function; it does however require one to provide a speed improvement. The C-only AES implementation included by CryptAes.c is extremely small, and since this file includes the EVP interface, it will significantly increase the size of any module that includes it. As a result, I have not replaced the original CryptAes.c as a default in any of the CryptLib implementations. Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com> Cc: Guomin Jiang <guomin.jiang@intel.com> Cc: Sung-Uk Bin <sunguk-bin@hp.com> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com> Christopher J Zurcher (1): CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c | 262 ++++++++++++++++++++ 1 file changed, 262 insertions(+) create mode 100644 CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c -- 2.28.0.windows.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c 2020-10-28 18:42 [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c Zurcher, Christopher J @ 2020-10-28 18:42 ` Zurcher, Christopher J 2020-10-31 1:10 ` [PATCH 0/1] " Yao, Jiewen 1 sibling, 0 replies; 10+ messages in thread From: Zurcher, Christopher J @ 2020-10-28 18:42 UTC (permalink / raw) To: devel; +Cc: Jiewen Yao, Jian J Wang, Xiaoyu Lu, Guomin Jiang, Sung-Uk Bin BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2507 The EVP interface is required to access the assembly-optimized AES implementation. This file alone will not provide assembly-optimized code; it must be combined with an architecture-specific implementation of OpensslLib. This is a drop-in replacement for CryptAes.c. Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com> Cc: Guomin Jiang <guomin.jiang@intel.com> Cc: Sung-Uk Bin <sunguk-bin@hp.com> Signed-off-by: Christopher J Zurcher <christopher.j.zurcher@intel.com> --- CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c | 262 ++++++++++++++++++++ 1 file changed, 262 insertions(+) diff --git a/CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c b/CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c new file mode 100644 index 0000000000..e243228c43 --- /dev/null +++ b/CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c @@ -0,0 +1,262 @@ +/** @file + AES Wrapper Implementation over OpenSSL EVP (Envelope) interface. + +Copyright (c) 2010 - 2020, Intel Corporation. All rights reserved.<BR> +SPDX-License-Identifier: BSD-2-Clause-Patent + +**/ + +#include "InternalCryptLib.h" +#include <openssl/evp.h> + +typedef struct { + CONST UINT8 *Key; + CONST EVP_CIPHER *EvpCipher; +} AES_CONTEXT; + +/** + Retrieves the size, in bytes, of the context buffer required for AES operations. + + @return The size, in bytes, of the context buffer required for AES operations. + +**/ +UINTN +EFIAPI +AesGetContextSize ( + VOID + ) +{ + // + // Store Key locally to provide it to the worker functions + // + return (UINTN) sizeof (AES_CONTEXT); +} + +/** + Initializes user-supplied memory as AES context for subsequent use. + + This function initializes user-supplied memory pointed by AesContext as AES context. + In addition, it sets up all AES key materials for subsequent encryption and decryption + operations. + There are 3 options for key length, 128 bits, 192 bits, and 256 bits. + + If AesContext is NULL, then return FALSE. + If Key is NULL, then return FALSE. + If KeyLength is not valid, then return FALSE. + + @param[out] AesContext Pointer to AES context being initialized. + @param[in] Key Pointer to the user-supplied AES key. + @param[in] KeyLength Length of AES key in bits. + + @retval TRUE AES context initialization succeeded. + @retval FALSE AES context initialization failed. + +**/ +BOOLEAN +EFIAPI +AesInit ( + OUT VOID *AesContext, + IN CONST UINT8 *Key, + IN UINTN KeyLength + ) +{ + AES_CONTEXT *Context = AesContext; + + // + // Check input parameters. + // + if (AesContext == NULL || Key == NULL || (KeyLength != 128 && KeyLength != 192 && KeyLength != 256)) { + return FALSE; + } + + // + // Store AES Key and EVP Cipher inside the user-provided Context + // + Context->Key = Key; + + switch (KeyLength) { + case 128: + Context->EvpCipher = EVP_aes_128_cbc(); + break; + case 192: + Context->EvpCipher = EVP_aes_192_cbc(); + break; + case 256: + Context->EvpCipher = EVP_aes_256_cbc(); + break; + default: + return FALSE; + } + + return TRUE; +} + +/** + Performs AES encryption on a data buffer of the specified size in CBC mode. + + This function performs AES encryption on data buffer pointed by Input, of specified + size of InputSize, in CBC mode. + InputSize must be multiple of block size (16 bytes). This function does not perform + padding. Caller must perform padding, if necessary, to ensure valid input data size. + Initialization vector should be one block size (16 bytes). + AesContext should be already correctly initialized by AesInit(). Behavior with + invalid AES context is undefined. + + If AesContext is NULL, then return FALSE. + If Input is NULL, then return FALSE. + If InputSize is not multiple of block size (16 bytes), then return FALSE. + If Ivec is NULL, then return FALSE. + If Output is NULL, then return FALSE. + + @param[in] AesContext Pointer to the AES context. + @param[in] Input Pointer to the buffer containing the data to be encrypted. + @param[in] InputSize Size of the Input buffer in bytes. + @param[in] Ivec Pointer to initialization vector. + @param[out] Output Pointer to a buffer that receives the AES encryption output. + + @retval TRUE AES encryption succeeded. + @retval FALSE AES encryption failed. + +**/ +BOOLEAN +EFIAPI +AesCbcEncrypt ( + IN VOID *AesContext, + IN CONST UINT8 *Input, + IN UINTN InputSize, + IN CONST UINT8 *Ivec, + OUT UINT8 *Output + ) +{ + AES_CONTEXT *Context = AesContext; + EVP_CIPHER_CTX *EvpContext; + INT32 OutputSize; + INT32 TempSize; + + // + // Check input parameters. + // + if (AesContext == NULL || Input == NULL || (InputSize % AES_BLOCK_SIZE) != 0) { + return FALSE; + } + + if (Ivec == NULL || Output == NULL || InputSize > INT_MAX) { + return FALSE; + } + + EvpContext = EVP_CIPHER_CTX_new(); + if (EvpContext == NULL) { + return FALSE; + } + + // + // Perform AES data encryption with CBC mode + // + if (EVP_EncryptInit_ex(EvpContext, Context->EvpCipher, NULL, Context->Key, Ivec) != 1) { + EVP_CIPHER_CTX_free(EvpContext); + return FALSE; + } + + // + // Disable padding to match the software-based implementation + // + EVP_CIPHER_CTX_set_padding (EvpContext, 0); + + if (EVP_EncryptUpdate(EvpContext, Output, &OutputSize, Input, (INT32) InputSize) != 1) { + EVP_CIPHER_CTX_free(EvpContext); + return FALSE; + } + + if (EVP_EncryptFinal_ex(EvpContext, Output + OutputSize, &TempSize) != 1) { + EVP_CIPHER_CTX_free(EvpContext); + return FALSE; + } + + EVP_CIPHER_CTX_free(EvpContext); + return TRUE; +} + +/** + Performs AES decryption on a data buffer of the specified size in CBC mode. + + This function performs AES decryption on data buffer pointed by Input, of specified + size of InputSize, in CBC mode. + InputSize must be multiple of block size (16 bytes). This function does not perform + padding. Caller must perform padding, if necessary, to ensure valid input data size. + Initialization vector should be one block size (16 bytes). + AesContext should be already correctly initialized by AesInit(). Behavior with + invalid AES context is undefined. + + If AesContext is NULL, then return FALSE. + If Input is NULL, then return FALSE. + If InputSize is not multiple of block size (16 bytes), then return FALSE. + If Ivec is NULL, then return FALSE. + If Output is NULL, then return FALSE. + + @param[in] AesContext Pointer to the AES context. + @param[in] Input Pointer to the buffer containing the data to be encrypted. + @param[in] InputSize Size of the Input buffer in bytes. + @param[in] Ivec Pointer to initialization vector. + @param[out] Output Pointer to a buffer that receives the AES encryption output. + + @retval TRUE AES decryption succeeded. + @retval FALSE AES decryption failed. + +**/ +BOOLEAN +EFIAPI +AesCbcDecrypt ( + IN VOID *AesContext, + IN CONST UINT8 *Input, + IN UINTN InputSize, + IN CONST UINT8 *Ivec, + OUT UINT8 *Output + ) +{ + AES_CONTEXT *Context = AesContext; + EVP_CIPHER_CTX *EvpContext; + INT32 OutputSize; + INT32 TempSize; + + // + // Check input parameters. + // + if (AesContext == NULL || Input == NULL || (InputSize % AES_BLOCK_SIZE) != 0) { + return FALSE; + } + + if (Ivec == NULL || Output == NULL || InputSize > INT_MAX) { + return FALSE; + } + + EvpContext = EVP_CIPHER_CTX_new(); + if (EvpContext == NULL) { + return FALSE; + } + + // + // Perform AES data encryption with CBC mode + // + if (EVP_DecryptInit_ex(EvpContext, Context->EvpCipher, NULL, Context->Key, Ivec) != 1) { + EVP_CIPHER_CTX_free(EvpContext); + return FALSE; + } + + // + // Disable padding to match the software-based implementation + // + EVP_CIPHER_CTX_set_padding (EvpContext, 0); + + if (EVP_DecryptUpdate(EvpContext, Output, &OutputSize, Input, (INT32) InputSize) != 1) { + EVP_CIPHER_CTX_free(EvpContext); + return FALSE; + } + + if (EVP_DecryptFinal_ex(EvpContext, Output + OutputSize, &TempSize) != 1) { + EVP_CIPHER_CTX_free(EvpContext); + return FALSE; + } + + EVP_CIPHER_CTX_free(EvpContext); + return TRUE; +} -- 2.28.0.windows.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c 2020-10-28 18:42 [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c Zurcher, Christopher J 2020-10-28 18:42 ` [PATCH 1/1] " Zurcher, Christopher J @ 2020-10-31 1:10 ` Yao, Jiewen 2020-11-02 22:36 ` Zurcher, Christopher J 1 sibling, 1 reply; 10+ messages in thread From: Yao, Jiewen @ 2020-10-31 1:10 UTC (permalink / raw) To: Zurcher, Christopher J, devel@edk2.groups.io Cc: Wang, Jian J, Lu, XiaoyuX, Jiang, Guomin, Sung-Uk Bin, Ard Biesheuvel HI Zucher I am not sure why you only add a .c file, but do not add this c to any INF file. This seems a dummy C file. I recommend you drop this and create a full patch to add C and update INF file. Since you are talking performance and size, do you have any data? For example, how fast you have got? What is the size difference before and after? This can help other people make decision to choose which version. Thank you Yao Jiewen > -----Original Message----- > From: Christopher J Zurcher <christopher.j.zurcher@intel.com> > Sent: Thursday, October 29, 2020 2:43 AM > To: devel@edk2.groups.io > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J > <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin > <guomin.jiang@intel.com>; Sung-Uk Bin <sunguk-bin@hp.com>; Ard > Biesheuvel <ard.biesheuvel@arm.com> > Subject: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for > CryptAes.c > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2507 > > This patch provides a drop-in replacement for CryptAes.c which utilizes > the EVP interface to OpenSSL. This enables access to the assembly-optimized > algorithms. > > This patch has been unit-tested in both VS and CLANG build environments > using both an X64 assembly-optimized implementation of OpensslLib and > the > standard implementation. > > Usage of this file does not require an assembly-optimized implementation of > OpensslLib to function; it does however require one to provide a speed > improvement. > > The C-only AES implementation included by CryptAes.c is extremely small, > and since this file includes the EVP interface, it will significantly > increase the size of any module that includes it. As a result, I have not > replaced the original CryptAes.c as a default in any of the CryptLib > implementations. > > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com> > Cc: Guomin Jiang <guomin.jiang@intel.com> > Cc: Sung-Uk Bin <sunguk-bin@hp.com> > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com> > > Christopher J Zurcher (1): > CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c | 262 > ++++++++++++++++++++ > 1 file changed, 262 insertions(+) > create mode 100644 CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c > > -- > 2.28.0.windows.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c 2020-10-31 1:10 ` [PATCH 0/1] " Yao, Jiewen @ 2020-11-02 22:36 ` Zurcher, Christopher J 2020-11-03 1:13 ` Yao, Jiewen 2020-11-03 7:51 ` Ard Biesheuvel 0 siblings, 2 replies; 10+ messages in thread From: Zurcher, Christopher J @ 2020-11-02 22:36 UTC (permalink / raw) To: Yao, Jiewen, devel@edk2.groups.io Cc: Wang, Jian J, Lu, XiaoyuX, Jiang, Guomin, Sung-Uk Bin, Ard Biesheuvel The size increase from adding the EVP interface to a module that does not already include it (through the HMAC functions) is ~192KB. Intel documentation on the IA version of the feature lists speed improvement of ~32% to ~47% depending on the operation and key size. Other architectures may see different speed improvements. I have only tested this file with OvmfPkg through QEMU. I did not add this .c file to any INF file because it will add ~192KB to any module that includes AES functionality and it should be up to the end user if they want to include this file for the size tradeoff vs. the speed gain for their particular architecture. Additionally as the only native OpensslLib implementation available currently is for X64 architecture, any other architecture would have a size increase with no speed improvement. Thanks, Christopher Zurcher > -----Original Message----- > From: Yao, Jiewen <jiewen.yao@intel.com> > Sent: Friday, October 30, 2020 18:10 > To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>; > devel@edk2.groups.io > Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; > Jiang, Guomin <guomin.jiang@intel.com>; Sung-Uk Bin <sunguk-bin@hp.com>; Ard > Biesheuvel <ard.biesheuvel@arm.com> > Subject: RE: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for > CryptAes.c > > HI Zucher > I am not sure why you only add a .c file, but do not add this c to any INF > file. This seems a dummy C file. > I recommend you drop this and create a full patch to add C and update INF > file. > > Since you are talking performance and size, do you have any data? > For example, how fast you have got? What is the size difference before and > after? > This can help other people make decision to choose which version. > > > Thank you > Yao Jiewen > > > > -----Original Message----- > > From: Christopher J Zurcher <christopher.j.zurcher@intel.com> > > Sent: Thursday, October 29, 2020 2:43 AM > > To: devel@edk2.groups.io > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J > > <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin > > <guomin.jiang@intel.com>; Sung-Uk Bin <sunguk-bin@hp.com>; Ard > > Biesheuvel <ard.biesheuvel@arm.com> > > Subject: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for > > CryptAes.c > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2507 > > > > This patch provides a drop-in replacement for CryptAes.c which utilizes > > the EVP interface to OpenSSL. This enables access to the assembly-optimized > > algorithms. > > > > This patch has been unit-tested in both VS and CLANG build environments > > using both an X64 assembly-optimized implementation of OpensslLib and > > the > > standard implementation. > > > > Usage of this file does not require an assembly-optimized implementation of > > OpensslLib to function; it does however require one to provide a speed > > improvement. > > > > The C-only AES implementation included by CryptAes.c is extremely small, > > and since this file includes the EVP interface, it will significantly > > increase the size of any module that includes it. As a result, I have not > > replaced the original CryptAes.c as a default in any of the CryptLib > > implementations. > > > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > Cc: Jian J Wang <jian.j.wang@intel.com> > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com> > > Cc: Guomin Jiang <guomin.jiang@intel.com> > > Cc: Sung-Uk Bin <sunguk-bin@hp.com> > > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com> > > > > Christopher J Zurcher (1): > > CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c > > > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c | 262 > > ++++++++++++++++++++ > > 1 file changed, 262 insertions(+) > > create mode 100644 CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c > > > > -- > > 2.28.0.windows.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c 2020-11-02 22:36 ` Zurcher, Christopher J @ 2020-11-03 1:13 ` Yao, Jiewen 2020-11-03 1:22 ` Bret Barkelew 2020-11-03 7:51 ` Ard Biesheuvel 1 sibling, 1 reply; 10+ messages in thread From: Yao, Jiewen @ 2020-11-03 1:13 UTC (permalink / raw) To: Zurcher, Christopher J, devel@edk2.groups.io Cc: Wang, Jian J, Lu, XiaoyuX, Jiang, Guomin, Sung-Uk Bin, Ard Biesheuvel Thanks to provide the data. I have a little concern on what is the usage if we just add a standalone C file. The EDKII build does not support cross package file including in INF, and does not recommend cross module file including in INF. What is the expected usage for the standalone CryptAes.c ? If you have a private project which want to use this, then you have to write your own INF file. But if so, why not include this CryptAes.c along with the new INF? Thank you Yao Jiewen > -----Original Message----- > From: Zurcher, Christopher J <christopher.j.zurcher@intel.com> > Sent: Tuesday, November 3, 2020 6:37 AM > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io > Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX > <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>; Sung-Uk > Bin <sunguk-bin@hp.com>; Ard Biesheuvel <ard.biesheuvel@arm.com> > Subject: RE: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation > for CryptAes.c > > The size increase from adding the EVP interface to a module that does not > already include it (through the HMAC functions) is ~192KB. > Intel documentation on the IA version of the feature lists speed > improvement of ~32% to ~47% depending on the operation and key size. > Other architectures may see different speed improvements. I have only > tested this file with OvmfPkg through QEMU. > > I did not add this .c file to any INF file because it will add ~192KB to any > module that includes AES functionality and it should be up to the end user if > they want to include this file for the size tradeoff vs. the speed gain for their > particular architecture. Additionally as the only native OpensslLib > implementation available currently is for X64 architecture, any other > architecture would have a size increase with no speed improvement. > > Thanks, > Christopher Zurcher > > > -----Original Message----- > > From: Yao, Jiewen <jiewen.yao@intel.com> > > Sent: Friday, October 30, 2020 18:10 > > To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>; > > devel@edk2.groups.io > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX > <xiaoyux.lu@intel.com>; > > Jiang, Guomin <guomin.jiang@intel.com>; Sung-Uk Bin <sunguk- > bin@hp.com>; Ard > > Biesheuvel <ard.biesheuvel@arm.com> > > Subject: RE: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation > for > > CryptAes.c > > > > HI Zucher > > I am not sure why you only add a .c file, but do not add this c to any INF > > file. This seems a dummy C file. > > I recommend you drop this and create a full patch to add C and update INF > > file. > > > > Since you are talking performance and size, do you have any data? > > For example, how fast you have got? What is the size difference before and > > after? > > This can help other people make decision to choose which version. > > > > > > Thank you > > Yao Jiewen > > > > > > > -----Original Message----- > > > From: Christopher J Zurcher <christopher.j.zurcher@intel.com> > > > Sent: Thursday, October 29, 2020 2:43 AM > > > To: devel@edk2.groups.io > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J > > > <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, > Guomin > > > <guomin.jiang@intel.com>; Sung-Uk Bin <sunguk-bin@hp.com>; Ard > > > Biesheuvel <ard.biesheuvel@arm.com> > > > Subject: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation > for > > > CryptAes.c > > > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2507 > > > > > > This patch provides a drop-in replacement for CryptAes.c which utilizes > > > the EVP interface to OpenSSL. This enables access to the assembly- > optimized > > > algorithms. > > > > > > This patch has been unit-tested in both VS and CLANG build > environments > > > using both an X64 assembly-optimized implementation of OpensslLib and > > > the > > > standard implementation. > > > > > > Usage of this file does not require an assembly-optimized > implementation of > > > OpensslLib to function; it does however require one to provide a speed > > > improvement. > > > > > > The C-only AES implementation included by CryptAes.c is extremely small, > > > and since this file includes the EVP interface, it will significantly > > > increase the size of any module that includes it. As a result, I have not > > > replaced the original CryptAes.c as a default in any of the CryptLib > > > implementations. > > > > > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > > Cc: Jian J Wang <jian.j.wang@intel.com> > > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com> > > > Cc: Guomin Jiang <guomin.jiang@intel.com> > > > Cc: Sung-Uk Bin <sunguk-bin@hp.com> > > > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com> > > > > > > Christopher J Zurcher (1): > > > CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c > > > > > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c | 262 > > > ++++++++++++++++++++ > > > 1 file changed, 262 insertions(+) > > > create mode 100644 > CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c > > > > > > -- > > > 2.28.0.windows.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c 2020-11-03 1:13 ` Yao, Jiewen @ 2020-11-03 1:22 ` Bret Barkelew 2020-11-03 21:54 ` Zurcher, Christopher J 0 siblings, 1 reply; 10+ messages in thread From: Bret Barkelew @ 2020-11-03 1:22 UTC (permalink / raw) To: devel@edk2.groups.io, Yao, Jiewen, Zurcher, Christopher J Cc: Wang, Jian J, Lu, XiaoyuX, Jiang, Guomin, Sung-Uk Bin, Ard Biesheuvel [-- Attachment #1: Type: text/plain, Size: 6477 bytes --] I agree that this should not be a C file. Based on the description, would a FixedAtBuild PCD work to wrap the code? I don’t know that it warrants a full LibraryClass, but that’s another option. Have you looked at just implementing it in a flavor of the new EDK2 binary crypto payloads? Those are shared and wouldn’t incur the per-module cost. - Bret From: Yao, Jiewen via groups.io<mailto:jiewen.yao=intel.com@groups.io> Sent: Monday, November 2, 2020 5:13 PM To: Zurcher, Christopher J<mailto:christopher.j.zurcher@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> Cc: Wang, Jian J<mailto:jian.j.wang@intel.com>; Lu, XiaoyuX<mailto:xiaoyux.lu@intel.com>; Jiang, Guomin<mailto:guomin.jiang@intel.com>; Sung-Uk Bin<mailto:sunguk-bin@hp.com>; Ard Biesheuvel<mailto:ard.biesheuvel@arm.com> Subject: [EXTERNAL] Re: [edk2-devel] [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c Thanks to provide the data. I have a little concern on what is the usage if we just add a standalone C file. The EDKII build does not support cross package file including in INF, and does not recommend cross module file including in INF. What is the expected usage for the standalone CryptAes.c ? If you have a private project which want to use this, then you have to write your own INF file. But if so, why not include this CryptAes.c along with the new INF? Thank you Yao Jiewen > -----Original Message----- > From: Zurcher, Christopher J <christopher.j.zurcher@intel.com> > Sent: Tuesday, November 3, 2020 6:37 AM > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io > Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX > <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>; Sung-Uk > Bin <sunguk-bin@hp.com>; Ard Biesheuvel <ard.biesheuvel@arm.com> > Subject: RE: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation > for CryptAes.c > > The size increase from adding the EVP interface to a module that does not > already include it (through the HMAC functions) is ~192KB. > Intel documentation on the IA version of the feature lists speed > improvement of ~32% to ~47% depending on the operation and key size. > Other architectures may see different speed improvements. I have only > tested this file with OvmfPkg through QEMU. > > I did not add this .c file to any INF file because it will add ~192KB to any > module that includes AES functionality and it should be up to the end user if > they want to include this file for the size tradeoff vs. the speed gain for their > particular architecture. Additionally as the only native OpensslLib > implementation available currently is for X64 architecture, any other > architecture would have a size increase with no speed improvement. > > Thanks, > Christopher Zurcher > > > -----Original Message----- > > From: Yao, Jiewen <jiewen.yao@intel.com> > > Sent: Friday, October 30, 2020 18:10 > > To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>; > > devel@edk2.groups.io > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX > <xiaoyux.lu@intel.com>; > > Jiang, Guomin <guomin.jiang@intel.com>; Sung-Uk Bin <sunguk- > bin@hp.com>; Ard > > Biesheuvel <ard.biesheuvel@arm.com> > > Subject: RE: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation > for > > CryptAes.c > > > > HI Zucher > > I am not sure why you only add a .c file, but do not add this c to any INF > > file. This seems a dummy C file. > > I recommend you drop this and create a full patch to add C and update INF > > file. > > > > Since you are talking performance and size, do you have any data? > > For example, how fast you have got? What is the size difference before and > > after? > > This can help other people make decision to choose which version. > > > > > > Thank you > > Yao Jiewen > > > > > > > -----Original Message----- > > > From: Christopher J Zurcher <christopher.j.zurcher@intel.com> > > > Sent: Thursday, October 29, 2020 2:43 AM > > > To: devel@edk2.groups.io > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J > > > <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, > Guomin > > > <guomin.jiang@intel.com>; Sung-Uk Bin <sunguk-bin@hp.com>; Ard > > > Biesheuvel <ard.biesheuvel@arm.com> > > > Subject: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation > for > > > CryptAes.c > > > > > > BZ: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2507&data=04%7C01%7Cbret.barkelew%40microsoft.com%7C4a13842bdd6240d452ca08d87f95ba5f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637399628344133791%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=oOONHll9TASuW8eBAMvvLUNZhb9jOSRN18liIP7BHrk%3D&reserved=0 > > > > > > This patch provides a drop-in replacement for CryptAes.c which utilizes > > > the EVP interface to OpenSSL. This enables access to the assembly- > optimized > > > algorithms. > > > > > > This patch has been unit-tested in both VS and CLANG build > environments > > > using both an X64 assembly-optimized implementation of OpensslLib and > > > the > > > standard implementation. > > > > > > Usage of this file does not require an assembly-optimized > implementation of > > > OpensslLib to function; it does however require one to provide a speed > > > improvement. > > > > > > The C-only AES implementation included by CryptAes.c is extremely small, > > > and since this file includes the EVP interface, it will significantly > > > increase the size of any module that includes it. As a result, I have not > > > replaced the original CryptAes.c as a default in any of the CryptLib > > > implementations. > > > > > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > > Cc: Jian J Wang <jian.j.wang@intel.com> > > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com> > > > Cc: Guomin Jiang <guomin.jiang@intel.com> > > > Cc: Sung-Uk Bin <sunguk-bin@hp.com> > > > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com> > > > > > > Christopher J Zurcher (1): > > > CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c > > > > > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c | 262 > > > ++++++++++++++++++++ > > > 1 file changed, 262 insertions(+) > > > create mode 100644 > CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c > > > > > > -- > > > 2.28.0.windows.1 [-- Attachment #2: Type: text/html, Size: 10376 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c 2020-11-03 1:22 ` Bret Barkelew @ 2020-11-03 21:54 ` Zurcher, Christopher J 2020-11-03 23:03 ` Yao, Jiewen 0 siblings, 1 reply; 10+ messages in thread From: Zurcher, Christopher J @ 2020-11-03 21:54 UTC (permalink / raw) To: Bret Barkelew, devel@edk2.groups.io, Yao, Jiewen Cc: Wang, Jian J, Lu, XiaoyuX, Jiang, Guomin, Sung-Uk Bin, Ard Biesheuvel [-- Attachment #1: Type: text/plain, Size: 7963 bytes --] I'm OK with using a PCD here if there are no other objections. -- Christopher Zurcher From: Bret Barkelew <Bret.Barkelew@microsoft.com> Sent: Monday, November 2, 2020 17:23 To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Zurcher, Christopher J <christopher.j.zurcher@intel.com> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>; Sung-Uk Bin <sunguk-bin@hp.com>; Ard Biesheuvel <ard.biesheuvel@arm.com> Subject: RE: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c I agree that this should not be a C file. Based on the description, would a FixedAtBuild PCD work to wrap the code? I don't know that it warrants a full LibraryClass, but that's another option. Have you looked at just implementing it in a flavor of the new EDK2 binary crypto payloads? Those are shared and wouldn't incur the per-module cost. - Bret From: Yao, Jiewen via groups.io<mailto:jiewen.yao=intel.com@groups.io> Sent: Monday, November 2, 2020 5:13 PM To: Zurcher, Christopher J<mailto:christopher.j.zurcher@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> Cc: Wang, Jian J<mailto:jian.j.wang@intel.com>; Lu, XiaoyuX<mailto:xiaoyux.lu@intel.com>; Jiang, Guomin<mailto:guomin.jiang@intel.com>; Sung-Uk Bin<mailto:sunguk-bin@hp.com>; Ard Biesheuvel<mailto:ard.biesheuvel@arm.com> Subject: [EXTERNAL] Re: [edk2-devel] [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c Thanks to provide the data. I have a little concern on what is the usage if we just add a standalone C file. The EDKII build does not support cross package file including in INF, and does not recommend cross module file including in INF. What is the expected usage for the standalone CryptAes.c ? If you have a private project which want to use this, then you have to write your own INF file. But if so, why not include this CryptAes.c along with the new INF? Thank you Yao Jiewen > -----Original Message----- > From: Zurcher, Christopher J <christopher.j.zurcher@intel.com<mailto:christopher.j.zurcher@intel.com>> > Sent: Tuesday, November 3, 2020 6:37 AM > To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Lu, XiaoyuX > <xiaoyux.lu@intel.com<mailto:xiaoyux.lu@intel.com>>; Jiang, Guomin <guomin.jiang@intel.com<mailto:guomin.jiang@intel.com>>; Sung-Uk > Bin <sunguk-bin@hp.com<mailto:sunguk-bin@hp.com>>; Ard Biesheuvel <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>> > Subject: RE: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation > for CryptAes.c > > The size increase from adding the EVP interface to a module that does not > already include it (through the HMAC functions) is ~192KB. > Intel documentation on the IA version of the feature lists speed > improvement of ~32% to ~47% depending on the operation and key size. > Other architectures may see different speed improvements. I have only > tested this file with OvmfPkg through QEMU. > > I did not add this .c file to any INF file because it will add ~192KB to any > module that includes AES functionality and it should be up to the end user if > they want to include this file for the size tradeoff vs. the speed gain for their > particular architecture. Additionally as the only native OpensslLib > implementation available currently is for X64 architecture, any other > architecture would have a size increase with no speed improvement. > > Thanks, > Christopher Zurcher > > > -----Original Message----- > > From: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> > > Sent: Friday, October 30, 2020 18:10 > > To: Zurcher, Christopher J <christopher.j.zurcher@intel.com<mailto:christopher.j.zurcher@intel.com>>; > > devel@edk2.groups.io<mailto:devel@edk2.groups.io> > > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Lu, XiaoyuX > <xiaoyux.lu@intel.com<mailto:xiaoyux.lu@intel.com>>; > > Jiang, Guomin <guomin.jiang@intel.com<mailto:guomin.jiang@intel.com>>; Sung-Uk Bin <sunguk- > bin@hp.com<mailto:bin@hp.com>>; Ard > > Biesheuvel <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>> > > Subject: RE: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation > for > > CryptAes.c > > > > HI Zucher > > I am not sure why you only add a .c file, but do not add this c to any INF > > file. This seems a dummy C file. > > I recommend you drop this and create a full patch to add C and update INF > > file. > > > > Since you are talking performance and size, do you have any data? > > For example, how fast you have got? What is the size difference before and > > after? > > This can help other people make decision to choose which version. > > > > > > Thank you > > Yao Jiewen > > > > > > > -----Original Message----- > > > From: Christopher J Zurcher <christopher.j.zurcher@intel.com<mailto:christopher.j.zurcher@intel.com>> > > > Sent: Thursday, October 29, 2020 2:43 AM > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> > > > Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wang, Jian J > > > <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Lu, XiaoyuX <xiaoyux.lu@intel.com<mailto:xiaoyux.lu@intel.com>>; Jiang, > Guomin > > > <guomin.jiang@intel.com<mailto:guomin.jiang@intel.com>>; Sung-Uk Bin <sunguk-bin@hp.com<mailto:sunguk-bin@hp.com>>; Ard > > > Biesheuvel <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>> > > > Subject: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation > for > > > CryptAes.c > > > > > > BZ: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2507&data=04%7C01%7Cbret.barkelew%40microsoft.com%7C4a13842bdd6240d452ca08d87f95ba5f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637399628344133791%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=oOONHll9TASuW8eBAMvvLUNZhb9jOSRN18liIP7BHrk%3D&reserved=0 > > > > > > This patch provides a drop-in replacement for CryptAes.c which utilizes > > > the EVP interface to OpenSSL. This enables access to the assembly- > optimized > > > algorithms. > > > > > > This patch has been unit-tested in both VS and CLANG build > environments > > > using both an X64 assembly-optimized implementation of OpensslLib and > > > the > > > standard implementation. > > > > > > Usage of this file does not require an assembly-optimized > implementation of > > > OpensslLib to function; it does however require one to provide a speed > > > improvement. > > > > > > The C-only AES implementation included by CryptAes.c is extremely small, > > > and since this file includes the EVP interface, it will significantly > > > increase the size of any module that includes it. As a result, I have not > > > replaced the original CryptAes.c as a default in any of the CryptLib > > > implementations. > > > > > > Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> > > > Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> > > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com<mailto:xiaoyux.lu@intel.com>> > > > Cc: Guomin Jiang <guomin.jiang@intel.com<mailto:guomin.jiang@intel.com>> > > > Cc: Sung-Uk Bin <sunguk-bin@hp.com<mailto:sunguk-bin@hp.com>> > > > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>> > > > > > > Christopher J Zurcher (1): > > > CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c > > > > > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c | 262 > > > ++++++++++++++++++++ > > > 1 file changed, 262 insertions(+) > > > create mode 100644 > CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c > > > > > > -- > > > 2.28.0.windows.1 [-- Attachment #2: Type: text/html, Size: 13210 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c 2020-11-03 21:54 ` Zurcher, Christopher J @ 2020-11-03 23:03 ` Yao, Jiewen 0 siblings, 0 replies; 10+ messages in thread From: Yao, Jiewen @ 2020-11-03 23:03 UTC (permalink / raw) To: Zurcher, Christopher J, Bret Barkelew, devel@edk2.groups.io Cc: Wang, Jian J, Lu, XiaoyuX, Jiang, Guomin, Sung-Uk Bin, Ard Biesheuvel [-- Attachment #1: Type: text/plain, Size: 8908 bytes --] Yes. I think PCD is a better option than adding another INF. Please add the comment as much as possible, to describe the PROs and CONs, such as size/performance. From: Zurcher, Christopher J <christopher.j.zurcher@intel.com> Sent: Wednesday, November 4, 2020 5:55 AM To: Bret Barkelew <Bret.Barkelew@microsoft.com>; devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>; Sung-Uk Bin <sunguk-bin@hp.com>; Ard Biesheuvel <ard.biesheuvel@arm.com> Subject: RE: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c I'm OK with using a PCD here if there are no other objections. -- Christopher Zurcher From: Bret Barkelew <Bret.Barkelew@microsoft.com<mailto:Bret.Barkelew@microsoft.com>> Sent: Monday, November 2, 2020 17:23 To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Zurcher, Christopher J <christopher.j.zurcher@intel.com<mailto:christopher.j.zurcher@intel.com>> Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Lu, XiaoyuX <xiaoyux.lu@intel.com<mailto:xiaoyux.lu@intel.com>>; Jiang, Guomin <guomin.jiang@intel.com<mailto:guomin.jiang@intel.com>>; Sung-Uk Bin <sunguk-bin@hp.com<mailto:sunguk-bin@hp.com>>; Ard Biesheuvel <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>> Subject: RE: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c I agree that this should not be a C file. Based on the description, would a FixedAtBuild PCD work to wrap the code? I don't know that it warrants a full LibraryClass, but that's another option. Have you looked at just implementing it in a flavor of the new EDK2 binary crypto payloads? Those are shared and wouldn't incur the per-module cost. - Bret From: Yao, Jiewen via groups.io<mailto:jiewen.yao=intel.com@groups.io> Sent: Monday, November 2, 2020 5:13 PM To: Zurcher, Christopher J<mailto:christopher.j.zurcher@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> Cc: Wang, Jian J<mailto:jian.j.wang@intel.com>; Lu, XiaoyuX<mailto:xiaoyux.lu@intel.com>; Jiang, Guomin<mailto:guomin.jiang@intel.com>; Sung-Uk Bin<mailto:sunguk-bin@hp.com>; Ard Biesheuvel<mailto:ard.biesheuvel@arm.com> Subject: [EXTERNAL] Re: [edk2-devel] [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c Thanks to provide the data. I have a little concern on what is the usage if we just add a standalone C file. The EDKII build does not support cross package file including in INF, and does not recommend cross module file including in INF. What is the expected usage for the standalone CryptAes.c ? If you have a private project which want to use this, then you have to write your own INF file. But if so, why not include this CryptAes.c along with the new INF? Thank you Yao Jiewen > -----Original Message----- > From: Zurcher, Christopher J <christopher.j.zurcher@intel.com<mailto:christopher.j.zurcher@intel.com>> > Sent: Tuesday, November 3, 2020 6:37 AM > To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Lu, XiaoyuX > <xiaoyux.lu@intel.com<mailto:xiaoyux.lu@intel.com>>; Jiang, Guomin <guomin.jiang@intel.com<mailto:guomin.jiang@intel.com>>; Sung-Uk > Bin <sunguk-bin@hp.com<mailto:sunguk-bin@hp.com>>; Ard Biesheuvel <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>> > Subject: RE: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation > for CryptAes.c > > The size increase from adding the EVP interface to a module that does not > already include it (through the HMAC functions) is ~192KB. > Intel documentation on the IA version of the feature lists speed > improvement of ~32% to ~47% depending on the operation and key size. > Other architectures may see different speed improvements. I have only > tested this file with OvmfPkg through QEMU. > > I did not add this .c file to any INF file because it will add ~192KB to any > module that includes AES functionality and it should be up to the end user if > they want to include this file for the size tradeoff vs. the speed gain for their > particular architecture. Additionally as the only native OpensslLib > implementation available currently is for X64 architecture, any other > architecture would have a size increase with no speed improvement. > > Thanks, > Christopher Zurcher > > > -----Original Message----- > > From: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> > > Sent: Friday, October 30, 2020 18:10 > > To: Zurcher, Christopher J <christopher.j.zurcher@intel.com<mailto:christopher.j.zurcher@intel.com>>; > > devel@edk2.groups.io<mailto:devel@edk2.groups.io> > > Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Lu, XiaoyuX > <xiaoyux.lu@intel.com<mailto:xiaoyux.lu@intel.com>>; > > Jiang, Guomin <guomin.jiang@intel.com<mailto:guomin.jiang@intel.com>>; Sung-Uk Bin <sunguk- > bin@hp.com<mailto:bin@hp.com>>; Ard > > Biesheuvel <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>> > > Subject: RE: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation > for > > CryptAes.c > > > > HI Zucher > > I am not sure why you only add a .c file, but do not add this c to any INF > > file. This seems a dummy C file. > > I recommend you drop this and create a full patch to add C and update INF > > file. > > > > Since you are talking performance and size, do you have any data? > > For example, how fast you have got? What is the size difference before and > > after? > > This can help other people make decision to choose which version. > > > > > > Thank you > > Yao Jiewen > > > > > > > -----Original Message----- > > > From: Christopher J Zurcher <christopher.j.zurcher@intel.com<mailto:christopher.j.zurcher@intel.com>> > > > Sent: Thursday, October 29, 2020 2:43 AM > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> > > > Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wang, Jian J > > > <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Lu, XiaoyuX <xiaoyux.lu@intel.com<mailto:xiaoyux.lu@intel.com>>; Jiang, > Guomin > > > <guomin.jiang@intel.com<mailto:guomin.jiang@intel.com>>; Sung-Uk Bin <sunguk-bin@hp.com<mailto:sunguk-bin@hp.com>>; Ard > > > Biesheuvel <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>> > > > Subject: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation > for > > > CryptAes.c > > > > > > BZ: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2507&data=04%7C01%7Cbret.barkelew%40microsoft.com%7C4a13842bdd6240d452ca08d87f95ba5f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637399628344133791%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=oOONHll9TASuW8eBAMvvLUNZhb9jOSRN18liIP7BHrk%3D&reserved=0 > > > > > > This patch provides a drop-in replacement for CryptAes.c which utilizes > > > the EVP interface to OpenSSL. This enables access to the assembly- > optimized > > > algorithms. > > > > > > This patch has been unit-tested in both VS and CLANG build > environments > > > using both an X64 assembly-optimized implementation of OpensslLib and > > > the > > > standard implementation. > > > > > > Usage of this file does not require an assembly-optimized > implementation of > > > OpensslLib to function; it does however require one to provide a speed > > > improvement. > > > > > > The C-only AES implementation included by CryptAes.c is extremely small, > > > and since this file includes the EVP interface, it will significantly > > > increase the size of any module that includes it. As a result, I have not > > > replaced the original CryptAes.c as a default in any of the CryptLib > > > implementations. > > > > > > Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> > > > Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> > > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com<mailto:xiaoyux.lu@intel.com>> > > > Cc: Guomin Jiang <guomin.jiang@intel.com<mailto:guomin.jiang@intel.com>> > > > Cc: Sung-Uk Bin <sunguk-bin@hp.com<mailto:sunguk-bin@hp.com>> > > > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>> > > > > > > Christopher J Zurcher (1): > > > CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c > > > > > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c | 262 > > > ++++++++++++++++++++ > > > 1 file changed, 262 insertions(+) > > > create mode 100644 > CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c > > > > > > -- > > > 2.28.0.windows.1 [-- Attachment #2: Type: text/html, Size: 14810 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c 2020-11-02 22:36 ` Zurcher, Christopher J 2020-11-03 1:13 ` Yao, Jiewen @ 2020-11-03 7:51 ` Ard Biesheuvel 2020-11-03 19:15 ` Zurcher, Christopher J 1 sibling, 1 reply; 10+ messages in thread From: Ard Biesheuvel @ 2020-11-03 7:51 UTC (permalink / raw) To: Zurcher, Christopher J, Yao, Jiewen, devel@edk2.groups.io Cc: Wang, Jian J, Lu, XiaoyuX, Jiang, Guomin, Sung-Uk Bin, Laszlo Ersek On 11/2/20 11:36 PM, Zurcher, Christopher J wrote: > The size increase from adding the EVP interface to a module that does not already include it (through the HMAC functions) is ~192KB. Which HMAC function do we use in UEFI that needs AES? Adding 192 KB for AES to each module that only uses HMAC-SHAxxx seems like a really bad idea to me. Maybe the IEEE 802.11 drivers have some dependencies on CBC-MAC for CCMP, but I don't think any wifi hardware exists today that relies on software support for this, so using optimized code here makes little sense. Also, how does the 32% to 47% speed improvement translate to actual boot speed improvements? A lot of the crypto is only applied to small quantities of data, and only the algorithms that are applied to arbitrary size chunks should be optimized in this way, imo. Note that, while widely regarded as the fastest, the OpenSSL asm implementations are not as robust as you might think, and they don't see a lot of test coverage running in a bare metal context with elevated privileges like we do in EDK2. (I may have brought up some of these points before - apologies if anything I say sounds redundant). > Intel documentation on the IA version of the feature lists speed improvement of ~32% to ~47% depending on the operation and key size. Other architectures may see different speed improvements. I have only tested this file with OvmfPkg through QEMU. > > I did not add this .c file to any INF file because it will add ~192KB to any module that includes AES functionality and it should be up to the end user if they want to include this file for the size tradeoff vs. the speed gain for their particular architecture. Additionally as the only native OpensslLib implementation available currently is for X64 architecture, any other architecture would have a size increase with no speed improvement. > > Thanks, > Christopher Zurcher > >> -----Original Message----- >> From: Yao, Jiewen <jiewen.yao@intel.com> >> Sent: Friday, October 30, 2020 18:10 >> To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>; >> devel@edk2.groups.io >> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; >> Jiang, Guomin <guomin.jiang@intel.com>; Sung-Uk Bin <sunguk-bin@hp.com>; Ard >> Biesheuvel <ard.biesheuvel@arm.com> >> Subject: RE: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for >> CryptAes.c >> >> HI Zucher >> I am not sure why you only add a .c file, but do not add this c to any INF >> file. This seems a dummy C file. >> I recommend you drop this and create a full patch to add C and update INF >> file. >> >> Since you are talking performance and size, do you have any data? >> For example, how fast you have got? What is the size difference before and >> after? >> This can help other people make decision to choose which version. >> >> >> Thank you >> Yao Jiewen >> >> >>> -----Original Message----- >>> From: Christopher J Zurcher <christopher.j.zurcher@intel.com> >>> Sent: Thursday, October 29, 2020 2:43 AM >>> To: devel@edk2.groups.io >>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J >>> <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin >>> <guomin.jiang@intel.com>; Sung-Uk Bin <sunguk-bin@hp.com>; Ard >>> Biesheuvel <ard.biesheuvel@arm.com> >>> Subject: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for >>> CryptAes.c >>> >>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2507 >>> >>> This patch provides a drop-in replacement for CryptAes.c which utilizes >>> the EVP interface to OpenSSL. This enables access to the assembly-optimized >>> algorithms. >>> >>> This patch has been unit-tested in both VS and CLANG build environments >>> using both an X64 assembly-optimized implementation of OpensslLib and >>> the >>> standard implementation. >>> >>> Usage of this file does not require an assembly-optimized implementation of >>> OpensslLib to function; it does however require one to provide a speed >>> improvement. >>> >>> The C-only AES implementation included by CryptAes.c is extremely small, >>> and since this file includes the EVP interface, it will significantly >>> increase the size of any module that includes it. As a result, I have not >>> replaced the original CryptAes.c as a default in any of the CryptLib >>> implementations. >>> >>> Cc: Jiewen Yao <jiewen.yao@intel.com> >>> Cc: Jian J Wang <jian.j.wang@intel.com> >>> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com> >>> Cc: Guomin Jiang <guomin.jiang@intel.com> >>> Cc: Sung-Uk Bin <sunguk-bin@hp.com> >>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com> >>> >>> Christopher J Zurcher (1): >>> CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c >>> >>> CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c | 262 >>> ++++++++++++++++++++ >>> 1 file changed, 262 insertions(+) >>> create mode 100644 CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c >>> >>> -- >>> 2.28.0.windows.1 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c 2020-11-03 7:51 ` Ard Biesheuvel @ 2020-11-03 19:15 ` Zurcher, Christopher J 0 siblings, 0 replies; 10+ messages in thread From: Zurcher, Christopher J @ 2020-11-03 19:15 UTC (permalink / raw) To: Ard Biesheuvel, Yao, Jiewen, devel@edk2.groups.io Cc: Wang, Jian J, Lu, XiaoyuX, Jiang, Guomin, Sung-Uk Bin, Laszlo Ersek The HMAC functions do not need AES; my point was that the HMAC functions as we have them today are already a wrapper for the EVP interface (this is a function of OpenSSL that we cannot change). So if a module already includes the additional ~192KB EVP interface through the HMAC functions, it would not see any size hit by also including the updated AES interface. The speed improvement for the AES functions are not intended to improve boot speed for most standard platforms. In fact most platforms today aren't even calling any AES functions. The only reason for this patch is to satisfy BZ 2507, which was filed by Eugene at HP (and now owned by Bin) for what I assume is a platform-specific need for improved AES speed. I was already working on my other patch to enable the architecture-specific algorithms for SHA speed improvement (required for a Windows provisioning feature) and that patch was also satisfying most of the needs of BZ 2507, with the exception of the file in this patch which provides the path for AES to access the architecture-specific algorithms. Thanks, Christopher Zurcher > -----Original Message----- > From: Ard Biesheuvel <ard.biesheuvel@arm.com> > Sent: Monday, November 2, 2020 23:51 > To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>; Yao, Jiewen > <jiewen.yao@intel.com>; devel@edk2.groups.io > Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; > Jiang, Guomin <guomin.jiang@intel.com>; Sung-Uk Bin <sunguk-bin@hp.com>; > Laszlo Ersek <lersek@redhat.com> > Subject: Re: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for > CryptAes.c > > On 11/2/20 11:36 PM, Zurcher, Christopher J wrote: > > The size increase from adding the EVP interface to a module that does not > already include it (through the HMAC functions) is ~192KB. > > Which HMAC function do we use in UEFI that needs AES? Adding 192 KB for > AES to each module that only uses HMAC-SHAxxx seems like a really bad > idea to me. Maybe the IEEE 802.11 drivers have some dependencies on > CBC-MAC for CCMP, but I don't think any wifi hardware exists today that > relies on software support for this, so using optimized code here makes > little sense. > > Also, how does the 32% to 47% speed improvement translate to actual boot > speed improvements? A lot of the crypto is only applied to small > quantities of data, and only the algorithms that are applied to > arbitrary size chunks should be optimized in this way, imo. > > Note that, while widely regarded as the fastest, the OpenSSL asm > implementations are not as robust as you might think, and they don't see > a lot of test coverage running in a bare metal context with elevated > privileges like we do in EDK2. > > (I may have brought up some of these points before - apologies if > anything I say sounds redundant). > > > > > Intel documentation on the IA version of the feature lists speed > improvement of ~32% to ~47% depending on the operation and key size. Other > architectures may see different speed improvements. I have only tested this > file with OvmfPkg through QEMU. > > > > I did not add this .c file to any INF file because it will add ~192KB to > any module that includes AES functionality and it should be up to the end > user if they want to include this file for the size tradeoff vs. the speed > gain for their particular architecture. Additionally as the only native > OpensslLib implementation available currently is for X64 architecture, any > other architecture would have a size increase with no speed improvement. > > > > Thanks, > > Christopher Zurcher > > > >> -----Original Message----- > >> From: Yao, Jiewen <jiewen.yao@intel.com> > >> Sent: Friday, October 30, 2020 18:10 > >> To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>; > >> devel@edk2.groups.io > >> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX > <xiaoyux.lu@intel.com>; > >> Jiang, Guomin <guomin.jiang@intel.com>; Sung-Uk Bin <sunguk-bin@hp.com>; > Ard > >> Biesheuvel <ard.biesheuvel@arm.com> > >> Subject: RE: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation > for > >> CryptAes.c > >> > >> HI Zucher > >> I am not sure why you only add a .c file, but do not add this c to any INF > >> file. This seems a dummy C file. > >> I recommend you drop this and create a full patch to add C and update INF > >> file. > >> > >> Since you are talking performance and size, do you have any data? > >> For example, how fast you have got? What is the size difference before and > >> after? > >> This can help other people make decision to choose which version. > >> > >> > >> Thank you > >> Yao Jiewen > >> > >> > >>> -----Original Message----- > >>> From: Christopher J Zurcher <christopher.j.zurcher@intel.com> > >>> Sent: Thursday, October 29, 2020 2:43 AM > >>> To: devel@edk2.groups.io > >>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J > >>> <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, > Guomin > >>> <guomin.jiang@intel.com>; Sung-Uk Bin <sunguk-bin@hp.com>; Ard > >>> Biesheuvel <ard.biesheuvel@arm.com> > >>> Subject: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for > >>> CryptAes.c > >>> > >>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2507 > >>> > >>> This patch provides a drop-in replacement for CryptAes.c which utilizes > >>> the EVP interface to OpenSSL. This enables access to the assembly- > optimized > >>> algorithms. > >>> > >>> This patch has been unit-tested in both VS and CLANG build environments > >>> using both an X64 assembly-optimized implementation of OpensslLib and > >>> the > >>> standard implementation. > >>> > >>> Usage of this file does not require an assembly-optimized implementation > of > >>> OpensslLib to function; it does however require one to provide a speed > >>> improvement. > >>> > >>> The C-only AES implementation included by CryptAes.c is extremely small, > >>> and since this file includes the EVP interface, it will significantly > >>> increase the size of any module that includes it. As a result, I have not > >>> replaced the original CryptAes.c as a default in any of the CryptLib > >>> implementations. > >>> > >>> Cc: Jiewen Yao <jiewen.yao@intel.com> > >>> Cc: Jian J Wang <jian.j.wang@intel.com> > >>> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com> > >>> Cc: Guomin Jiang <guomin.jiang@intel.com> > >>> Cc: Sung-Uk Bin <sunguk-bin@hp.com> > >>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com> > >>> > >>> Christopher J Zurcher (1): > >>> CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c > >>> > >>> CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c | 262 > >>> ++++++++++++++++++++ > >>> 1 file changed, 262 insertions(+) > >>> create mode 100644 CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c > >>> > >>> -- > >>> 2.28.0.windows.1 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-11-03 23:03 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-10-28 18:42 [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c Zurcher, Christopher J 2020-10-28 18:42 ` [PATCH 1/1] " Zurcher, Christopher J 2020-10-31 1:10 ` [PATCH 0/1] " Yao, Jiewen 2020-11-02 22:36 ` Zurcher, Christopher J 2020-11-03 1:13 ` Yao, Jiewen 2020-11-03 1:22 ` Bret Barkelew 2020-11-03 21:54 ` Zurcher, Christopher J 2020-11-03 23:03 ` Yao, Jiewen 2020-11-03 7:51 ` Ard Biesheuvel 2020-11-03 19:15 ` Zurcher, Christopher J
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox