On 02/20/19 10:21, Ard Biesheuvel wrote: > On Wed, 20 Feb 2019 at 09:16, Laszlo Ersek <ler...@redhat.com> 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 <anthony.per...@citrix.com> >> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> >> Cc: Jordan Justen <jordan.l.jus...@intel.com> >> Cc: Julien Grall <julien.gr...@linaro.org> >> Cc: Ray Ni <ray...@intel.com> >> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1515418 >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Laszlo Ersek <ler...@redhat.com> >> --- >> >> 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 >> <https://bugzilla.tianocore.org/show_bug.cgi?id=1398> and >> <https://mantis.uefi.org/mantis/view.php?id=1885>, and follow Ray's >> advice in >> >> <http://mid.mail-archive.com/734D49CCEBEEF84792F5B80ED585239D5BACE29B@SHSMSX104.ccr.corp.intel.com>: >> >> - 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.<BR><BR> > # PROGRESS_CODE_OS_LOADER_LOAD = (EFI_SOFTWARE_DXE_BS_DRIVER | > (EFI_OEM_SPECIFIC | 0x00000000)) = 0x03058000<BR> > # @Prompt Progress Code for OS Loader LoadImage start. > # @ValidList 0x80000003 | 0x03058000 > > gEfiMdeModulePkgTokenSpaceGuid.PcdProgressCodeOsLoaderLoad|0x03058000|UINT32|0x30001030 > > ## Progress Code for OS Loader StartImage start.<BR><BR> > # PROGRESS_CODE_OS_LOADER_START = (EFI_SOFTWARE_DXE_BS_DRIVER | > (EFI_OEM_SPECIFIC | 0x00000001)) = 0x03058001<BR> > # @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 <ard.biesheu...@linaro.org> Thank you! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
