Re: [PATCH v2 1/5] MdeModulePkg/UefiBootManagerLib: fix LoadImage/StartImage status code rep.

["Ni, Ray" <ray.ni@Intel.com>]

On 2/20/2019 4:16 PM, Laszlo Ersek wrote:
> In the EFI_RETURN_STATUS_EXTENDED_DATA structure from PI-1.7, there may be
> padding between the DataHeader and ReturnStatus members. The
> REPORT_STATUS_CODE_EX() macro starts populating the structure immediately
> after DataHeader, therefore the source data must provide for the padding.
> 
> Extract the BmReportImageFailure() function from EfiBootManagerBoot(),
> prepare a zero padding (if any) in a temporary
> EFI_RETURN_STATUS_EXTENDED_DATA object, and fix the
> REPORT_STATUS_CODE_EX() macro invocation.
> 
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1539
> Fixes: c2cf8720a5aad74230767a1f11bade2d86de3745
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>      v2:
>      - new in v2
> 
>   MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h |  1 +
>   MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c     | 69 +++++++++++++++-----
>   2 files changed, 52 insertions(+), 18 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
> index 978fbff966f6..0fef63fceedf 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
> @@ -51,6 +51,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>   #include <Guid/MemoryTypeInformation.h>
>   #include <Guid/FileInfo.h>
>   #include <Guid/GlobalVariable.h>
> +#include <Guid/StatusCodeDataTypeId.h>
>   #include <Guid/StatusCodeDataTypeVariable.h>
>   
>   #include <Library/PrintLib.h>
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> index 9be1633b7480..ffb98c6c9b83 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> @@ -1667,6 +1667,55 @@ BmIsBootManagerMenuFilePath (
>     return FALSE;
>   }
>   
> +/**
> +  Report status code with EFI_RETURN_STATUS_EXTENDED_DATA about LoadImage() or
> +  StartImage() failure.
> +
> +  @param[in] ErrorCode      An Error Code in the Software Class, DXE Boot
> +                            Service Driver Subclass. ErrorCode will be used to
> +                            compose the Value parameter for status code
> +                            reporting. Must be one of
> +                            EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR and
> +                            EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED.
> +
> +  @param[in] FailureStatus  The failure status returned by the boot service
> +                            that should be reported.
> +**/
> +VOID
> +BmReportImageFailure (
Laszlo,
Thanks for quick fixing this issue.
To match the status code it reports, how about rename the function as 
"BmReportLoadFailure"?
Another minor comments in below.
> +  IN UINT32     ErrorCode,
> +  IN EFI_STATUS FailureStatus
> +  )
> +{
> +  EFI_RETURN_STATUS_EXTENDED_DATA ExtendedData;
> +  VOID                            *PaddingStart;
> +  UINTN                           PaddingSize;
> +
> +  if (!ReportErrorCodeEnabled ()) {
> +    return;
> +  }
> +
> +  ASSERT (
> +    (ErrorCode == EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR) ||
> +    (ErrorCode == EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED)
> +    );
> +
> +  PaddingStart = &ExtendedData.DataHeader + 1;
> +  PaddingSize = (UINTN) &ExtendedData.ReturnStatus - (UINTN) PaddingStart;
> +  ZeroMem (PaddingStart, PaddingSize);

I prefer "ZeroMem (&ExtendedData, sizeof (ExtendedData))" instead of 
introducing local variables such as PaddingStart/PaddingSize.
This makes code more good-looking:).

> +  ExtendedData.ReturnStatus = FailureStatus;
> +
> +  REPORT_STATUS_CODE_EX (
> +    (EFI_ERROR_CODE | EFI_ERROR_MINOR),
> +    (EFI_SOFTWARE_DXE_BS_DRIVER | ErrorCode),
> +    0,
> +    NULL,
> +    NULL,
> +    PaddingStart,
&ExtendedData.DataHeader + 1

> +    PaddingSize + sizeof (ExtendedData.ReturnStatus)
sizeof (ExtendedData) - sizeof (ExtendedData.DataHeader)
> +    );
> +}
> +
>   /**
>     Attempt to boot the EFI boot option. This routine sets L"BootCurent" and
>     also signals the EFI ready to boot event. If the device path for the option
> @@ -1822,15 +1871,7 @@ EfiBootManagerBoot (
>         //
>         // Report Status Code with the failure status to indicate that the failure to load boot option
>         //
> -      REPORT_STATUS_CODE_EX (
> -        EFI_ERROR_CODE | EFI_ERROR_MINOR,
> -        (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR),
> -        0,
> -        NULL,
> -        NULL,
> -        &Status,
> -        sizeof (EFI_STATUS)
> -        );
> +      BmReportImageFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR, Status);
>         BootOption->Status = Status;
>         //
>         // Destroy the RAM disk
> @@ -1911,15 +1952,7 @@ EfiBootManagerBoot (
>       //
>       // Report Status Code with the failure status to indicate that boot failure
>       //
> -    REPORT_STATUS_CODE_EX (
> -      EFI_ERROR_CODE | EFI_ERROR_MINOR,
> -      (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED),
> -      0,
> -      NULL,
> -      NULL,
> -      &Status,
> -      sizeof (EFI_STATUS)
> -      );
> +    BmReportImageFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED, Status);
>     }
>     PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber);
>   
> 


-- 
Thanks,
Ray