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

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

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