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

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

Reviewed-by: Liming Gao <liming.gao@intel.com>

> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, February 18, 2019 10:22 PM
> To: Bi, Dandan <dandan.bi@intel.com>; edk2-devel@lists.01.org
> Cc: Max Knutsen <maknutse@microsoft.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Michael Turner
> <Michael.Turner@microsoft.com>; Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [patch V4] MdeModulePkg/ReportStatusCodeLib: Avoid using AllocatePool if possible
> 
> Reviewed-by: Star Zeng <star.zeng@intel.com>.
> 
> -----Original Message-----
> From: Bi, Dandan
> Sent: Monday, February 18, 2019 9:37 PM
> To: edk2-devel@lists.01.org
> Cc: Max Knutsen <maknutse@microsoft.com>; Zeng, Star <star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Michael Turner <Michael.Turner@microsoft.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [patch V4] 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);
> }
> 
> V3:
> And the code below into the else condition (stack buffer is not enough) in /DxeReportStatusCodeLib/ReportStatusCodeLib.c
> 
>   if (gBS == NULL || gBS->AllocatePool == NULL || gBS->FreePool == NULL) {
>     return EFI_UNSUPPORTED;
>   }
> 
> V4:
> Refine code logic.
> 
> 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: Star Zeng <star.zeng@intel.com>
> 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>
> ---
>  .../ReportStatusCodeLib.c                     | 45 +++++++++----------
>  .../ReportStatusCodeLib.c                     | 14 +++---
>  2 files changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/DxeReportStatusCodeLib/ReportStatusCodeLib.c
> b/MdeModulePkg/Library/DxeReportStatusCodeLib/ReportStatusCodeLib.c
> index b28dc5c3bb..d3cf8b1de3 100644
> --- a/MdeModulePkg/Library/DxeReportStatusCodeLib/ReportStatusCodeLib.c
> +++ b/MdeModulePkg/Library/DxeReportStatusCodeLib/ReportStatusCodeLib.c
> @@ -494,41 +494,38 @@ ReportStatusCodeEx (
>    UINT64                Buffer[(MAX_EXTENDED_DATA_SIZE / sizeof (UINT64)) + 1];
> 
>    ASSERT (!((ExtendedData == NULL) && (ExtendedDataSize != 0)));
>    ASSERT (!((ExtendedData != NULL) && (ExtendedDataSize == 0)));
> 
> -  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);
> +  if (ExtendedDataSize <= (MAX_EXTENDED_DATA_SIZE - sizeof (EFI_STATUS_CODE_DATA))) {
> +    //
> +    // Use Buffer instead of allocating if possible.
> +    //
> +    StatusCodeData = (EFI_STATUS_CODE_DATA *)Buffer;  } else {
> +    if (gBS == NULL || gBS->AllocatePool == NULL || gBS->FreePool == NULL) {
> +      return EFI_UNSUPPORTED;
> +    }
> 
> -  StatusCodeData = NULL;
> -  if (Tpl <= TPL_NOTIFY) {
>      //
> -    // Allocate space for the Status Code Header and its buffer
> +    // Retrieve the current TPL
>      //
> -    gBS->AllocatePool (EfiBootServicesData, sizeof (EFI_STATUS_CODE_DATA) + ExtendedDataSize, (VOID **)&StatusCodeData);
> -  }
> +    Tpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);
> +    gBS->RestoreTPL (Tpl);
> +
> +    if (Tpl > TPL_NOTIFY) {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> 
> -  if (StatusCodeData == NULL) {
>      //
> -    // If a buffer could not be allocated, then see if the local variable Buffer can be used
> +    // Allocate space for the Status Code Header and its buffer
>      //
> -    if (ExtendedDataSize > (MAX_EXTENDED_DATA_SIZE - sizeof (EFI_STATUS_CODE_DATA))) {
> -      //
> -      // The local variable Buffer not large enough to hold the extended data associated
> -      // with the status code being reported.
> -      //
> -      DEBUG ((EFI_D_ERROR, "Status code extended data is too large to be reported!\n"));
> +    StatusCodeData = NULL;
> +    gBS->AllocatePool (EfiBootServicesData, sizeof (EFI_STATUS_CODE_DATA) + ExtendedDataSize, (VOID **)&StatusCodeData);
> +    if (StatusCodeData == NULL) {
>        return EFI_OUT_OF_RESOURCES;
>      }
> -    StatusCodeData = (EFI_STATUS_CODE_DATA  *)Buffer;
>    }
> 
>    //
>    // Fill in the extended data header
>    //
> @@ -626,6 +623,6 @@ EFIAPI
>  ReportDebugCodeEnabled (
>    VOID
>    )
>  {
>    return (BOOLEAN) ((PcdGet8 (PcdReportStatusCodePropertyMask) & REPORT_STATUS_CODE_PROPERTY_DEBUG_CODE_ENABLED) != 0);
> -}
> +}
> \ No newline at end of file
> diff --git a/MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/ReportStatusCodeLib.c
> b/MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/ReportStatusCodeLib.c
> index b73103517a..9b79854538 100644
> --- a/MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/ReportStatusCodeLib.c
> +++ b/MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/ReportStatusCod
> +++ eLib.c
> @@ -630,16 +630,20 @@ ReportStatusCodeEx (
>    UINT64                StatusCodeBuffer[(MAX_EXTENDED_DATA_SIZE / sizeof (UINT64)) + 1];
> 
>    ASSERT (!((ExtendedData == NULL) && (ExtendedDataSize != 0)));
>    ASSERT (!((ExtendedData != NULL) && (ExtendedDataSize == 0)));
> 
> -  if (mHaveExitedBootServices) {
> -    if (sizeof (EFI_STATUS_CODE_DATA) + ExtendedDataSize > MAX_EXTENDED_DATA_SIZE) {
> +  if (ExtendedDataSize <= (MAX_EXTENDED_DATA_SIZE - sizeof (EFI_STATUS_CODE_DATA))) {
> +    //
> +    // Use Buffer instead of allocating if possible.
> +    //
> +    StatusCodeData = (EFI_STATUS_CODE_DATA *) StatusCodeBuffer;  } else
> + {
> +    if (mHaveExitedBootServices) {
>        return EFI_OUT_OF_RESOURCES;
>      }
> -    StatusCodeData = (EFI_STATUS_CODE_DATA *) StatusCodeBuffer;
> -  } else  {
> +
>      if (gBS == NULL || gBS->AllocatePool == NULL || gBS->FreePool == NULL) {
>        return EFI_UNSUPPORTED;
>      }
> 
>      //
> @@ -678,11 +682,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