Re: [patch V2] MdeModulePkg/ReportStatusCodeLib: Avoid using AllocatePool if possible

["Gao, Liming" <liming.gao@intel.com>]

Star:
  This patch is from MS Project MU. So, the original author is kept. Dandan contributes this patch to edk2. So, Dandan is as Signed-off-by. This is the expected way. Below is author and signed-off-by definition. 

The author is the person who originally wrote the work, whereas the committer is the person who last applied the work. (from https://git-scm.com/book/en/v2/Git-Basics-Viewing-the-Commit-History)
Signed-off-by: tag indicates that the signer was involved in the development of the patch, or that he/she was in the patch's delivery path. (from https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst)

Thanks
Liming
> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, February 15, 2019 5:25 PM
> To: Bi, Dandan <dandan.bi@intel.com>; edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Michael Turner <Michael.Turner@microsoft.com>; Max Knutsen <maknutse@microsoft.com>; Gao,
> Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [patch V2] MdeModulePkg/ReportStatusCodeLib: Avoid using AllocatePool if possible
> 
> After applying the patch, the author is Max and the Signed-off-by is Dandan, is it expected?
> 
> And the code below is better to be into the else condition (stack buffer is not enough), right?
> 
>   if (gBS == NULL || gBS->AllocatePool == NULL || gBS->FreePool == NULL) {
>     return EFI_UNSUPPORTED;
>   }
> 
>   //
>   // Retrieve the current TPL
>   //
>   Tpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);
>   gBS->RestoreTPL (Tpl);
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Dandan Bi
> Sent: Thursday, February 14, 2019 10:39 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Michael Turner <Michael.Turner@microsoft.com>; Max Knutsen <maknutse@microsoft.com>; Gao,
> Liming <liming.gao@intel.com>
> Subject: [edk2] [patch V2] MdeModulePkg/ReportStatusCodeLib: Avoid using AllocatePool if possible
> 
> From: Max Knutsen <maknutse@microsoft.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1114
> 
> V2:  simplify the code logic.
> update
> if (!mHaveExitedBootServices &&
>   (StatusCodeData != (EFI_STATUS_CODE_DATA *) StatusCodeBuffer)) {
>   gBS->FreePool (StatusCodeData);
> }
> to
> if (StatusCodeData != (EFI_STATUS_CODE_DATA *) StatusCodeBuffer) {
>   gBS->FreePool (StatusCodeData);
> }
> 
> When report status code with ExtendedData data, and the extended data can fit in the local static buffer, there is no need to use
> AllocatePool to hold the ExtendedData data.
> 
> This patch is just to do the enhancement to avoid using AllocatePool.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Michael Turner <Michael.Turner@microsoft.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> ---
>  .../Library/DxeReportStatusCodeLib/ReportStatusCodeLib.c | 9 +++++++--  .../RuntimeDxeReportStatusCodeLib/ReportStatusCodeLib.c  |
> 9 +++++++--
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/DxeReportStatusCodeLib/ReportStatusCodeLib.c
> b/MdeModulePkg/Library/DxeReportStatusCodeLib/ReportStatusCodeLib.c
> index b28dc5c3bb..c88be5a1a3 100644
> --- a/MdeModulePkg/Library/DxeReportStatusCodeLib/ReportStatusCodeLib.c
> +++ b/MdeModulePkg/Library/DxeReportStatusCodeLib/ReportStatusCodeLib.c
> @@ -505,13 +505,18 @@ ReportStatusCodeEx (
>    //
>    Tpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);
>    gBS->RestoreTPL (Tpl);
> 
>    StatusCodeData = NULL;
> -  if (Tpl <= TPL_NOTIFY) {
> +  if (ExtendedDataSize <= (MAX_EXTENDED_DATA_SIZE - sizeof
> + (EFI_STATUS_CODE_DATA))) {
>      //
> -    // Allocate space for the Status Code Header and its buffer
> +    // Use Buffer instead of allocating if possible.
> +    //
> +    StatusCodeData = (EFI_STATUS_CODE_DATA *)Buffer;  } else if (Tpl <=
> + TPL_NOTIFY){
> +    //
> +    // If Buffer is not big enough, allocate space for the Status Code
> + Header and its buffer
>      //
>      gBS->AllocatePool (EfiBootServicesData, sizeof (EFI_STATUS_CODE_DATA) + ExtendedDataSize, (VOID **)&StatusCodeData);
>    }
> 
>    if (StatusCodeData == NULL) {
> diff --git a/MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/ReportStatusCodeLib.c
> b/MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/ReportStatusCodeLib.c
> index b73103517a..75e2f88ad2 100644
> --- a/MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/ReportStatusCodeLib.c
> +++ b/MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/ReportStatusCod
> +++ eLib.c
> @@ -635,11 +635,14 @@ ReportStatusCodeEx (
>    if (mHaveExitedBootServices) {
>      if (sizeof (EFI_STATUS_CODE_DATA) + ExtendedDataSize > MAX_EXTENDED_DATA_SIZE) {
>        return EFI_OUT_OF_RESOURCES;
>      }
>      StatusCodeData = (EFI_STATUS_CODE_DATA *) StatusCodeBuffer;
> -  } else  {
> +  } else if (sizeof (EFI_STATUS_CODE_DATA) + ExtendedDataSize > MAX_EXTENDED_DATA_SIZE) {
> +    //
> +    // Only use AllocatePool for larger ExtendedData.
> +    //
>      if (gBS == NULL || gBS->AllocatePool == NULL || gBS->FreePool == NULL) {
>        return EFI_UNSUPPORTED;
>      }
> 
>      //
> @@ -648,10 +651,12 @@ ReportStatusCodeEx (
>      StatusCodeData = NULL;
>      gBS->AllocatePool (EfiBootServicesData, sizeof (EFI_STATUS_CODE_DATA) + ExtendedDataSize, (VOID **)&StatusCodeData);
>      if (StatusCodeData == NULL) {
>        return EFI_OUT_OF_RESOURCES;
>      }
> +  } else {
> +    StatusCodeData = (EFI_STATUS_CODE_DATA *) StatusCodeBuffer;
>    }
> 
>    //
>    // Fill in the extended data header
>    //
> @@ -678,11 +683,11 @@ ReportStatusCodeEx (
>    Status = InternalReportStatusCode (Type, Value, Instance, CallerId, StatusCodeData);
> 
>    //
>    // Free the allocated buffer
>    //
> -  if (!mHaveExitedBootServices) {
> +  if (StatusCodeData != (EFI_STATUS_CODE_DATA *) StatusCodeBuffer) {
>      gBS->FreePool (StatusCodeData);
>    }
> 
>    return Status;
>  }
> --
> 2.18.0.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel