[patch 0/2] Report error status when fail to load/start boot option

[patch 0/2] Report error status when fail to load/start boot option

[Dandan Bi]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1398

According to PI1.7, this patch series is to add the support that
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.

These two commit are also available at:
https://github.com/dandanbi/edk2/commits/ExtendedStatusCodeV1

PI1.7 is at
https://uefi.org/sites/default/files/resources/PI_Spec_1_7_final_Jan_2019.pdf

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: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Dandan Bi (2):
  MdePkg/StatusCodeDataTypeId.h: Add new definition per PI1.7 Spec
  MdeModulePkg/BmBoot: Report status when fail to load/start boot option

 .../Library/UefiBootManagerLib/BmBoot.c       | 22 ++++++++++++++-----
 MdePkg/Include/Guid/StatusCodeDataTypeId.h    | 22 ++++++++++++++++++-
 2 files changed, 37 insertions(+), 7 deletions(-)

-- 
2.18.0.windows.1

[patch 1/2] MdePkg/StatusCodeDataTypeId.h: Add new definition per PI1.7 Spec

[Dandan Bi]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1398

According to PI1.7 Spec, add the new definition
EFI_RETURN_STATUS_EXTENDED_DATA in StatusCodeDataTypeId.h

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
---
 MdePkg/Include/Guid/StatusCodeDataTypeId.h | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Include/Guid/StatusCodeDataTypeId.h b/MdePkg/Include/Guid/StatusCodeDataTypeId.h
