From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 3D0E020886F35 for ; Tue, 19 Feb 2019 17:19:16 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 35FC759462; Wed, 20 Feb 2019 01:19:16 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-156.rdu2.redhat.com [10.10.120.156]) by smtp.corp.redhat.com (Postfix) with ESMTP id DFA5C5D6B3; Wed, 20 Feb 2019 01:19:14 +0000 (UTC) To: Dandan Bi Cc: edk2-devel@lists.01.org, Hao Wu , Ruiyu Ni References: <20190215085141.64244-1-dandan.bi@intel.com> <20190215085141.64244-3-dandan.bi@intel.com> From: Laszlo Ersek Message-ID: <3dbe48e2-3c1d-1cf8-3172-6a96e27ee454@redhat.com> Date: Wed, 20 Feb 2019 02:19:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190215085141.64244-3-dandan.bi@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Wed, 20 Feb 2019 01:19:16 +0000 (UTC) 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 01:19:17 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > 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 = 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