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 4C6472194EB7D for ; Wed, 20 Feb 2019 04:01:21 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 16A8E6698C; Wed, 20 Feb 2019 12:01:21 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-220.rdu2.redhat.com [10.10.120.220]) by smtp.corp.redhat.com (Postfix) with ESMTP id 61AC317AE4; Wed, 20 Feb 2019 12:01:19 +0000 (UTC) To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , Anthony Perard , Jordan Justen , Julien Grall , Ray Ni References: <20190220081644.8238-1-lersek@redhat.com> <20190220081644.8238-3-lersek@redhat.com> From: Laszlo Ersek Message-ID: <4cfefdaa-885a-1810-b3b8-1eecab74d849@redhat.com> Date: Wed, 20 Feb 2019 13:01: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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Wed, 20 Feb 2019 12:01:21 +0000 (UTC) Subject: Re: [PATCH v2 2/5] OvmfPkg: add library to track boot option loading/starting on the console 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 12:01:22 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/20/19 10:21, Ard Biesheuvel wrote: > On Wed, 20 Feb 2019 at 09:16, Laszlo Ersek wrote: >> >> Introduce the Platform Boot Manager Print Status Code Library (for short, >> PlatformBmPrintScLib) class for catching and printing the LoadImage() / >> StartImage() preparations, and return statuses, that are reported by >> UefiBootManagerLib. >> >> In the primary library instance, catch only such status codes that >> UefiBootManagerLib reports from the same module that contains >> PlatformBmPrintScLib. The intent is to establish a reporting-printing >> channel within BdsDxe, between UefiBootManagerLib and >> PlatformBmPrintScLib. Ignore status codes originating elsewhence, e.g. >> from UiApp's copy of UefiBootManagerLib. >> >> Cc: Anthony Perard >> Cc: Ard Biesheuvel >> Cc: Jordan Justen >> Cc: Julien Grall >> Cc: Ray Ni >> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1515418 >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Laszlo Ersek >> --- >> >> Notes: >> v2: >> >> - Split the status code handling to a separate library, so that it's >> easy to reuse in ArmVirtPkg. >> >> - Rework the logic based on >> and >> , and follow Ray's >> advice in >> : >> >> - The boot option details are fetched via BootCurrent. >> >> - For reporting LoadImage() and StartImage() preparations, replace the >> originally proposed PcdDebugCodeOsLoaderDetail status code with the >> existent (edk2-specific) PcdProgressCodeOsLoaderLoad and >> PcdProgressCodeOsLoaderStart status codes. >> >> - For reporting LoadImage() and StartImage() return values, replace >> the originally proposed PcdDebugCodeOsLoaderDetail status code with >> the standard EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR and >> EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED status codes. >> >> - For all four kinds of reports, replace the originally proposed "OS >> Loader Detail" structure (and GUID) with the recently standardized >> EFI_RETURN_STATUS_EXTENDED_DATA structure. >> >> OvmfPkg/OvmfPkg.dec | 5 + >> OvmfPkg/OvmfPkgIa32.dsc | 1 + >> OvmfPkg/OvmfPkgIa32X64.dsc | 1 + >> OvmfPkg/OvmfPkgX64.dsc | 1 + >> OvmfPkg/Include/Library/PlatformBmPrintScLib.h | 41 +++ >> OvmfPkg/Library/PlatformBmPrintScLib/PlatformBmPrintScLib.inf | 66 +++++ >> OvmfPkg/Library/PlatformBmPrintScLib/StatusCodeHandler.c | 310 ++++++++++++++++++++ >> 7 files changed, 425 insertions(+) >> [...] >> + // >> + // Set the EFI_STATUS_CODE_VALUE convenience variables. >> + // >> + mLoadPrep = PcdGet32 (PcdProgressCodeOsLoaderLoad); >> + mLoadFail = (EFI_SOFTWARE_DXE_BS_DRIVER | >> + EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR); >> + mStartPrep = PcdGet32 (PcdProgressCodeOsLoaderStart); >> + mStartFail = (EFI_SOFTWARE_DXE_BS_DRIVER | >> + EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED); >> + > > This bit looks somewhat dodgy to me, but I suppose the asymmetry is > 'prior art' from EDK2, no? Yes, that's the case. All four status code values are taken verbatim from EfiBootManagerBoot() [MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c], where they are reported / produced. I use module-global variables here because (a) I need no generality wrt. status codes values in this module (I really only care for these four), and (b) the original expressions are simply unbearably long; considering the frequent use of these status code values in the patch. Regarding the reporting in EfiBootManagerBoot(): the status code values - (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_FAILED) are from the PI spec. If we expand the macros a bit, we get, respectively: - EFI_SOFTWARE | 0x00050000 | EFI_SUBCLASS_SPECIFIC | 0x00000002 - EFI_SOFTWARE | 0x00050000 | EFI_SUBCLASS_SPECIFIC | 0x00000003 So we are in the "software class", the "DXE Boot Service Driver" subclass, and we report values 2 and 3, which are meant to be unique only within that subclass. Conversely, the "prep" status code values are edk2 extensions. The PCDs allow a platform, in theory anyway, to tweak the exact values. But in practice, that should never be necessary. Let's check their default values, in "MdeModulePkg/MdeModulePkg.dec": > ## Progress Code for OS Loader LoadImage start.

> # PROGRESS_CODE_OS_LOADER_LOAD = (EFI_SOFTWARE_DXE_BS_DRIVER | (EFI_OEM_SPECIFIC | 0x00000000)) = 0x03058000
> # @Prompt Progress Code for OS Loader LoadImage start. > # @ValidList 0x80000003 | 0x03058000 > gEfiMdeModulePkgTokenSpaceGuid.PcdProgressCodeOsLoaderLoad|0x03058000|UINT32|0x30001030 > > ## Progress Code for OS Loader StartImage start.

> # PROGRESS_CODE_OS_LOADER_START = (EFI_SOFTWARE_DXE_BS_DRIVER | (EFI_OEM_SPECIFIC | 0x00000001)) = 0x03058001
> # @Prompt Progress Code for OS Loader StartImage start. > # @ValidList 0x80000003 | 0x03058001 > gEfiMdeModulePkgTokenSpaceGuid.PcdProgressCodeOsLoaderStart|0x03058001|UINT32|0x30001031 Meaning - EFI_SOFTWARE | 0x00050000 | EFI_OEM_SPECIFIC | 0x00000000 - EFI_SOFTWARE | 0x00050000 | EFI_OEM_SPECIFIC | 0x00000001 We stay within the same class & subclass, but use OEM-specific values 0 and 1, rather than standard values 2 and 3 that are specific to the subclass. I'd prefer if these weren't even PCDs, and the PROGRESS_CODE_OS_LOADER_LOAD and PROGRESS_CODE_OS_LOADER_START macros actually existed in some header file. That would be similarly clear about the values being edk2 extensions, but without muddying the picture with (academic?) platform overrides. > In any case, this looks good to me otherwise, so for the series > > Reviewed-by: Ard Biesheuvel Thank you! Laszlo