index 22cf8e5aae..b01999581b 100644
--- a/MdePkg/Include/Guid/StatusCodeDataTypeId.h
+++ b/MdePkg/Include/Guid/StatusCodeDataTypeId.h
@@ -1,9 +1,9 @@
 /** @file
   GUID used to identify id for the caller who is initiating the Status Code.
 
-  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD License
   which accompanies this distribution.  The full text of the license may be found at
   http://opensource.org/licenses/bsd-license.php
 
@@ -116,10 +116,11 @@ extern EFI_GUID gEfiStatusCodeDataTypeStringGuid;
 ///   - EFI_MEMORY_RANGE_EXTENDED_DATA
 ///   - EFI_DEBUG_ASSERT_DATA
 ///   - EFI_STATUS_CODE_EXCEP_EXTENDED_DATA
 ///   - EFI_STATUS_CODE_START_EXTENDED_DATA
 ///   - EFI_LEGACY_OPROM_EXTENDED_DATA
+///   - EFI_RETURN_STATUS_EXTENDED_DATA
 ///
 #define EFI_STATUS_CODE_SPECIFIC_DATA_GUID \
   { 0x335984bd, 0xe805, 0x409a, { 0xb8, 0xf8, 0xd2, 0x7e, 0xce, 0x5f, 0xf7, 0xa6 } }
 
 ///
@@ -782,8 +783,27 @@ typedef struct {
   /// The base address of the shadowed legacy ROM image.  May or may not point to the shadow RAM area.
   ///
   EFI_PHYSICAL_ADDRESS  RomImageBase;
 } EFI_LEGACY_OPROM_EXTENDED_DATA;
 
+///
+/// This structure defines extended data describing an EFI_STATUS return value that stands for a
+/// failed function call (such as a UEFI boot service).
+///
+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;
+
 extern EFI_GUID gEfiStatusCodeSpecificDataGuid;
 
 #endif
-- 
2.18.0.windows.1

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

[Dandan Bi]

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);
 
   //
-- 
2.18.0.windows.1

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

[Laszlo Ersek]

On 02/15/19 09:51, Dandan Bi wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1398
> 
> According to PI1.7, this patch series is to add the support that
> 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.
> 
> These two commit are also available at:
> https://github.com/dandanbi/edk2/commits/ExtendedStatusCodeV1
> 
> PI1.7 is at
> https://uefi.org/sites/default/files/resources/PI_Spec_1_7_final_Jan_2019.pdf
> 
> 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: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Dandan Bi (2):
>   MdePkg/StatusCodeDataTypeId.h: Add new definition per PI1.7 Spec
>   MdeModulePkg/BmBoot: Report status when fail to load/start boot option
> 
>  .../Library/UefiBootManagerLib/BmBoot.c       | 22 ++++++++++++++-----
>  MdePkg/Include/Guid/StatusCodeDataTypeId.h    | 22 ++++++++++++++++++-
>  2 files changed, 37 insertions(+), 7 deletions(-)
> 

Looks good to me.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thank you,
Laszlo

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

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Friday, February 15, 2019 9:40 PM
> To: Bi, Dandan <dandan.bi@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2] [patch 0/2] Report error status when fail to load/start boot
> option
> 
> On 02/15/19 09:51, Dandan Bi wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1398
> >
> > According to PI1.7, this patch series is to add the support that
> > 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.
> >
> > These two commit are also available at:
> > https://github.com/dandanbi/edk2/commits/ExtendedStatusCodeV1
> >
> > PI1.7 is at
> > https://uefi.org/sites/default/files/resources/PI_Spec_1_7_final_Jan_2
> > 019.pdf
> >
> > 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: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com> Dandan Bi (2):
> >   MdePkg/StatusCodeDataTypeId.h: Add new definition per PI1.7 Spec
> >   MdeModulePkg/BmBoot: Report status when fail to load/start boot
> > option
> >
> >  .../Library/UefiBootManagerLib/BmBoot.c       | 22 ++++++++++++++-----
> >  MdePkg/Include/Guid/StatusCodeDataTypeId.h    | 22 ++++++++++++++++++-
> >  2 files changed, 37 insertions(+), 7 deletions(-)
> >
> 
> Looks good to me.
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thank you,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

On 02/20/19 02:19, Laszlo Ersek wrote:
> 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.

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

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

https://bugzilla.tianocore.org/show_bug.cgi?id=1539

Thanks
Laszlo

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

[Ni, Ray]

Laszlo,
Thanks for catching this.

GenPerformMemoryTest() in
MdeModulePkg\Universal\MemoryTest\GenericMemoryTestDxe\LightMemoryTest.c
uses the same technics as you suggested.

I give up to propose another option: having pack(1) for the new status structure.


> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, February 20, 2019 9:19 AM
> To: Bi, Dandan <dandan.bi@intel.com>
> Cc: edk2-devel@lists.01.org; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> Subject: Re: [edk2] [patch 2/2] MdeModulePkg/BmBoot: Report status when
> fail to load/start boot option
> 
> 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

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

[Bi, Dandan]

Hi Laszlo,

Thanks for catching this issue.
I am sorry that I didn't consider the alignment issue when working on this patch.


Thanks,
Dandan

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Wednesday, February 20, 2019 9:19 AM
> To: Bi, Dandan <dandan.bi@intel.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; edk2-
> devel@lists.01.org
> Subject: Re: [edk2] [patch 2/2] MdeModulePkg/BmBoot: Report status when
> fail to load/start boot option
> 
> 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
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Laszlo Ersek]

+Mark, for my comments on the process

On 02/20/19 03:21, Ni, Ray wrote:
> Laszlo,
> Thanks for catching this.
> 
> GenPerformMemoryTest() in
> MdeModulePkg\Universal\MemoryTest\GenericMemoryTestDxe\LightMemoryTest.c
> uses the same technics as you suggested.
> 
> I give up to propose another option: having pack(1) for the new status structure.

I think that byte-packing EFI_RETURN_STATUS_EXTENDED_DATA in the PI-1.7
spec would have been viable, but we should have thought about that in
advance. The PI and UEFI specs require natural alignment for structure
members, unless stated otherwise. There *are* a number of examples of
"stated otherwise" (the term that the specs use is frequently
"byte-packed structure"). Again, we should have thought of that in
advance, before PI-1.7 was released, so now we can only fix the way the
object is populated.

More generally, I think this demonstrates a flaw in the standardization
process. PIWG and USWG should only standardize existing practice; that
is, features that have at least one functional implementation. (Not
necessarily open source, but the interfaces should be battle-hardened
*first*.) The restriction where we can't even propose patches for edk2
before the standards are updated is very counter-productive. There's
practically zero chance for getting an actual implementation
peer-reviewed and peer-tested before proposing the API for the standard.

Personally I have suggested in the past to implement some new features
as edk2 extensions first, and once they work, to upstream them to the
standards. This idea is usually rejected by maintainers, because
maintainers worry about becoming stuck with more and more edk2
extensions to core code (i.e., they worry about the feature proponents
not making good on their promise to standardize). Unfortunately, the
alternative (= the current practice) is that we standardize speculative
interfaces, which then prove suboptimal once we implement them.

Thanks
Laszlo

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

[Doran, Mark]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, February 20, 2019 1:25 AM
> To: Ni, Ray <ray.ni@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> Cc: edk2-devel@lists.01.org; Wu, Hao A <hao.a.wu@intel.com>; Doran, Mark
> <mark.doran@intel.com>
> Subject: Re: [edk2] [patch 2/2] MdeModulePkg/BmBoot: Report status when
> fail to load/start boot option
> 
> +Mark, for my comments on the process

Thank you :)
 
> On 02/20/19 03:21, Ni, Ray wrote:
> > Laszlo,
> > Thanks for catching this.
> >
> > GenPerformMemoryTest() in
> > MdeModulePkg\Universal\MemoryTest\GenericMemoryTestDxe\LightMemoryTest
> > .c uses the same technics as you suggested.
> >
> > I give up to propose another option: having pack(1) for the new status
> structure.
> 
> I think that byte-packing EFI_RETURN_STATUS_EXTENDED_DATA in the PI-1.7
> spec would have been viable, but we should have thought about that in
> advance. The PI and UEFI specs require natural alignment for structure
> members, unless stated otherwise. There *are* a number of examples of
> "stated otherwise" (the term that the specs use is frequently "byte-packed
> structure"). Again, we should have thought of that in advance, before PI-
> 1.7 was released, so now we can only fix the way the object is populated.

Agreed.
 
> More generally, I think this demonstrates a flaw in the standardization
> process. PIWG and USWG should only standardize existing practice; that is,
> features that have at least one functional implementation. (Not necessarily
> open source, but the interfaces should be battle-hardened
> *first*.) The restriction where we can't even propose patches for edk2
> before the standards are updated is very counter-productive. There's
> practically zero chance for getting an actual implementation peer-reviewed
> and peer-tested before proposing the API for the standard.
> 
> Personally I have suggested in the past to implement some new features as
> edk2 extensions first, and once they work, to upstream them to the
> standards. This idea is usually rejected by maintainers, because
> maintainers worry about becoming stuck with more and more edk2 extensions
> to core code (i.e., they worry about the feature proponents not making good
> on their promise to standardize). Unfortunately, the alternative (= the
> current practice) is that we standardize speculative interfaces, which then
> prove suboptimal once we implement them.

So personally I'm a big believer in having working code for things we write down in the standards.  When EFI was first drafted that's how we did it: what was then known as "the sample implementation" and the actual spec content were more or less developed in parallel and the spec wasn't done until the code was working satisfactorily.  We got away from that largely because when the UEFI Forum was formed, there was nominal interest in promoting more than one implementation and keeping spec and code requirements separate was a part of that.

Recent changes, particularly inside Intel, present an opportunity for us to revisit this issue.  I'm advocating for the approach of spec-follows-code for new Contributions that Intel will make to the open source project going forward.  I'll have more to say about this at upcoming industry events.

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

[Laszlo Ersek]

On 02/20/19 18:19, Doran, Mark wrote:
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, February 20, 2019 1:25 AM
>> To: Ni, Ray <ray.ni@intel.com>; Bi, Dandan <dandan.bi@intel.com>
>> Cc: edk2-devel@lists.01.org; Wu, Hao A <hao.a.wu@intel.com>; Doran, Mark
>> <mark.doran@intel.com>
>> Subject: Re: [edk2] [patch 2/2] MdeModulePkg/BmBoot: Report status when
>> fail to load/start boot option
>>
>> +Mark, for my comments on the process
> 
> Thank you :)
>  
>> On 02/20/19 03:21, Ni, Ray wrote:
>>> Laszlo,
>>> Thanks for catching this.
>>>
>>> GenPerformMemoryTest() in
>>> MdeModulePkg\Universal\MemoryTest\GenericMemoryTestDxe\LightMemoryTest
>>> .c uses the same technics as you suggested.
>>>
>>> I give up to propose another option: having pack(1) for the new status
>> structure.
>>
>> I think that byte-packing EFI_RETURN_STATUS_EXTENDED_DATA in the PI-1.7
>> spec would have been viable, but we should have thought about that in
>> advance. The PI and UEFI specs require natural alignment for structure
>> members, unless stated otherwise. There *are* a number of examples of
>> "stated otherwise" (the term that the specs use is frequently "byte-packed
>> structure"). Again, we should have thought of that in advance, before PI-
>> 1.7 was released, so now we can only fix the way the object is populated.
> 
> Agreed.
>  
>> More generally, I think this demonstrates a flaw in the standardization
>> process. PIWG and USWG should only standardize existing practice; that is,
>> features that have at least one functional implementation. (Not necessarily
>> open source, but the interfaces should be battle-hardened
>> *first*.) The restriction where we can't even propose patches for edk2
>> before the standards are updated is very counter-productive. There's
>> practically zero chance for getting an actual implementation peer-reviewed
>> and peer-tested before proposing the API for the standard.
>>
>> Personally I have suggested in the past to implement some new features as
>> edk2 extensions first, and once they work, to upstream them to the
>> standards. This idea is usually rejected by maintainers, because
>> maintainers worry about becoming stuck with more and more edk2 extensions
>> to core code (i.e., they worry about the feature proponents not making good
>> on their promise to standardize). Unfortunately, the alternative (= the
>> current practice) is that we standardize speculative interfaces, which then
>> prove suboptimal once we implement them.
> 
> So personally I'm a big believer in having working code for things we write down in the standards.  When EFI was first drafted that's how we did it: what was then known as "the sample implementation" and the actual spec content were more or less developed in parallel and the spec wasn't done until the code was working satisfactorily.  We got away from that largely because when the UEFI Forum was formed, there was nominal interest in promoting more than one implementation and keeping spec and code requirements separate was a part of that.
> 
> Recent changes, particularly inside Intel, present an opportunity for us to revisit this issue.  I'm advocating for the approach of spec-follows-code for new Contributions that Intel will make to the open source project going forward.  I'll have more to say about this at upcoming industry events.
> 

Sounds fantastic! Thank you!
Laszlo