From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.136; helo=mga12.intel.com; envelope-from=dandan.bi@intel.com; receiver=edk2-devel@lists.01.org Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 51CFA208AE2B5 for ; Tue, 19 Feb 2019 18:35:22 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Feb 2019 18:35:21 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,388,1544515200"; d="scan'208";a="145667240" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga004.fm.intel.com with ESMTP; 19 Feb 2019 18:35:21 -0800 Received: from fmsmsx117.amr.corp.intel.com (10.18.116.17) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.408.0; Tue, 19 Feb 2019 18:35:21 -0800 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by fmsmsx117.amr.corp.intel.com (10.18.116.17) with Microsoft SMTP Server (TLS) id 14.3.408.0; Tue, 19 Feb 2019 18:35:21 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.102]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.36]) with mapi id 14.03.0415.000; Wed, 20 Feb 2019 10:35:19 +0800 From: "Bi, Dandan" To: Laszlo Ersek CC: "Wu, Hao A" , "Ni, Ray" , "edk2-devel@lists.01.org" Thread-Topic: [edk2] [patch 2/2] MdeModulePkg/BmBoot: Report status when fail to load/start boot option Thread-Index: AQHUxQvCjlMHpyNFKEa+BBITLpqeSaXnZGCAgACaLXA= Date: Wed, 20 Feb 2019 02:35:18 +0000 Message-ID: <3C0D5C461C9E904E8F62152F6274C0BB40B942E1@SHSMSX104.ccr.corp.intel.com> References: <20190215085141.64244-1-dandan.bi@intel.com> <20190215085141.64244-3-dandan.bi@intel.com> <3dbe48e2-3c1d-1cf8-3172-6a96e27ee454@redhat.com> In-Reply-To: <3dbe48e2-3c1d-1cf8-3172-6a96e27ee454@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [patch 2/2] MdeModulePkg/BmBoot: Report status when fail to load/start boot option X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Feb 2019 02:35:22 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 > Cc: Wu, Hao A ; Ni, Ray ; edk2- > devel@lists.01.org > Subject: Re: [edk2] [patch 2/2] MdeModulePkg/BmBoot: Report status when > fail to load/start boot option >=20 > Hi Dandan, >=20 > On 02/15/19 09:51, Dandan Bi wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1398 > > > > 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 > > Cc: Hao Wu > > Cc: Ruiyu Ni > > Cc: Laszlo Ersek > > Cc: Sean Brogan > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Dandan Bi > > --- > > .../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 =3D Status; > > // > > // Destroy the RAM disk > > // > > @@ -1902,15 +1907,20 @@ EfiBootManagerBoot ( > > Status =3D gBS->StartImage (ImageHandle, &BootOption->ExitDataSize, > &BootOption->ExitData); > > DEBUG ((DEBUG_INFO | DEBUG_LOAD, "Image Return Status =3D %r\n", > Status)); > > BootOption->Status =3D 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); > > > > // > > >=20 > Unfortunately, this patch is not good; we made a mistake here. >=20 > Consider the EFI_RETURN_STATUS_EXTENDED_DATA structure, added in > patch > #1: >=20 > > 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 fail= ure > triggered the > > /// reporting of the status code (generally an error code or a debug = code). > > /// > > EFI_STATUS ReturnStatus; > > } EFI_RETURN_STATUS_EXTENDED_DATA; >=20 > According to the UEFI spec, unless specified otherwise, structure members > are aligned naturally. >=20 > And, the PI spec references the UEFI spec with regard to data types. >=20 > Accordingly, when this structure is built for X64, the size of this struc= ture 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 >=20 > > typedef struct { > > /// > > /// The size of the structure. This is specified to enable future exp= ansion. > > /// > > 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; >=20 > which extends to 20 bytes. >=20 > I'm working on patches that capture / process > EFI_RETURN_STATUS_EXTENDED_DATA. The fields I'm seeing in DataHeader > are (on X64): > - HeaderSize =3D 0x14 (20 decimal) > - Size =3D 0x8, > - Type =3D { > Data1 =3D 0x335984bd, > Data2 =3D 0xe805, > Data3 =3D 0x409a, > Data4 =3D {0xb8, 0xf8, 0xd2, 0x7e, 0xce, 0x5f, 0xf7, 0xa6} > } >=20 > The "DataHeader.Size" field is incorrect. It should be 12 (that is, 32-20= ), > according to the documentation: >=20 > > /// DataHeader.Size should be > > sizeof(EFI_RETURN_STATUS_EXTENDED_DATA) - HeaderSize, >=20 > 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 () macr= o > with the trailing portion of this temporary object. >=20 > I'll report the same in a TianoCore BZ, and will try to submit a patch as= well. >=20 > I'm sorry that I didn't catch this in review. >=20 > Thanks > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel