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-boun...@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Wednesday, February 20, 2019 9:19 AM > To: Bi, Dandan <dandan...@intel.com> > Cc: Wu, Hao A <hao.a...@intel.com>; Ni, Ray <ray...@intel.com>; edk2- > de...@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.w...@intel.com> > > Cc: Hao Wu <hao.a...@intel.com> > > Cc: Ruiyu Ni <ruiyu...@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Cc: Sean Brogan <sean.bro...@microsoft.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Dandan Bi <dandan...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
