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

["Bi, Dandan" <dandan.bi@intel.com>]

Hi Laszlo,

Thanks for helping fix it.
Reviewed-by: Bi Dandan <dandan.bi@intel.com>


Thanks,
Dandan
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, February 21, 2019 6:41 PM
> To: edk2-devel@lists.01.org
> Cc: Bi, Dandan <dandan.bi@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>; Sean
> Brogan <sean.brogan@microsoft.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH v3 1/5] MdeModulePkg/UefiBootManagerLib: fix
> LoadImage/StartImage status code rep.
> 
> 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 BmReportLoadFailure() 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:
>     v3:
> 
>     - rename BmReportImageFailure() to BmReportLoadFailure() [Ray]
> 
>     - eliminate PaddingStart and PaddingSize; zero out the full ExtendedData
>       struct [Ray]
> 
>     - don't pick up Ard's R-b due to the change above being functional in
>       nature
> 
>     v2:
>     - new in v2
> 
>  MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h |  1 +
>  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c     | 65
> ++++++++++++++------
>  2 files changed, 48 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..02ff354ef6a3 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> @@ -1667,6 +1667,51 @@ 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
> +BmReportLoadFailure (
> +  IN UINT32     ErrorCode,
> +  IN EFI_STATUS FailureStatus
> +  )
> +{
> +  EFI_RETURN_STATUS_EXTENDED_DATA ExtendedData;
> +
> +  if (!ReportErrorCodeEnabled ()) {
> +    return;
> +  }
> +
> +  ASSERT (
> +    (ErrorCode == EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR) ||
> +    (ErrorCode == EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED)
> +    );
> +
> +  ZeroMem (&ExtendedData, sizeof (ExtendedData));
> + ExtendedData.ReturnStatus = FailureStatus;
> +
> +  REPORT_STATUS_CODE_EX (
> +    (EFI_ERROR_CODE | EFI_ERROR_MINOR),
> +    (EFI_SOFTWARE_DXE_BS_DRIVER | ErrorCode),
> +    0,
> +    NULL,
> +    NULL,
> +    &ExtendedData.DataHeader + 1,
> +    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 +1867,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)
> -        );
> +      BmReportLoadFailure
> (EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR,
> + Status);
>        BootOption->Status = Status;
>        //
>        // Destroy the RAM disk
> @@ -1911,15 +1948,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)
> -      );
> +    BmReportLoadFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED,
> Status);
>    }
>    PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32)
> OptionNumber);
> 
> --
> 2.19.1.3.g30247aa5d201
>