Re: [patch 2/2] MdeModulePkg/BmBoot: Report status when fail to load/start boot option

[Laszlo Ersek <lersek@redhat.com>]

Hi Dandan,

On 02/15/19 09:51, Dandan Bi wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1398
>
> According to PI1.7 Spec, report extended data describing an
> EFI_STATUS return value along with
> EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR and
> EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED status code
> when fail to load or start boot option image.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> ---
>  .../Library/UefiBootManagerLib/BmBoot.c       | 22 ++++++++++++++-----
>  1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> index 6444fb43eb..9be1633b74 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> @@ -1818,15 +1818,20 @@ EfiBootManagerBoot (
>        FreePool (FilePath);
>      }
>
>      if (EFI_ERROR (Status)) {
>        //
> -      // Report Status Code to indicate that the failure to load boot option
> +      // Report Status Code with the failure status to indicate that the failure to load boot option
>        //
> -      REPORT_STATUS_CODE (
> +      REPORT_STATUS_CODE_EX (
>          EFI_ERROR_CODE | EFI_ERROR_MINOR,
> -        (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR)
> +        (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR),
> +        0,
> +        NULL,
> +        NULL,
> +        &Status,
> +        sizeof (EFI_STATUS)
>          );
>        BootOption->Status = Status;
>        //
>        // Destroy the RAM disk
>        //
> @@ -1902,15 +1907,20 @@ EfiBootManagerBoot (
>    Status = gBS->StartImage (ImageHandle, &BootOption->ExitDataSize, &BootOption->ExitData);
>    DEBUG ((DEBUG_INFO | DEBUG_LOAD, "Image Return Status = %r\n", Status));
>    BootOption->Status = Status;
>    if (EFI_ERROR (Status)) {
>      //
> -    // Report Status Code to indicate that boot failure
> +    // Report Status Code with the failure status to indicate that boot failure
>      //
> -    REPORT_STATUS_CODE (
> +    REPORT_STATUS_CODE_EX (
>        EFI_ERROR_CODE | EFI_ERROR_MINOR,
> -      (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED)
> +      (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED),
> +      0,
> +      NULL,
> +      NULL,
> +      &Status,
> +      sizeof (EFI_STATUS)
>        );
>    }
>    PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber);
>
>    //
>

Unfortunately, this patch is not good; we made a mistake here.

Consider the EFI_RETURN_STATUS_EXTENDED_DATA structure, added in patch
#1:

> typedef struct {
>   ///
>   /// The data header identifying the data:
>   /// DataHeader.HeaderSize should be sizeof(EFI_STATUS_CODE_DATA),
>   /// DataHeader.Size should be sizeof(EFI_RETURN_STATUS_EXTENDED_DATA) - HeaderSize,
>   /// DataHeader.Type should be EFI_STATUS_CODE_SPECIFIC_DATA_GUID.
>   ///
>   EFI_STATUS_CODE_DATA DataHeader;
>   ///
>   /// The EFI_STATUS return value of the service or function whose failure triggered the
>   /// reporting of the status code (generally an error code or a debug code).
>   ///
>   EFI_STATUS           ReturnStatus;
> } EFI_RETURN_STATUS_EXTENDED_DATA;

According to the UEFI spec, unless specified otherwise, structure
members are aligned naturally.

And, the PI spec references the UEFI spec with regard to data types.

Accordingly, when this structure is built for X64, the size of this
structure is 32 bytes, and the offset of ReturnStatus is 24. There is a
4-byte padding between DataHeader (which is 20 bytes in size) and the
ReturnStatus field. DataHeader has type

> typedef struct {
>   ///
>   /// The size of the structure. This is specified to enable future expansion.
>   ///
>   UINT16    HeaderSize;
>   ///
>   /// The size of the data in bytes. This does not include the size of the header structure.
>   ///
>   UINT16    Size;
>   ///
>   /// The GUID defining the type of the data.
>   ///
>   EFI_GUID  Type;
> } EFI_STATUS_CODE_DATA;

which extends to 20 bytes.

I'm working on patches that capture / process
EFI_RETURN_STATUS_EXTENDED_DATA. The fields I'm seeing in DataHeader are
(on X64):
- HeaderSize = 0x14 (20 decimal)
- Size = 0x8,
- Type = {
    Data1 = 0x335984bd,
    Data2 = 0xe805,
    Data3 = 0x409a,
    Data4 = {0xb8, 0xf8, 0xd2, 0x7e, 0xce, 0x5f, 0xf7, 0xa6}
  }

The "DataHeader.Size" field is incorrect. It should be 12 (that is,
32-20), according to the documentation:

>   /// DataHeader.Size should be sizeof(EFI_RETURN_STATUS_EXTENDED_DATA) - HeaderSize,

I think in the code above, we should use a temporary
EFI_RETURN_STATUS_EXTENDED_DATA structure, zero it out, then set the
ReturnStatus field in it. Finally, call the REPORT_STATUS_CODE_EX ()
macro with the trailing portion of this temporary object.

I'll report the same in a TianoCore BZ, and will try to submit a patch
as well.

I'm sorry that I didn't catch this in review.

Thanks
Laszlo