* [PATCH 0/5] MdeModulePkg, OvmfPkg: more easily visible boot progress reporting
@ 2017-11-22 23:58 Laszlo Ersek
2017-11-22 23:58 ` [PATCH 1/5] MdeModulePkg/BdsDxe: fall back to a Boot Manager Menu loop before hanging Laszlo Ersek
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-11-22 23:58 UTC (permalink / raw)
To: edk2-devel-01
Cc: Anthony Perard, Ard Biesheuvel, Eric Dong, Jordan Justen,
Julien Grall, Ruiyu Ni, Star Zeng
Repo: https://github.com/lersek/edk2.git
Branch: boot_diags
The point of this series is to communicate OVMF boot progress and boot
failure in a way that is easier to consume for users who aren't versed
in OVMF debug log capturing and analysis. This means that we have to
write stuff about booting to the system console.
(Please read <https://bugzilla.redhat.com/show_bug.cgi?id=1515418> as
well about our use case.)
Patch #1 makes the
DEBUG ((EFI_D_ERROR, "[Bds] Unable to boot!\n"));
CpuDeadLoop ();
part in BdsDxe more friendly, by printing a similar message to the
console, and entering the boot manager menu after a keypress. This was
suggested by Ray earlier; please refer to
<https://bugzilla.tianocore.org/show_bug.cgi?id=513>.
Patches #2 through #4 introduce a new, structured, status code for the
reporting and routing/handling infrastructure. UefiBootManagerLib should
not write directly to the console -- it already only logs DEBUGs and
reports status codes --, but we should emit more detailed information in
status codes than we currently do. OVMF's PlatformBds is modified to
catch these status codes and print them to the console. Other platforms
are not affected (beyond any catch-all handlers picking up the new
status codes, if EFI_DEBUG_CODE reporting is enabled).
Patch #5 makes sure that RELEASE builds of OVMF are also not affected,
by clearing EFI_DEBUG_CODE reporting in those builds.
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@linaro.org>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Thanks
Laszlo
Laszlo Ersek (5):
MdeModulePkg/BdsDxe: fall back to a Boot Manager Menu loop before
hanging
MdeModulePkg: introduce the EDKII_OS_LOADER_DETAIL status code payload
MdeModulePkg/UefiBootManagerLib: report EDKII_OS_LOADER_DETAIL status
code
OvmfPkg/PlatformBootManagerLib: print EDKII_OS_LOADER_DETAIL to ConOut
OvmfPkg: disable EFI_DEBUG_CODE reporting in RELEASE builds
MdeModulePkg/MdeModulePkg.dec | 9 +
MdeModulePkg/MdeModulePkg.uni | 7 +
OvmfPkg/OvmfPkgIa32.dsc | 7 +
OvmfPkg/OvmfPkgIa32X64.dsc | 7 +
OvmfPkg/OvmfPkgX64.dsc | 7 +
MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf | 2 +
OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 4 +
MdeModulePkg/Include/Guid/StatusCodeDataTypeOsLoaderDetail.h | 109 +++++++
MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h | 84 ++++++
OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h | 15 +
MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 51 +++-
MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c | 166 +++++++++++
MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 60 +++-
OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 8 +
OvmfPkg/Library/PlatformBootManagerLib/StatusCodeHandler.c | 298 ++++++++++++++++++++
15 files changed, 828 insertions(+), 6 deletions(-)
create mode 100644 MdeModulePkg/Include/Guid/StatusCodeDataTypeOsLoaderDetail.h
create mode 100644 OvmfPkg/Library/PlatformBootManagerLib/StatusCodeHandler.c
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/5] MdeModulePkg/BdsDxe: fall back to a Boot Manager Menu loop before hanging
2017-11-22 23:58 [PATCH 0/5] MdeModulePkg, OvmfPkg: more easily visible boot progress reporting Laszlo Ersek
@ 2017-11-22 23:58 ` Laszlo Ersek
2017-11-23 3:43 ` Ni, Ruiyu
2017-11-22 23:58 ` [PATCH 2/5] MdeModulePkg: introduce the EDKII_OS_LOADER_DETAIL status code payload Laszlo Ersek
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Laszlo Ersek @ 2017-11-22 23:58 UTC (permalink / raw)
To: edk2-devel-01
Cc: Ard Biesheuvel, Jordan Justen, Ruiyu Ni, Eric Dong, Star Zeng
Under the following scenario:
- no UEFI bootable application available anywhere in the system,
- ... not even for the default platform recovery option,
- no shell is built into the firmware image,
- but UiApp is available in the firmware image,
we should preferably not just hang in BdsEntry() with:
DEBUG ((EFI_D_ERROR, "[Bds] Unable to boot!\n"));
CpuDeadLoop ();
while the user sits at the TianoCore logo page, wondering what's going on.
Print an informative message to the console, wait for a keypress, and then
return to the Boot Manager Menu forever.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1515418
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=513
Suggested-by: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 60 ++++++++++++++++++--
1 file changed, 56 insertions(+), 4 deletions(-)
diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index dccc49090219..2b24755ac368 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -676,24 +676,73 @@ BdsAllocateMemoryForPerformanceData (
//
// Mark L"PerfDataMemAddr" variable to read-only if the Variable Lock protocol exists
// Still lock it even the variable cannot be saved to prevent it's set by 3rd party code.
//
Status = gBS->LocateProtocol (&gEdkiiVariableLockProtocolGuid, NULL, (VOID **) &VariableLock);
if (!EFI_ERROR (Status)) {
Status = VariableLock->RequestToLock (VariableLock, L"PerfDataMemAddr", &gPerformanceProtocolGuid);
ASSERT_EFI_ERROR (Status);
}
}
}
+/**
+ Enter an infinite loop of calling the Boot Manager Menu.
+
+ This is a last resort alternative to BdsEntry() giving up for good. This
+ function never returns.
+
+ @param[in] BootManagerMenu The EFI_BOOT_MANAGER_LOAD_OPTION located and/or
+ created by the EfiBootManagerGetBootManagerMenu()
+ call in BdsEntry().
+**/
+VOID
+BdsBootManagerMenuLoop (
+ IN EFI_BOOT_MANAGER_LOAD_OPTION *BootManagerMenu
+ )
+{
+ EFI_INPUT_KEY Key;
+
+ //
+ // Normally BdsDxe does not print anything to the system console, but this is
+ // a last resort -- the end-user will likely not see any DEBUG messages
+ // logged in this situation.
+ //
+ // AsciiPrint() will NULL-check gST->ConOut internally. We check gST->ConIn
+ // here to see if it makes sense to request and wait for a keypress.
+ //
+ if (gST->ConIn != NULL) {
+ AsciiPrint (
+ "%a: No bootable option or device was found.\n"
+ "%a: Press any key to enter the Boot Manager Menu.\n",
+ gEfiCallerBaseName,
+ gEfiCallerBaseName
+ );
+ BdsWaitForSingleEvent (gST->ConIn->WaitForKey, 0);
+
+ //
+ // Drain any queued keys.
+ //
+ while (!EFI_ERROR (gST->ConIn->ReadKeyStroke (gST->ConIn, &Key))) {
+ //
+ // just throw away Key
+ //
+ }
+ }
+
+ for (;;) {
+ EfiBootManagerBoot (BootManagerMenu);
+ }
+}
+
/**
Service routine for BdsInstance->Entry(). Devices are connected, the
consoles are initialized, and the boot options are tried.
@param This Protocol Instance structure.
**/
VOID
EFIAPI
BdsEntry (
IN EFI_BDS_ARCH_PROTOCOL *This
@@ -1079,34 +1128,37 @@ BdsEntry (
}
do {
//
// Retry to boot if any of the boot succeeds
//
LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypeBoot);
BootSuccess = BootBootOptions (LoadOptions, LoadOptionCount, (BootManagerMenuStatus != EFI_NOT_FOUND) ? &BootManagerMenu : NULL);
EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
} while (BootSuccess);
}
- if (BootManagerMenuStatus != EFI_NOT_FOUND) {
- EfiBootManagerFreeLoadOption (&BootManagerMenu);
- }
-
if (!BootSuccess) {
LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypePlatformRecovery);
ProcessLoadOptions (LoadOptions, LoadOptionCount);
EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
}
+ //
+ // If BootManagerMenu is available, fall back to it indefinitely.
+ //
+ if (BootManagerMenuStatus != EFI_NOT_FOUND) {
+ BdsBootManagerMenuLoop (&BootManagerMenu);
+ }
+
DEBUG ((EFI_D_ERROR, "[Bds] Unable to boot!\n"));
CpuDeadLoop ();
}
/**
Set the variable and report the error through status code upon failure.
@param VariableName A Null-terminated string that is the name of the vendor's variable.
Each VariableName is unique for each VendorGuid. VariableName must
contain 1 or more characters. If VariableName is an empty string,
then EFI_INVALID_PARAMETER is returned.
@param VendorGuid A unique identifier for the vendor.
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/5] MdeModulePkg: introduce the EDKII_OS_LOADER_DETAIL status code payload
2017-11-22 23:58 [PATCH 0/5] MdeModulePkg, OvmfPkg: more easily visible boot progress reporting Laszlo Ersek
2017-11-22 23:58 ` [PATCH 1/5] MdeModulePkg/BdsDxe: fall back to a Boot Manager Menu loop before hanging Laszlo Ersek
@ 2017-11-22 23:58 ` Laszlo Ersek
2017-11-22 23:58 ` [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report EDKII_OS_LOADER_DETAIL status code Laszlo Ersek
` (2 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-11-22 23:58 UTC (permalink / raw)
To: edk2-devel-01
Cc: Ard Biesheuvel, Jordan Justen, Ruiyu Ni, Eric Dong, Star Zeng
The EfiBootManagerBoot() function in UefiBootManagerLib is the central
function for loading and starting Boot#### options. The library is built
into multiple drivers and applications (such as BdsDxe and UiApp),
therefore it is careful not to print anything to the system console. All
progress information is reported via DEBUG messages and progress/status
codes.
Individual platforms can customize how they handle the status codes
reported. However, the codes currently reported about LoadImage() and
StartImage() invocations are simple constants. Even if a platform displays
them on some end-user accessible device -- because end-users are generally
clueless of debug log capturing --, the information contained is
insufficient for even rudimentary bug reporting.
Introduce:
- a new, common debug code that is similar to:
- PcdProgressCodeOsLoaderLoad (LoadImage() preparation),
- EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR (LoadImage() error),
- PcdProgressCodeOsLoaderStart (StartImage() preparation),
- EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED (StartImage() error),
under which more details will be possible to convey about the same set
of events;
- a new status code payload structure (somewhat similar to
EDKII_SET_VARIABLE_STATUS from
"Include/Guid/StatusCodeDataTypeVariable.h"), for defining the wire
format of said details.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1515418
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdeModulePkg/MdeModulePkg.dec | 9 ++
MdeModulePkg/MdeModulePkg.uni | 7 ++
MdeModulePkg/Include/Guid/StatusCodeDataTypeOsLoaderDetail.h | 109 ++++++++++++++++++++
3 files changed, 125 insertions(+)
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 856d67aceb21..6ba95dedc20d 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -383,24 +383,27 @@ [Guids]
gEdkiiNonDiscoverableAmbaDeviceGuid = { 0x94440339, 0xCC93, 0x4506, {0xB4, 0xC6, 0xEE, 0x8D, 0x0F, 0x4C, 0xA1, 0x91 } }
gEdkiiNonDiscoverableEhciDeviceGuid = { 0xEAEE5615, 0x0CFD, 0x45FC, {0x87, 0x69, 0xA0, 0xD8, 0x56, 0x95, 0xAF, 0x85 } }
gEdkiiNonDiscoverableNvmeDeviceGuid = { 0xC5F25542, 0x2A79, 0x4A26, {0x81, 0xBB, 0x4E, 0xA6, 0x32, 0x33, 0xB3, 0x09 } }
gEdkiiNonDiscoverableOhciDeviceGuid = { 0xB20005B0, 0xBB2D, 0x496F, {0x86, 0x9C, 0x23, 0x0B, 0x44, 0x79, 0xE7, 0xD1 } }
gEdkiiNonDiscoverableSdhciDeviceGuid = { 0x1DD1D619, 0xF9B8, 0x463E, {0x86, 0x81, 0xD1, 0xDC, 0x7C, 0x07, 0xB7, 0x2C } }
gEdkiiNonDiscoverableUfsDeviceGuid = { 0x2EA77912, 0x80A8, 0x4947, {0xBE, 0x69, 0xCD, 0xD0, 0x0A, 0xFB, 0xE5, 0x56 } }
gEdkiiNonDiscoverableUhciDeviceGuid = { 0xA8CDA0A2, 0x4F37, 0x4A1B, {0x8E, 0x10, 0x8E, 0xF3, 0xCC, 0x3B, 0xF3, 0xA8 } }
gEdkiiNonDiscoverableXhciDeviceGuid = { 0xB1BE0BC5, 0x6C28, 0x442D, {0xAA, 0x37, 0x15, 0x1B, 0x42, 0x57, 0xBD, 0x78 } }
## Include/Guid/PlatformHasAcpi.h
gEdkiiPlatformHasAcpiGuid = { 0xf0966b41, 0xc23f, 0x41b9, { 0x96, 0x04, 0x0f, 0xf7, 0xe1, 0x11, 0x96, 0x5a } }
+ ## Include/Guid/StatusCodeDataTypeOsLoaderDetail.h
+ gEdkiiStatusCodeDataTypeOsLoaderDetailGuid = { 0xBE4904DC, 0x7EC4, 0x4167, { 0x90, 0x52, 0x38, 0x4D, 0x0B, 0x81, 0x30, 0x0B } }
+
[Ppis]
## Include/Ppi/AtaController.h
gPeiAtaControllerPpiGuid = { 0xa45e60d1, 0xc719, 0x44aa, { 0xb0, 0x7a, 0xaa, 0x77, 0x7f, 0x85, 0x90, 0x6d }}
## Include/Ppi/UsbHostController.h
gPeiUsbHostControllerPpiGuid = { 0x652B38A9, 0x77F4, 0x453F, { 0x89, 0xD5, 0xE7, 0xBD, 0xC3, 0x52, 0xFC, 0x53 }}
## Include/Ppi/Usb2HostController.h
gPeiUsb2HostControllerPpiGuid = { 0xfedd6305, 0xe2d7, 0x4ed5, { 0x9f, 0xaa, 0xda, 0x8, 0xe, 0x33, 0x6c, 0x22 }}
## Include/Ppi/UsbController.h
gPeiUsbControllerPpiGuid = { 0x3BC1F6DE, 0x693E, 0x4547, { 0xA3, 0x00, 0x21, 0x82, 0x3C, 0xA4, 0x20, 0xB2 }}
@@ -846,24 +849,30 @@ [PcdsFixedAtBuild]
## 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
+ ## Debug Code for OS Loader Detail.<BR><BR>
+ # DEBUG_CODE_OS_LOADER_DETAIL = (EFI_SOFTWARE_DXE_BS_DRIVER | (EFI_OEM_SPECIFIC | 0x00000003)) = 0x03058003<BR>
+ # @Prompt Debug Code for OS Loader Detail.
+ # @ValidList 0x80000007 | 0x03058003
+ gEfiMdeModulePkgTokenSpaceGuid.PcdDebugCodeOsLoaderDetail|0x03058003|UINT32|0x3000103B
+
## Progress Code for S3 Suspend start.<BR><BR>
# PROGRESS_CODE_S3_SUSPEND_START = (EFI_SOFTWARE_SMM_DRIVER | (EFI_OEM_SPECIFIC | 0x00000000)) = 0x03078000<BR>
# @Prompt Progress Code for S3 Suspend start.
# @ValidList 0x80000003 | 0x03078000
gEfiMdeModulePkgTokenSpaceGuid.PcdProgressCodeS3SuspendStart|0x03078000|UINT32|0x30001032
## Progress Code for S3 Suspend end.<BR><BR>
# PROGRESS_CODE_S3_SUSPEND_END = (EFI_SOFTWARE_SMM_DRIVER | (EFI_OEM_SPECIFIC | 0x00000001)) = 0x03078001<BR>
# @Prompt Progress Code for S3 Suspend end.
# @ValidList 0x80000003 | 0x03078001
gEfiMdeModulePkgTokenSpaceGuid.PcdProgressCodeS3SuspendEnd|0x03078001|UINT32|0x30001033
diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni
index 588905a9a1fa..b0872970325c 100644
--- a/MdeModulePkg/MdeModulePkg.uni
+++ b/MdeModulePkg/MdeModulePkg.uni
@@ -35,24 +35,31 @@
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdProgressCodeOsLoaderLoad_PROMPT #language en-US "Progress Code for OS Loader LoadImage start."
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdProgressCodeOsLoaderLoad_HELP #language en-US "Progress Code for OS Loader LoadImage start.<BR><BR>\n"
"PROGRESS_CODE_OS_LOADER_LOAD = (EFI_SOFTWARE_DXE_BS_DRIVER | (EFI_OEM_SPECIFIC | 0x00000000)) = 0x03058000<BR>"
#string STR_gEfiMdeModulePkgTokenSpaceGuid_ERR_80000003 #language en-US "Incorrect progress code provided."
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdProgressCodeOsLoaderStart_PROMPT #language en-US "Progress Code for OS Loader StartImage start"
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdProgressCodeOsLoaderStart_HELP #language en-US "Progress Code for OS Loader StartImage start.<BR><BR>\n"
"PROGRESS_CODE_OS_LOADER_START = (EFI_SOFTWARE_DXE_BS_DRIVER | (EFI_OEM_SPECIFIC | 0x00000001)) = 0x03058001<BR>"
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_ERR_80000007 #language en-US "Incorrect debug code provided."
+
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdDebugCodeOsLoaderDetail_PROMPT #language en-US "Debug Code for OS Loader Detail"
+
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdDebugCodeOsLoaderDetail_HELP #language en-US "Debug Code for OS Loader Detail.<BR><BR>\n"
+ "DEBUG_CODE_OS_LOADER_DETAIL = (EFI_SOFTWARE_DXE_BS_DRIVER | (EFI_OEM_SPECIFIC | 0x00000003)) = 0x03058003<BR>"
+
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdProgressCodeS3SuspendStart_PROMPT #language en-US "Progress Code for S3 Suspend start"
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdProgressCodeS3SuspendStart_HELP #language en-US "Progress Code for S3 Suspend start.<BR><BR>\n"
"PROGRESS_CODE_S3_SUSPEND_START = (EFI_SOFTWARE_SMM_DRIVER | (EFI_OEM_SPECIFIC | 0x00000000)) = 0x03078000<BR>"
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdProgressCodeS3SuspendEnd_PROMPT #language en-US "Progress Code for S3 Suspend end"
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdProgressCodeS3SuspendEnd_HELP #language en-US "Progress Code for S3 Suspend end.<BR><BR>\n"
"PROGRESS_CODE_S3_SUSPEND_END = (EFI_SOFTWARE_SMM_DRIVER | (EFI_OEM_SPECIFIC | 0x00000001)) = 0x03078001<BR>"
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdErrorCodeSetVariable_PROMPT #language en-US "Error Code for SetVariable failure"
diff --git a/MdeModulePkg/Include/Guid/StatusCodeDataTypeOsLoaderDetail.h b/MdeModulePkg/Include/Guid/StatusCodeDataTypeOsLoaderDetail.h
new file mode 100644
index 000000000000..061ecef0ad21
--- /dev/null
+++ b/MdeModulePkg/Include/Guid/StatusCodeDataTypeOsLoaderDetail.h
@@ -0,0 +1,109 @@
+/** @file
+ Define the GUID, macros, and the data structure for passing details of OS
+ Loading from the UEFI Boot Manager to the Platform, as debug codes.
+
+ Copyright (C) 2017, Red Hat, Inc.
+
+ This program and the accompanying materials are licensed and made available
+ under the terms and conditions of the BSD License that accompanies this
+ distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php.
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+ WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+**/
+
+#ifndef _STATUS_CODE_DATA_TYPE_OS_LOADER_DETAIL_H_
+#define _STATUS_CODE_DATA_TYPE_OS_LOADER_DETAIL_H_
+
+#include <Protocol/DevicePath.h>
+#include <Uefi/UefiBaseType.h>
+
+//
+// The GUID for EFI_STATUS_CODE_DATA.Type, in order to identify the trailing
+// payload as EDKII_OS_LOADER_DETAIL.
+//
+// The EFI_STATUS_CODE_VALUE under which to report such data is
+// PcdGet32(PcdDebugCodeOsLoaderDetail).
+//
+#define EDKII_STATUS_CODE_DATA_TYPE_OS_LOADER_DETAIL_GUID \
+ { \
+ 0xBE4904DC, \
+ 0x7EC4, \
+ 0x4167, \
+ { 0x90, 0x52, 0x38, 0x4D, 0x0B, 0x81, 0x30, 0x0B } \
+ }
+
+//
+// The UEFI Boot Manager is about to call gBS->LoadImage() on the boot option.
+//
+#define EDKII_OS_LOADER_DETAIL_TYPE_LOAD 0x00000000
+//
+// gBS->LoadImage() failed on the boot option.
+//
+#define EDKII_OS_LOADER_DETAIL_TYPE_LOAD_ERROR 0x00000001
+//
+// The UEFI Boot Manager is about to call gBS->StartImage() on the boot option.
+//
+#define EDKII_OS_LOADER_DETAIL_TYPE_START 0x00000002
+//
+// gBS->StartImage() failed on the boot option.
+//
+#define EDKII_OS_LOADER_DETAIL_TYPE_START_ERROR 0x00000003
+
+//
+// Structure for passing details about the above actions and results. Currently
+// a common structure is used for all of them.
+//
+// The structure can be extended compatibly by adding fields at the end. The
+// presence of such fields can be deduced from the containing
+// EFI_STATUS_CODE_DATA.Size field. Incompatible extensions require a new GUID
+// for the containing EFI_STATUS_CODE_DATA.Type field.
+//
+#pragma pack (1)
+typedef struct {
+ //
+ // EDKII_OS_LOADER_DETAIL_TYPE_* values.
+ //
+ UINT32 Type;
+ //
+ // The number of the Boot#### UEFI variable from which the OS is being
+ // loaded.
+ //
+ UINT16 BootOptionNumber;
+ //
+ // The size of Description in bytes, including the terminating L'\0'
+ // character. If DescriptionSize is 0, then Description is absent. The type
+ // of this field is UINT16 because all of EDKII_OS_LOADER_DETAIL has to fit
+ // into EFI_STATUS_CODE_DATA.Size, which has type UINT16.
+ //
+ UINT16 DescriptionSize;
+ //
+ // The size of DevicePath in bytes, including the terminating end node. If
+ // DevicePathSize is 0, then DevicePath is absent. The type of this field is
+ // UINT16 because all of EDKII_OS_LOADER_DETAIL has to fit into
+ // EFI_STATUS_CODE_DATA.Size, which has type UINT16.
+ //
+ //
+ UINT16 DevicePathSize;
+ //
+ // Used only for EDKII_OS_LOADER_DETAIL_TYPE_*_ERROR, the following field
+ // reports the failure code.
+ //
+ EFI_STATUS Status;
+ //
+ // The human-readable description of the boot option for which the OS is
+ // being loaded. Populated from EFI_LOAD_OPTION.Description.
+ //
+ // CHAR16 Description[];
+ //
+ // Describes the device and location of the OS image being loaded. Populated
+ // from the first element of the packed EFI_LOAD_OPTION.FilePathList array.
+ //
+ // EFI_DEVICE_PATH_PROTOCOL DevicePath;
+} EDKII_OS_LOADER_DETAIL;
+#pragma pack ()
+
+extern EFI_GUID gEdkiiStatusCodeDataTypeOsLoaderDetailGuid;
+
+#endif // _STATUS_CODE_DATA_TYPE_OS_LOADER_DETAIL_H_
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report EDKII_OS_LOADER_DETAIL status code
2017-11-22 23:58 [PATCH 0/5] MdeModulePkg, OvmfPkg: more easily visible boot progress reporting Laszlo Ersek
2017-11-22 23:58 ` [PATCH 1/5] MdeModulePkg/BdsDxe: fall back to a Boot Manager Menu loop before hanging Laszlo Ersek
2017-11-22 23:58 ` [PATCH 2/5] MdeModulePkg: introduce the EDKII_OS_LOADER_DETAIL status code payload Laszlo Ersek
@ 2017-11-22 23:58 ` Laszlo Ersek
2017-11-23 5:21 ` Ni, Ruiyu
2017-11-22 23:58 ` [PATCH 4/5] OvmfPkg/PlatformBootManagerLib: print EDKII_OS_LOADER_DETAIL to ConOut Laszlo Ersek
2017-11-22 23:58 ` [PATCH 5/5] OvmfPkg: disable EFI_DEBUG_CODE reporting in RELEASE builds Laszlo Ersek
4 siblings, 1 reply; 14+ messages in thread
From: Laszlo Ersek @ 2017-11-22 23:58 UTC (permalink / raw)
To: edk2-devel-01
Cc: Ard Biesheuvel, Jordan Justen, Ruiyu Ni, Eric Dong, Star Zeng
The EfiBootManagerBoot() function reports progress codes about LoadImage()
preparation and failure, and StartImage() preparation and failure. These
codes are flat (scalar) constants.
Extend this by "broadcasting" the Boot#### option number, description,
device path, and -- on load / start failure -- the error result at the
same locations, through EFI_DEBUG_CODE reporting. Use the
PcdDebugCodeOsLoaderDetail status code value and the
EDKII_OS_LOADER_DETAIL status code structure introduced earlier in this
series.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1515418
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf | 2 +
MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h | 84 ++++++++++
MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 51 +++++-
MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c | 166 ++++++++++++++++++++
4 files changed, 301 insertions(+), 2 deletions(-)
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf b/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
index ad4901db5713..633906fc6ca4 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
+++ b/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
@@ -79,24 +79,25 @@ [Guids]
## SOMETIMES_PRODUCES ## Variable:L"BootCurrent" (The boot option of current boot)
## SOMETIMES_CONSUMES ## Variable:L"BootXX" (Boot option variable)
## SOMETIMES_CONSUMES ## Variable:L"BootOrder" (The boot option array)
## SOMETIMES_CONSUMES ## Variable:L"DriverOrder" (The driver order list)
## SOMETIMES_CONSUMES ## Variable:L"ConIn" (The device path of console in device)
## SOMETIMES_CONSUMES ## Variable:L"ConOut" (The device path of console out device)
## SOMETIMES_CONSUMES ## Variable:L"ErrOut" (The device path of error out device)
gEfiGlobalVariableGuid
gPerformanceProtocolGuid ## SOMETIMES_CONSUMES ## Variable:L"PerfDataMemAddr" (The ACPI address of performance data)
gEdkiiStatusCodeDataTypeVariableGuid ## SOMETIMES_CONSUMES ## GUID
+ gEdkiiStatusCodeDataTypeOsLoaderDetailGuid ## SOMETIMES_CONSUMES ## GUID
gEfiDiskInfoAhciInterfaceGuid ## SOMETIMES_CONSUMES ## GUID
gEfiDiskInfoIdeInterfaceGuid ## SOMETIMES_CONSUMES ## GUID
gEfiDiskInfoScsiInterfaceGuid ## SOMETIMES_CONSUMES ## GUID
gEfiDiskInfoSdMmcInterfaceGuid ## SOMETIMES_CONSUMES ## GUID
[Protocols]
gEfiPciRootBridgeIoProtocolGuid ## CONSUMES
gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES
gEfiLoadFileProtocolGuid ## SOMETIMES_CONSUMES
gEfiSimpleTextOutProtocolGuid ## SOMETIMES_CONSUMES
gEfiPciIoProtocolGuid ## SOMETIMES_CONSUMES
gEfiLoadedImageProtocolGuid ## CONSUMES
@@ -112,16 +113,17 @@ [Protocols]
gEfiUsbIoProtocolGuid ## SOMETIMES_CONSUMES
gEfiNvmExpressPassThruProtocolGuid ## SOMETIMES_CONSUMES
gEfiDiskInfoProtocolGuid ## SOMETIMES_CONSUMES
gEfiDriverHealthProtocolGuid ## SOMETIMES_CONSUMES
gEfiFormBrowser2ProtocolGuid ## SOMETIMES_CONSUMES
gEfiRamDiskProtocolGuid ## SOMETIMES_CONSUMES
gEfiDeferredImageLoadProtocolGuid ## SOMETIMES_CONSUMES
[Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdProgressCodeOsLoaderLoad ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdProgressCodeOsLoaderStart ## SOMETIMES_CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdDebugCodeOsLoaderDetail ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdDriverHealthConfigureForm ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxRepairCount ## CONSUMES
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
index 0224bd34a9ed..ddcb0347aef6 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
+++ b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
@@ -44,24 +44,25 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <Protocol/BootLogo.h>
#include <Protocol/DriverHealth.h>
#include <Protocol/FormBrowser2.h>
#include <Protocol/VariableLock.h>
#include <Protocol/RamDisk.h>
#include <Protocol/DeferredImageLoad.h>
#include <Guid/MemoryTypeInformation.h>
#include <Guid/FileInfo.h>
#include <Guid/GlobalVariable.h>
#include <Guid/Performance.h>
#include <Guid/StatusCodeDataTypeVariable.h>
+#include <Guid/StatusCodeDataTypeOsLoaderDetail.h>
#include <Library/PrintLib.h>
#include <Library/DebugLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiRuntimeServicesTableLib.h>
#include <Library/UefiLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/DxeServicesTableLib.h>
#include <Library/HobLib.h>
#include <Library/BaseLib.h>
#include <Library/DevicePathLib.h>
@@ -298,24 +299,107 @@ BmStopHotkeyService (
@retval EFI_NOT_FOUND The variable trying to be updated or deleted was not found.
**/
EFI_STATUS
BmSetVariableAndReportStatusCodeOnError (
IN CHAR16 *VariableName,
IN EFI_GUID *VendorGuid,
IN UINT32 Attributes,
IN UINTN DataSize,
IN VOID *Data
);
+/**
+ Dynamically allocate and initialize an EDKII_OS_LOADER_DETAIL status code
+ payload.
+
+ @param[in] BootOption Capture the OptionNumber, FilePath and
+ Description fields of BootOption in the
+ EDKII_OS_LOADER_DETAIL payload.
+
+ @param[out] OsLoaderDetail On successful return, set to the
+ EDKII_OS_LOADER_DETAIL object that has been
+ allocated and initialized. On failure, not
+ modified.
+
+ @param[out] OsLoaderDetailSize On successful return, set to the size of the
+ EDKII_OS_LOADER_DETAIL object that has been
+ allocated and initialized. On failure, not
+ modified.
+
+ @retval EFI_UNSUPPORTED EFI_DEBUG_CODE reporting is disabled in the
+ platform.
+
+ @retval EFI_INVALID_PARAMETER BootOption->OptionNumber is not in
+ 0x0000..0xFFFF, inclusive.
+
+ @retval EFI_BAD_BUFFER_SIZE BootOption->FilePath and/or
+ BootOption->Description would exceed the
+ UINT16 size limits presented by
+ EDKII_OS_LOADER_DETAIL or
+ EFI_STATUS_CODE_DATA.
+
+ @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
+
+ @retval EFI_SUCCESS The EDKII_OS_LOADER_DETAIL object has been
+ allocated and initialized. The caller is
+ responsible for freeing the object with
+ FreePool().
+**/
+EFI_STATUS
+BmCreateOsLoaderDetail (
+ IN CONST EFI_BOOT_MANAGER_LOAD_OPTION *BootOption,
+ OUT EDKII_OS_LOADER_DETAIL **OsLoaderDetail,
+ OUT UINTN *OsLoaderDetailSize
+ );
+
+/**
+ Report an EFI_DEBUG_CODE status code with EDKII_OS_LOADER_DETAIL as payload
+ (i.e., extended data).
+
+ @param[in,out] OsLoaderDetail The EDKII_OS_LOADER_DETAIL payload previously
+ created with BmCreateOsLoaderDetail(), and
+ modified zero or more times by
+ BmReportOsLoaderDetail(). If OsLoaderDetail is
+ NULL, the function does nothing. Otherwise,
+ the Type and Status fields are overwritten in
+ OsLoaderDetail, and a status code is reported.
+
+ @param[in] OsLoaderDetailSize The size returned by BmCreateOsLoaderDetail().
+ If OsLoaderDetail is NULL, OsLoaderDetailSize
+ may be zero.
+
+ @param[in] DetailType OsLoaderDetail->Type is set to DetailType
+ before reporting the status code. The caller
+ is responsible for passing an
+ EDKII_OS_LOADER_DETAIL_TYPE_* value.
+
+ @param[in] DetailStatus OsLoaderDetail->Status is set to DetailStatus
+ before reporting the status code.
+
+ @retval EFI_UNSUPPORTED EFI_DEBUG_CODE reporting is disabled in the
+ platform.
+
+ @retval EFI_ABORTED OsLoaderDetail is NULL.
+
+ @return Values propagated from REPORT_STATUS_CODE_EX().
+**/
+EFI_STATUS
+BmReportOsLoaderDetail (
+ IN OUT EDKII_OS_LOADER_DETAIL *OsLoaderDetail OPTIONAL,
+ IN UINTN OsLoaderDetailSize,
+ IN UINT32 DetailType,
+ IN EFI_STATUS DetailStatus
+ );
+
/**
Function compares a device path data structure to that of all the nodes of a
second device path instance.
@param Multi A pointer to a multi-instance device path data
structure.
@param Single A pointer to a single-instance device path data
structure.
@retval TRUE If the Single device path is contained within Multi device path.
@retval FALSE The Single device path is not match within Multi device path.
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
index d6844823aa55..049afbf7d1f9 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
@@ -1665,34 +1665,39 @@ EfiBootManagerBoot (
EFI_STATUS Status;
EFI_HANDLE ImageHandle;
EFI_LOADED_IMAGE_PROTOCOL *ImageInfo;
UINT16 Uint16;
UINTN OptionNumber;
UINTN OriginalOptionNumber;
EFI_DEVICE_PATH_PROTOCOL *FilePath;
EFI_DEVICE_PATH_PROTOCOL *RamDiskDevicePath;
VOID *FileBuffer;
UINTN FileSize;
EFI_BOOT_LOGO_PROTOCOL *BootLogo;
EFI_EVENT LegacyBootEvent;
+ EDKII_OS_LOADER_DETAIL *OsLoaderDetail;
+ UINTN OsLoaderDetailSize;
if (BootOption == NULL) {
return;
}
if (BootOption->FilePath == NULL || BootOption->OptionType != LoadOptionTypeBoot) {
BootOption->Status = EFI_INVALID_PARAMETER;
return;
}
+ OsLoaderDetail = NULL;
+ OsLoaderDetailSize = 0;
+
//
// 1. Create Boot#### for a temporary boot if there is no match Boot#### (i.e. a boot by selected a EFI Shell using "Boot From File")
//
OptionNumber = BmFindBootOptionInVariable (BootOption);
if (OptionNumber == LoadOptionNumberUnassigned) {
Status = BmGetFreeOptionNumber (LoadOptionTypeBoot, &Uint16);
if (!EFI_ERROR (Status)) {
//
// Save the BootOption->OptionNumber to restore later
//
OptionNumber = Uint16;
OriginalOptionNumber = BootOption->OptionNumber;
@@ -1751,68 +1756,93 @@ EfiBootManagerBoot (
//
// 6. Load EFI boot option to ImageHandle
//
DEBUG_CODE_BEGIN ();
if (BootOption->Description == NULL) {
DEBUG ((DEBUG_INFO | DEBUG_LOAD, "[Bds]Booting from unknown device path\n"));
} else {
DEBUG ((DEBUG_INFO | DEBUG_LOAD, "[Bds]Booting %s\n", BootOption->Description));
}
DEBUG_CODE_END ();
+ //
+ // Try to create the status code payload structure for detailed debug
+ // reporting.
+ //
+ Status = BmCreateOsLoaderDetail (BootOption, &OsLoaderDetail,
+ &OsLoaderDetailSize);
+ if (EFI_ERROR (Status) && (Status != EFI_UNSUPPORTED)) {
+ DEBUG ((DEBUG_WARN | DEBUG_LOAD, "[Bds]BmCreateOsLoaderDetail(): %r\n",
+ Status));
+ }
+
ImageHandle = NULL;
RamDiskDevicePath = NULL;
if (DevicePathType (BootOption->FilePath) != BBS_DEVICE_PATH) {
Status = EFI_NOT_FOUND;
FilePath = NULL;
EfiBootManagerConnectDevicePath (BootOption->FilePath, NULL);
FileBuffer = BmGetNextLoadOptionBuffer (LoadOptionTypeBoot, BootOption->FilePath, &FilePath, &FileSize);
if (FileBuffer != NULL) {
RamDiskDevicePath = BmGetRamDiskDevicePath (FilePath);
REPORT_STATUS_CODE (EFI_PROGRESS_CODE, PcdGet32 (PcdProgressCodeOsLoaderLoad));
+ BmReportOsLoaderDetail (
+ OsLoaderDetail,
+ OsLoaderDetailSize,
+ EDKII_OS_LOADER_DETAIL_TYPE_LOAD,
+ 0 // DetailStatus -- unused here
+ );
+
Status = gBS->LoadImage (
TRUE,
gImageHandle,
FilePath,
FileBuffer,
FileSize,
&ImageHandle
);
}
if (FileBuffer != NULL) {
FreePool (FileBuffer);
}
if (FilePath != NULL) {
FreePool (FilePath);
}
if (EFI_ERROR (Status)) {
//
// Report Status Code to indicate that the failure to load boot option
//
REPORT_STATUS_CODE (
EFI_ERROR_CODE | EFI_ERROR_MINOR,
(EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR)
);
+ BmReportOsLoaderDetail (
+ OsLoaderDetail,
+ OsLoaderDetailSize,
+ EDKII_OS_LOADER_DETAIL_TYPE_LOAD_ERROR,
+ Status
+ );
+
BootOption->Status = Status;
//
// Destroy the RAM disk
//
if (RamDiskDevicePath != NULL) {
BmDestroyRamDisk (RamDiskDevicePath);
FreePool (RamDiskDevicePath);
}
- return;
+ goto FreeOsLoaderDetail;
}
}
//
// Check to see if we should legacy BOOT. If yes then do the legacy boot
// Write boot to OS performance data for Legacy boot
//
if ((DevicePathType (BootOption->FilePath) == BBS_DEVICE_PATH) && (DevicePathSubType (BootOption->FilePath) == BBS_BBS_DP)) {
if (mBmLegacyBoot != NULL) {
//
// Write boot to OS performance data for legacy boot.
//
@@ -1826,25 +1856,25 @@ EfiBootManagerBoot (
NULL,
&LegacyBootEvent
);
ASSERT_EFI_ERROR (Status);
);
mBmLegacyBoot (BootOption);
} else {
BootOption->Status = EFI_UNSUPPORTED;
}
PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber);
- return;
+ goto FreeOsLoaderDetail;
}
//
// Provide the image with its load options
//
Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid, (VOID **) &ImageInfo);
ASSERT_EFI_ERROR (Status);
if (!BmIsAutoCreateBootOption (BootOption)) {
ImageInfo->LoadOptionsSize = BootOption->OptionalDataSize;
ImageInfo->LoadOptions = BootOption->OptionalData;
}
@@ -1858,36 +1888,48 @@ EfiBootManagerBoot (
// Before calling the image, enable the Watchdog Timer for 5 minutes period
//
gBS->SetWatchdogTimer (5 * 60, 0x0000, 0x00, NULL);
//
// Write boot to OS performance data for UEFI boot
//
PERF_CODE (
BmWriteBootToOsPerformanceData (NULL, NULL);
);
REPORT_STATUS_CODE (EFI_PROGRESS_CODE, PcdGet32 (PcdProgressCodeOsLoaderStart));
+ BmReportOsLoaderDetail (
+ OsLoaderDetail,
+ OsLoaderDetailSize,
+ EDKII_OS_LOADER_DETAIL_TYPE_START,
+ 0 // DetailStatus -- unused here
+ );
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 (
EFI_ERROR_CODE | EFI_ERROR_MINOR,
(EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED)
);
+ BmReportOsLoaderDetail (
+ OsLoaderDetail,
+ OsLoaderDetailSize,
+ EDKII_OS_LOADER_DETAIL_TYPE_START_ERROR,
+ Status
+ );
}
PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber);
//
// Destroy the RAM disk
//
if (RamDiskDevicePath != NULL) {
BmDestroyRamDisk (RamDiskDevicePath);
FreePool (RamDiskDevicePath);
}
//
@@ -1912,24 +1954,29 @@ EfiBootManagerBoot (
L"BootCurrent",
&gEfiGlobalVariableGuid,
0,
0,
NULL
);
//
// Deleting variable with current variable implementation shouldn't fail.
// When BootXXXX (e.g.: BootManagerMenu) boots BootYYYY, exiting BootYYYY causes BootCurrent deleted,
// exiting BootXXXX causes deleting BootCurrent returns EFI_NOT_FOUND.
//
ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_FOUND);
+
+FreeOsLoaderDetail:
+ if (OsLoaderDetail != NULL) {
+ FreePool (OsLoaderDetail);
+ }
}
/**
Check whether there is a instance in BlockIoDevicePath, which contain multi device path
instances, has the same partition node with HardDriveDevicePath device path
@param BlockIoDevicePath Multi device path instances which need to check
@param HardDriveDevicePath A device path which starts with a hard drive media
device path.
@retval TRUE There is a matched device path instance.
@retval FALSE There is no matched device path instance.
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
index 81d365940043..29da896854b8 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
@@ -369,24 +369,190 @@ BmSetVariableAndReportStatusCodeOnError (
SetVariableStatus,
sizeof (EDKII_SET_VARIABLE_STATUS) + NameSize + DataSize
);
FreePool (SetVariableStatus);
}
}
return Status;
}
+/**
+ Dynamically allocate and initialize an EDKII_OS_LOADER_DETAIL status code
+ payload.
+
+ @param[in] BootOption Capture the OptionNumber, FilePath and
+ Description fields of BootOption in the
+ EDKII_OS_LOADER_DETAIL payload.
+
+ @param[out] OsLoaderDetail On successful return, set to the
+ EDKII_OS_LOADER_DETAIL object that has been
+ allocated and initialized. On failure, not
+ modified.
+
+ @param[out] OsLoaderDetailSize On successful return, set to the size of the
+ EDKII_OS_LOADER_DETAIL object that has been
+ allocated and initialized. On failure, not
+ modified.
+
+ @retval EFI_UNSUPPORTED EFI_DEBUG_CODE reporting is disabled in the
+ platform.
+
+ @retval EFI_INVALID_PARAMETER BootOption->OptionNumber is not in
+ 0x0000..0xFFFF, inclusive.
+
+ @retval EFI_BAD_BUFFER_SIZE BootOption->FilePath and/or
+ BootOption->Description would exceed the
+ UINT16 size limits presented by
+ EDKII_OS_LOADER_DETAIL or
+ EFI_STATUS_CODE_DATA.
+
+ @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
+
+ @retval EFI_SUCCESS The EDKII_OS_LOADER_DETAIL object has been
+ allocated and initialized. The caller is
+ responsible for freeing the object with
+ FreePool().
+**/
+EFI_STATUS
+BmCreateOsLoaderDetail (
+ IN CONST EFI_BOOT_MANAGER_LOAD_OPTION *BootOption,
+ OUT EDKII_OS_LOADER_DETAIL **OsLoaderDetail,
+ OUT UINTN *OsLoaderDetailSize
+ )
+{
+ UINTN DescriptionSize;
+ UINTN DevicePathSize;
+ UINTN PayloadSize;
+ EDKII_OS_LOADER_DETAIL *Payload;
+ UINT8 *VariableSizeData;
+
+ if (!ReportDebugCodeEnabled ()) {
+ return EFI_UNSUPPORTED;
+ }
+
+ if (BootOption->OptionNumber >= LoadOptionNumberMax) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ DescriptionSize = (BootOption->Description == NULL) ?
+ 0 :
+ StrSize (BootOption->Description);
+ DevicePathSize = GetDevicePathSize (BootOption->FilePath);
+ PayloadSize = sizeof *Payload + DescriptionSize + DevicePathSize;
+
+ if (DescriptionSize > MAX_UINT16 ||
+ DevicePathSize > MAX_UINT16 ||
+ PayloadSize > MAX_UINT16) {
+ return EFI_BAD_BUFFER_SIZE;
+ }
+
+ Payload = AllocateZeroPool (PayloadSize);
+ if (Payload == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+ VariableSizeData = (UINT8 *)(Payload + 1);
+
+ //
+ // Populate the variable size fields at the end of the payload.
+ //
+ CopyMem (VariableSizeData, BootOption->Description, DescriptionSize);
+ VariableSizeData += DescriptionSize;
+
+ CopyMem (VariableSizeData, BootOption->FilePath, DevicePathSize);
+ VariableSizeData += DevicePathSize;
+
+ ASSERT (VariableSizeData - (UINT8 *)Payload == PayloadSize);
+
+ //
+ // Populate the fixed fields in the payload. Any members not listed below
+ // remain zero-filled at this point.
+ //
+ Payload->BootOptionNumber = (UINT16)BootOption->OptionNumber;
+ Payload->DescriptionSize = (UINT16)DescriptionSize;
+ Payload->DevicePathSize = (UINT16)DevicePathSize;
+
+ *OsLoaderDetail = Payload;
+ *OsLoaderDetailSize = PayloadSize;
+ return EFI_SUCCESS;
+}
+
+
+/**
+ Report an EFI_DEBUG_CODE status code with EDKII_OS_LOADER_DETAIL as payload
+ (i.e., extended data).
+
+ @param[in,out] OsLoaderDetail The EDKII_OS_LOADER_DETAIL payload previously
+ created with BmCreateOsLoaderDetail(), and
+ modified zero or more times by
+ BmReportOsLoaderDetail(). If OsLoaderDetail is
+ NULL, the function does nothing. Otherwise,
+ the Type and Status fields are overwritten in
+ OsLoaderDetail, and a status code is reported.
+
+ @param[in] OsLoaderDetailSize The size returned by BmCreateOsLoaderDetail().
+ If OsLoaderDetail is NULL, OsLoaderDetailSize
+ may be zero.
+
+ @param[in] DetailType OsLoaderDetail->Type is set to DetailType
+ before reporting the status code. The caller
+ is responsible for passing an
+ EDKII_OS_LOADER_DETAIL_TYPE_* value.
+
+ @param[in] DetailStatus OsLoaderDetail->Status is set to DetailStatus
+ before reporting the status code.
+
+ @retval EFI_UNSUPPORTED EFI_DEBUG_CODE reporting is disabled in the
+ platform.
+
+ @retval EFI_ABORTED OsLoaderDetail is NULL.
+
+ @return Values propagated from REPORT_STATUS_CODE_EX().
+**/
+EFI_STATUS
+BmReportOsLoaderDetail (
+ IN OUT EDKII_OS_LOADER_DETAIL *OsLoaderDetail OPTIONAL,
+ IN UINTN OsLoaderDetailSize,
+ IN UINT32 DetailType,
+ IN EFI_STATUS DetailStatus
+ )
+{
+ EFI_STATUS Status;
+
+ if (!ReportDebugCodeEnabled ()) {
+ return EFI_UNSUPPORTED;
+ }
+
+ if (OsLoaderDetail == NULL) {
+ return EFI_ABORTED;
+ }
+
+ OsLoaderDetail->Type = DetailType;
+ OsLoaderDetail->Status = DetailStatus;
+
+ Status = REPORT_STATUS_CODE_EX (
+ EFI_DEBUG_CODE, // Type
+ PcdGet32 (PcdDebugCodeOsLoaderDetail), // Value
+ 0, // Instance
+ &gEfiCallerIdGuid, // CallerId
+ &gEdkiiStatusCodeDataTypeOsLoaderDetailGuid, // ExtendedDataGuid
+ OsLoaderDetail, // ExtendedData
+ OsLoaderDetailSize // ExtendedDataSize
+ );
+ return Status;
+}
+
+
/**
Print the device path info.
@param DevicePath The device path need to print.
**/
VOID
BmPrintDp (
EFI_DEVICE_PATH_PROTOCOL *DevicePath
)
{
CHAR16 *Str;
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/5] OvmfPkg/PlatformBootManagerLib: print EDKII_OS_LOADER_DETAIL to ConOut
2017-11-22 23:58 [PATCH 0/5] MdeModulePkg, OvmfPkg: more easily visible boot progress reporting Laszlo Ersek
` (2 preceding siblings ...)
2017-11-22 23:58 ` [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report EDKII_OS_LOADER_DETAIL status code Laszlo Ersek
@ 2017-11-22 23:58 ` Laszlo Ersek
2017-11-22 23:58 ` [PATCH 5/5] OvmfPkg: disable EFI_DEBUG_CODE reporting in RELEASE builds Laszlo Ersek
4 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-11-22 23:58 UTC (permalink / raw)
To: edk2-devel-01
Cc: Ard Biesheuvel, Jordan Justen, Ruiyu Ni, Anthony Perard,
Julien Grall
Parse and print the EDKII_OS_LOADER_DETAIL debug codes from
UefiBootManagerLib (when it acts as part of BdsDxe -- not as part of
UiApp, for example). In effect this displays LoadImage() and StartImage()
attempts and failures on the splash screen, visibly to end-users.
While at it, print two other (earlier) console messages about boot option
generation and boot option filtering / reordering.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien.grall@linaro.org>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1515418
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
We can port this later to ArmVirtPkg. For that, first we'll have to
replace commit 59541d41633c ("ArmVirtPkg: remove status code support",
2017-07-05) with a port of commit a6d594c5fabd ("OvmfPkg: use StatusCode
Router and Handler from MdeModulePkg", 2016-08-03).
OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 4 +
OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h | 15 +
OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 8 +
OvmfPkg/Library/PlatformBootManagerLib/StatusCodeHandler.c | 298 ++++++++++++++++++++
4 files changed, 325 insertions(+)
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index 27789b7377bc..36901fc39d95 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
@@ -21,24 +21,25 @@ [Defines]
LIBRARY_CLASS = PlatformBootManagerLib|DXE_DRIVER
#
# The following information is for reference only and not required by the build tools.
#
# VALID_ARCHITECTURES = IA32 X64 IPF EBC
#
[Sources]
BdsPlatform.c
PlatformData.c
QemuKernel.c
+ StatusCodeHandler.c
BdsPlatform.h
[Packages]
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec
SourceLevelDebugPkg/SourceLevelDebugPkg.dec
OvmfPkg/OvmfPkg.dec
[LibraryClasses]
BaseLib
MemoryAllocationLib
@@ -54,28 +55,31 @@ [LibraryClasses]
QemuFwCfgLib
QemuFwCfgS3Lib
LoadLinuxLib
QemuBootOrderLib
UefiLib
[Pcd]
gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut
gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile
+ gEfiMdeModulePkgTokenSpaceGuid.PcdDebugCodeOsLoaderDetail
[Pcd.IA32, Pcd.X64]
gEfiMdePkgTokenSpaceGuid.PcdFSBClock
[Protocols]
gEfiDecompressProtocolGuid
gEfiPciRootBridgeIoProtocolGuid
gEfiS3SaveStateProtocolGuid # PROTOCOL SOMETIMES_CONSUMED
gEfiDxeSmmReadyToLockProtocolGuid # PROTOCOL SOMETIMES_PRODUCED
gEfiLoadedImageProtocolGuid # PROTOCOL SOMETIMES_PRODUCED
gEfiFirmwareVolume2ProtocolGuid # PROTOCOL SOMETIMES_CONSUMED
+ gEfiRscHandlerProtocolGuid # PROTOCOL SOMETIMES_CONSUMED
[Guids]
gEfiXenInfoGuid
gEfiEndOfDxeEventGroupGuid
gRootBridgesConnectedEventGroupGuid
+ gEdkiiStatusCodeDataTypeOsLoaderDetailGuid
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
index 97ffbb514825..493cfee85f54 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
@@ -183,13 +183,28 @@ PlatformInitializeConsole (
/**
Loads and boots UEFI Linux via the FwCfg interface.
@retval EFI_NOT_FOUND - The Linux kernel was not found
**/
EFI_STATUS
TryRunningQemuKernel (
VOID
);
+/**
+ Register a status code handler for printing EDKII_OS_LOADER_DETAIL reports to
+ the console.
+
+ @retval EFI_SUCCESS The status code handler has been successfully
+ registered.
+
+ @return Error codes propagated from boot services and from
+ EFI_RSC_HANDLER_PROTOCOL.
+**/
+EFI_STATUS
+RegisterStatusCodeHandler (
+ VOID
+ );
+
#endif // _PLATFORM_SPECIFIC_BDS_PLATFORM_H_
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index 025252e72b39..429f2926d2af 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -1454,35 +1454,43 @@ Routine Description:
BootLogoEnableLogo ();
//
// Perform some platform specific connect sequence
//
PlatformBdsConnectSequence ();
//
// Process QEMU's -kernel command line option
//
TryRunningQemuKernel ();
+ AsciiPrint (
+ "%a: auto-generating trailing boot options for bootable devices\n",
+ gEfiCallerBaseName
+ );
EfiBootManagerRefreshAllBootOption ();
//
// Register UEFI Shell
//
PlatformRegisterFvBootOption (
PcdGetPtr (PcdShellFile), L"EFI Internal Shell", LOAD_OPTION_ACTIVE
);
+ AsciiPrint ("%a: filtering and reordering boot options\n",
+ gEfiCallerBaseName);
RemoveStaleFvFileOptions ();
SetBootOrderFromQemu ();
+
+ RegisterStatusCodeHandler ();
}
/**
This notification function is invoked when an instance of the
EFI_DEVICE_PATH_PROTOCOL is produced.
@param Event The event that occurred
@param Context For EFI compatibility. Not used.
**/
VOID
EFIAPI
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/StatusCodeHandler.c b/OvmfPkg/Library/PlatformBootManagerLib/StatusCodeHandler.c
new file mode 100644
index 000000000000..cec4fb6ed6ce
--- /dev/null
+++ b/OvmfPkg/Library/PlatformBootManagerLib/StatusCodeHandler.c
@@ -0,0 +1,298 @@
+/** @file
+ Register a status code handler for printing EDKII_OS_LOADER_DETAIL reports to
+ the console.
+
+ This feature enables users that are not accustomed to analyzing the OVMF
+ debug log to glean some information about UEFI boot option processing
+ (loading and starting).
+
+ Copyright (C) 2017, Red Hat, Inc.
+
+ This program and the accompanying materials are licensed and made available
+ under the terms and conditions of the BSD License which accompanies this
+ distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+ WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+**/
+
+#include <Guid/StatusCodeDataTypeOsLoaderDetail.h>
+#include <Protocol/ReportStatusCodeHandler.h>
+
+#include "BdsPlatform.h"
+
+
+/**
+ Handle status codes reported through ReportStatusCodeLib /
+ EFI_STATUS_CODE_PROTOCOL.ReportStatusCode(). Format matching status codes to
+ the system console.
+
+ The highest TPL at which this handler can be registered with
+ EFI_RSC_HANDLER_PROTOCOL.Register() is TPL_NOTIFY. That's because
+ AsciiPrint() uses EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL internally.
+
+ The parameter list of this function precisely matches that of
+ EFI_STATUS_CODE_PROTOCOL.ReportStatusCode().
+
+ The return status of this function is ignored by the caller, but the function
+ still returns sensible codes:
+
+ @retval EFI_SUCCESS The status code has been processed; either as
+ a no-op, due to filtering, or by formatting it
+ to the system console.
+
+ @retval EFI_INVALID_PARAMETER Unknown or malformed contents have been
+ detected in EFI_STATUS_CODE_DATA, or in the
+ EDKII_OS_LOADER_DETAIL payload within
+ EFI_STATUS_CODE_DATA.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+HandleStatusCode (
+ IN EFI_STATUS_CODE_TYPE CodeType,
+ IN EFI_STATUS_CODE_VALUE Value,
+ IN UINT32 Instance,
+ IN EFI_GUID *CallerId,
+ IN EFI_STATUS_CODE_DATA *Data
+ )
+{
+ EDKII_OS_LOADER_DETAIL *OsLoaderDetail;
+ UINT8 *VariableSizeData;
+ CHAR16 *Description;
+ EFI_DEVICE_PATH_PROTOCOL *DevicePath;
+ BOOLEAN DevPathStringIsDynamic;
+ CHAR16 *DevPathString;
+ EFI_STATUS Status;
+
+ //
+ // Ignore all status codes other than OsLoaderDetail.
+ //
+ if (Value != PcdGet32 (PcdDebugCodeOsLoaderDetail)) {
+ return EFI_SUCCESS;
+ }
+
+ //
+ // The status codes we are interested in are emitted by UefiBootManagerLib.
+ // UefiBootManagerLib is built into several drivers and applications, e.g.
+ // BdsDxe and UiApp. Process (i.e., print to the console) only those status
+ // codes that come from BdsDxe; that is, from the driver module that this
+ // PlatformBootManagerLib instance is also built into.
+ //
+ if (!CompareGuid (CallerId, &gEfiCallerIdGuid)) {
+ return EFI_SUCCESS;
+ }
+
+ //
+ // Sanity checks -- now that Value has been validated, we have expectations
+ // to enforce.
+ //
+ if ((Data == NULL) ||
+ (Data->HeaderSize < sizeof *Data) ||
+ (Data->Size < sizeof *OsLoaderDetail) ||
+ (!CompareGuid (&Data->Type,
+ &gEdkiiStatusCodeDataTypeOsLoaderDetailGuid))) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "%a: unknown or malformed data for status code 0x%x\n",
+ __FUNCTION__,
+ PcdGet32 (PcdDebugCodeOsLoaderDetail)
+ ));
+ return EFI_INVALID_PARAMETER;
+ }
+
+ OsLoaderDetail = (EDKII_OS_LOADER_DETAIL *)(
+ (UINT8 *)Data + Data->HeaderSize
+ );
+
+ //
+ // More sanity checks. The additions on the RHS are carried out in UINTN and
+ // cannot overflow.
+ //
+ if (Data->Size < (sizeof *OsLoaderDetail +
+ OsLoaderDetail->DescriptionSize +
+ OsLoaderDetail->DevicePathSize)) {
+ DEBUG ((DEBUG_ERROR, "%a: malformed EDKII_OS_LOADER_DETAIL\n",
+ __FUNCTION__));
+ return EFI_INVALID_PARAMETER;
+ }
+
+ //
+ // Extract the known variable size fields from the payload.
+ //
+ VariableSizeData = (UINT8 *)(OsLoaderDetail + 1);
+
+ Description = (CHAR16 *)VariableSizeData;
+ VariableSizeData += OsLoaderDetail->DescriptionSize;
+
+ DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)VariableSizeData;
+ VariableSizeData += OsLoaderDetail->DevicePathSize;
+
+ ASSERT (VariableSizeData - (UINT8 *)OsLoaderDetail <= Data->Size);
+
+ //
+ // Prepare the extracted variable size fields for printing.
+ //
+ if (OsLoaderDetail->DescriptionSize == 0) {
+ Description = L"<no description available>";
+ }
+
+ DevPathStringIsDynamic = FALSE;
+ if (OsLoaderDetail->DevicePathSize == 0) {
+ DevPathString = L"<no device path available>";
+ } else {
+ DevPathString = ConvertDevicePathToText (
+ DevicePath,
+ FALSE, // DisplayOnly
+ FALSE // AllowShortcuts
+ );
+ if (DevPathString == NULL) {
+ DevPathString = L"<out of memory while formatting device path>";
+ } else {
+ DevPathStringIsDynamic = TRUE;
+ }
+ }
+
+ //
+ // Print the message to the console.
+ //
+ switch (OsLoaderDetail->Type) {
+ case EDKII_OS_LOADER_DETAIL_TYPE_LOAD:
+ case EDKII_OS_LOADER_DETAIL_TYPE_START:
+ AsciiPrint (
+ "%a: %a Boot%04x \"%s\" from %s\n",
+ gEfiCallerBaseName,
+ (OsLoaderDetail->Type == EDKII_OS_LOADER_DETAIL_TYPE_LOAD ?
+ "loading" :
+ "starting"),
+ OsLoaderDetail->BootOptionNumber,
+ Description,
+ DevPathString
+ );
+ break;
+
+ case EDKII_OS_LOADER_DETAIL_TYPE_LOAD_ERROR:
+ case EDKII_OS_LOADER_DETAIL_TYPE_START_ERROR:
+ AsciiPrint (
+ "%a: failed to %a Boot%04x \"%s\" from %s: %r\n",
+ gEfiCallerBaseName,
+ (OsLoaderDetail->Type == EDKII_OS_LOADER_DETAIL_TYPE_LOAD_ERROR ?
+ "load" :
+ "start"),
+ OsLoaderDetail->BootOptionNumber,
+ Description,
+ DevPathString,
+ OsLoaderDetail->Status
+ );
+ break;
+
+ default:
+ DEBUG ((DEBUG_ERROR, "%a: unknown EDKII_OS_LOADER_DETAIL.Type 0x%x\n",
+ __FUNCTION__, OsLoaderDetail->Type));
+ Status = EFI_INVALID_PARAMETER;
+ goto ReleaseDevPathString;
+ }
+
+ Status = EFI_SUCCESS;
+
+ReleaseDevPathString:
+ if (DevPathStringIsDynamic) {
+ FreePool (DevPathString);
+ }
+ return Status;
+}
+
+
+/**
+ Unregister HandleStatusCode() at ExitBootServices().
+
+ (See EFI_RSC_HANDLER_PROTOCOL in Volume 3 of the Platform Init spec.)
+
+ @param[in] Event Event whose notification function is being invoked.
+
+ @param[in] Context Pointer to EFI_RSC_HANDLER_PROTOCOL, originally looked up
+ when HandleStatusCode() was registered.
+**/
+STATIC
+VOID
+EFIAPI
+UnregisterAtExitBootServices (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ EFI_RSC_HANDLER_PROTOCOL *StatusCodeRouter;
+
+ StatusCodeRouter = Context;
+ StatusCodeRouter->Unregister (HandleStatusCode);
+}
+
+
+/**
+ Register a status code handler for printing EDKII_OS_LOADER_DETAIL reports to
+ the console.
+
+ @retval EFI_SUCCESS The status code handler has been successfully
+ registered.
+
+ @return Error codes propagated from boot services and from
+ EFI_RSC_HANDLER_PROTOCOL.
+**/
+EFI_STATUS
+RegisterStatusCodeHandler (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ EFI_RSC_HANDLER_PROTOCOL *StatusCodeRouter;
+ EFI_EVENT ExitBootEvent;
+
+ Status = gBS->LocateProtocol (&gEfiRscHandlerProtocolGuid,
+ NULL /* Registration */, (VOID **)&StatusCodeRouter);
+ //
+ // This protocol is provided by the ReportStatusCodeRouterRuntimeDxe driver
+ // that we build into the firmware image. Given that PlatformBootManagerLib
+ // is used as part of BdsDxe, and BDS Entry occurs after all DXE drivers have
+ // been dispatched, the EFI_RSC_HANDLER_PROTOCOL is available at this point.
+ //
+ ASSERT_EFI_ERROR (Status);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // Register the handler callback.
+ //
+ Status = StatusCodeRouter->Register (HandleStatusCode, TPL_CALLBACK);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: failed to register status code handler: %r\n",
+ __FUNCTION__, Status));
+ return Status;
+ }
+
+ //
+ // Status code reporting and routing/handling extend into OS runtime. Since
+ // we don't want our handler to survive the BDS phase, we have to unregister
+ // the callback at ExitBootServices(). (See EFI_RSC_HANDLER_PROTOCOL in
+ // Volume 3 of the Platform Init spec.)
+ //
+ Status = gBS->CreateEvent (
+ EVT_SIGNAL_EXIT_BOOT_SERVICES, // Type
+ TPL_CALLBACK, // NotifyTpl
+ UnregisterAtExitBootServices, // NotifyFunction
+ StatusCodeRouter, // NotifyContext
+ &ExitBootEvent // Event
+ );
+ if (EFI_ERROR (Status)) {
+ //
+ // We have to unregister the callback right now, and fail the function.
+ //
+ DEBUG ((DEBUG_ERROR, "%a: failed to create ExitBootServices() event: %r\n",
+ __FUNCTION__, Status));
+ StatusCodeRouter->Unregister (HandleStatusCode);
+ return Status;
+ }
+
+ return EFI_SUCCESS;
+}
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 5/5] OvmfPkg: disable EFI_DEBUG_CODE reporting in RELEASE builds
2017-11-22 23:58 [PATCH 0/5] MdeModulePkg, OvmfPkg: more easily visible boot progress reporting Laszlo Ersek
` (3 preceding siblings ...)
2017-11-22 23:58 ` [PATCH 4/5] OvmfPkg/PlatformBootManagerLib: print EDKII_OS_LOADER_DETAIL to ConOut Laszlo Ersek
@ 2017-11-22 23:58 ` Laszlo Ersek
4 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-11-22 23:58 UTC (permalink / raw)
To: edk2-devel-01
Cc: Ard Biesheuvel, Jordan Justen, Ruiyu Ni, Anthony Perard,
Julien Grall
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien.grall@linaro.org>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1515418
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/OvmfPkgIa32.dsc | 7 +++++++
OvmfPkg/OvmfPkgIa32X64.dsc | 7 +++++++
OvmfPkg/OvmfPkgX64.dsc | 7 +++++++
3 files changed, 21 insertions(+)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 7ccb61147f27..c5bd9aa4dc6c 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -420,25 +420,32 @@ [PcdsFixedAtBuild]
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
!endif
!if $(FD_SIZE_IN_KB) == 4096
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
!endif
gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
+ # REPORT_STATUS_CODE_PROPERTY_PROGRESS_CODE_ENABLED 0x01
+ # REPORT_STATUS_CODE_PROPERTY_ERROR_CODE_ENABLED 0x02
+ # REPORT_STATUS_CODE_PROPERTY_DEBUG_CODE_ENABLED 0x04
+!if $(TARGET) == RELEASE
+ gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x03
+!else
gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
+!endif
# DEBUG_INIT 0x00000001 // Initialization
# DEBUG_WARN 0x00000002 // Warnings
# DEBUG_LOAD 0x00000004 // Load events
# DEBUG_FS 0x00000008 // EFI File system
# DEBUG_POOL 0x00000010 // Alloc & Free (pool)
# DEBUG_PAGE 0x00000020 // Alloc & Free (page)
# DEBUG_INFO 0x00000040 // Informational debug messages
# DEBUG_DISPATCH 0x00000080 // PEI/DXE/SMM Dispatchers
# DEBUG_VARIABLE 0x00000100 // Variable
# DEBUG_BM 0x00000400 // Boot Manager
# DEBUG_BLKIO 0x00001000 // BlkIo Driver
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 237ec71b5ec0..6e39896b318f 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -425,25 +425,32 @@ [PcdsFixedAtBuild]
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
!endif
!if $(FD_SIZE_IN_KB) == 4096
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
!endif
gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
+ # REPORT_STATUS_CODE_PROPERTY_PROGRESS_CODE_ENABLED 0x01
+ # REPORT_STATUS_CODE_PROPERTY_ERROR_CODE_ENABLED 0x02
+ # REPORT_STATUS_CODE_PROPERTY_DEBUG_CODE_ENABLED 0x04
+!if $(TARGET) == RELEASE
+ gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x03
+!else
gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
+!endif
# DEBUG_INIT 0x00000001 // Initialization
# DEBUG_WARN 0x00000002 // Warnings
# DEBUG_LOAD 0x00000004 // Load events
# DEBUG_FS 0x00000008 // EFI File system
# DEBUG_POOL 0x00000010 // Alloc & Free (pool)
# DEBUG_PAGE 0x00000020 // Alloc & Free (page)
# DEBUG_INFO 0x00000040 // Informational debug messages
# DEBUG_DISPATCH 0x00000080 // PEI/DXE/SMM Dispatchers
# DEBUG_VARIABLE 0x00000100 // Variable
# DEBUG_BM 0x00000400 // Boot Manager
# DEBUG_BLKIO 0x00001000 // BlkIo Driver
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index a5047fa38e61..2e2d4d584429 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -425,25 +425,32 @@ [PcdsFixedAtBuild]
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
!endif
!if $(FD_SIZE_IN_KB) == 4096
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
!endif
gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
+ # REPORT_STATUS_CODE_PROPERTY_PROGRESS_CODE_ENABLED 0x01
+ # REPORT_STATUS_CODE_PROPERTY_ERROR_CODE_ENABLED 0x02
+ # REPORT_STATUS_CODE_PROPERTY_DEBUG_CODE_ENABLED 0x04
+!if $(TARGET) == RELEASE
+ gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x03
+!else
gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
+!endif
# DEBUG_INIT 0x00000001 // Initialization
# DEBUG_WARN 0x00000002 // Warnings
# DEBUG_LOAD 0x00000004 // Load events
# DEBUG_FS 0x00000008 // EFI File system
# DEBUG_POOL 0x00000010 // Alloc & Free (pool)
# DEBUG_PAGE 0x00000020 // Alloc & Free (page)
# DEBUG_INFO 0x00000040 // Informational debug messages
# DEBUG_DISPATCH 0x00000080 // PEI/DXE/SMM Dispatchers
# DEBUG_VARIABLE 0x00000100 // Variable
# DEBUG_BM 0x00000400 // Boot Manager
# DEBUG_BLKIO 0x00001000 // BlkIo Driver
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] MdeModulePkg/BdsDxe: fall back to a Boot Manager Menu loop before hanging
2017-11-22 23:58 ` [PATCH 1/5] MdeModulePkg/BdsDxe: fall back to a Boot Manager Menu loop before hanging Laszlo Ersek
@ 2017-11-23 3:43 ` Ni, Ruiyu
2017-11-23 13:09 ` Laszlo Ersek
0 siblings, 1 reply; 14+ messages in thread
From: Ni, Ruiyu @ 2017-11-23 3:43 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Ard Biesheuvel, Justen, Jordan L, Dong, Eric, Zeng, Star
Thanks for the patch. It follows the idea described in:
https://lists.01.org/pipermail/edk2-devel/2017-April/010294.html.
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
Thanks/Ray
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, November 23, 2017 7:59 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 1/5] MdeModulePkg/BdsDxe: fall back to a Boot Manager
> Menu loop before hanging
>
> Under the following scenario:
>
> - no UEFI bootable application available anywhere in the system,
> - ... not even for the default platform recovery option,
> - no shell is built into the firmware image,
> - but UiApp is available in the firmware image,
>
> we should preferably not just hang in BdsEntry() with:
>
> DEBUG ((EFI_D_ERROR, "[Bds] Unable to boot!\n"));
> CpuDeadLoop ();
>
> while the user sits at the TianoCore logo page, wondering what's going on.
> Print an informative message to the console, wait for a keypress, and then
> return to the Boot Manager Menu forever.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1515418
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=513
> Suggested-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 60 ++++++++++++++++++-
> -
> 1 file changed, 56 insertions(+), 4 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> index dccc49090219..2b24755ac368 100644
> --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> @@ -676,24 +676,73 @@ BdsAllocateMemoryForPerformanceData (
> //
> // Mark L"PerfDataMemAddr" variable to read-only if the Variable Lock
> protocol exists
> // Still lock it even the variable cannot be saved to prevent it's set by 3rd
> party code.
> //
> Status = gBS->LocateProtocol (&gEdkiiVariableLockProtocolGuid, NULL,
> (VOID **) &VariableLock);
> if (!EFI_ERROR (Status)) {
> Status = VariableLock->RequestToLock (VariableLock,
> L"PerfDataMemAddr", &gPerformanceProtocolGuid);
> ASSERT_EFI_ERROR (Status);
> }
> }
> }
>
> +/**
> + Enter an infinite loop of calling the Boot Manager Menu.
> +
> + This is a last resort alternative to BdsEntry() giving up for good.
> + This function never returns.
> +
> + @param[in] BootManagerMenu The
> EFI_BOOT_MANAGER_LOAD_OPTION located and/or
> + created by the EfiBootManagerGetBootManagerMenu()
> + call in BdsEntry().
> +**/
> +VOID
> +BdsBootManagerMenuLoop (
> + IN EFI_BOOT_MANAGER_LOAD_OPTION *BootManagerMenu
> + )
> +{
> + EFI_INPUT_KEY Key;
> +
> + //
> + // Normally BdsDxe does not print anything to the system console, but
> + this is // a last resort -- the end-user will likely not see any
> + DEBUG messages // logged in this situation.
> + //
> + // AsciiPrint() will NULL-check gST->ConOut internally. We check
> + gST->ConIn // here to see if it makes sense to request and wait for a
> keypress.
> + //
> + if (gST->ConIn != NULL) {
> + AsciiPrint (
> + "%a: No bootable option or device was found.\n"
> + "%a: Press any key to enter the Boot Manager Menu.\n",
> + gEfiCallerBaseName,
> + gEfiCallerBaseName
> + );
> + BdsWaitForSingleEvent (gST->ConIn->WaitForKey, 0);
> +
> + //
> + // Drain any queued keys.
> + //
> + while (!EFI_ERROR (gST->ConIn->ReadKeyStroke (gST->ConIn, &Key))) {
> + //
> + // just throw away Key
> + //
> + }
> + }
> +
> + for (;;) {
> + EfiBootManagerBoot (BootManagerMenu);
> + }
> +}
> +
> /**
>
> Service routine for BdsInstance->Entry(). Devices are connected, the
> consoles are initialized, and the boot options are tried.
>
> @param This Protocol Instance structure.
>
> **/
> VOID
> EFIAPI
> BdsEntry (
> IN EFI_BDS_ARCH_PROTOCOL *This
> @@ -1079,34 +1128,37 @@ BdsEntry (
> }
>
> do {
> //
> // Retry to boot if any of the boot succeeds
> //
> LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount,
> LoadOptionTypeBoot);
> BootSuccess = BootBootOptions (LoadOptions, LoadOptionCount,
> (BootManagerMenuStatus != EFI_NOT_FOUND) ? &BootManagerMenu :
> NULL);
> EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
> } while (BootSuccess);
> }
>
> - if (BootManagerMenuStatus != EFI_NOT_FOUND) {
> - EfiBootManagerFreeLoadOption (&BootManagerMenu);
> - }
> -
> if (!BootSuccess) {
> LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount,
> LoadOptionTypePlatformRecovery);
> ProcessLoadOptions (LoadOptions, LoadOptionCount);
> EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
> }
>
> + //
> + // If BootManagerMenu is available, fall back to it indefinitely.
> + //
> + if (BootManagerMenuStatus != EFI_NOT_FOUND) {
> + BdsBootManagerMenuLoop (&BootManagerMenu); }
> +
> DEBUG ((EFI_D_ERROR, "[Bds] Unable to boot!\n"));
> CpuDeadLoop ();
> }
>
> /**
> Set the variable and report the error through status code upon failure.
>
> @param VariableName A Null-terminated string that is the name of
> the vendor's variable.
> Each VariableName is unique for each VendorGuid.
> VariableName must
> contain 1 or more characters. If VariableName is an empty
> string,
> then EFI_INVALID_PARAMETER is returned.
> @param VendorGuid A unique identifier for the vendor.
> --
> 2.14.1.3.gb7cf6e02401b
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report EDKII_OS_LOADER_DETAIL status code
2017-11-22 23:58 ` [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report EDKII_OS_LOADER_DETAIL status code Laszlo Ersek
@ 2017-11-23 5:21 ` Ni, Ruiyu
2017-11-23 14:03 ` Laszlo Ersek
0 siblings, 1 reply; 14+ messages in thread
From: Ni, Ruiyu @ 2017-11-23 5:21 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Ard Biesheuvel, Justen, Jordan L, Dong, Eric, Zeng, Star
Laszlo,
When booting a boot option, UefiBootManagerBoot() sets a Boot#### variable
and saves #### to BootCurrent variable.
So all the details (except the EDKII_OS_LOADER_DETAIL.Status) can be retrieved
from reading Boot#### variable.
4 more comments below.
Thanks/Ray
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, November 23, 2017 7:59 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report
> EDKII_OS_LOADER_DETAIL status code
>
> The EfiBootManagerBoot() function reports progress codes about
> LoadImage() preparation and failure, and StartImage() preparation and
> failure. These codes are flat (scalar) constants.
>
> Extend this by "broadcasting" the Boot#### option number, description,
> device path, and -- on load / start failure -- the error result at the same
> locations, through EFI_DEBUG_CODE reporting. Use the
> PcdDebugCodeOsLoaderDetail status code value and the
> EDKII_OS_LOADER_DETAIL status code structure introduced earlier in this
> series.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1515418
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf | 2
> +
> MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h | 84
> ++++++++++
> MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 51 +++++-
> MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c | 166
> ++++++++++++++++++++
> 4 files changed, 301 insertions(+), 2 deletions(-)
>
> diff --git
> a/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
> b/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
> index ad4901db5713..633906fc6ca4 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
> +++
> b/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
> @@ -79,24 +79,25 @@ [Guids]
>
> ## SOMETIMES_PRODUCES ## Variable:L"BootCurrent" (The boot option of
> current boot)
> ## SOMETIMES_CONSUMES ## Variable:L"BootXX" (Boot option variable)
> ## SOMETIMES_CONSUMES ## Variable:L"BootOrder" (The boot option
> array)
> ## SOMETIMES_CONSUMES ## Variable:L"DriverOrder" (The driver order
> list)
> ## SOMETIMES_CONSUMES ## Variable:L"ConIn" (The device path of
> console in device)
> ## SOMETIMES_CONSUMES ## Variable:L"ConOut" (The device path of
> console out device)
> ## SOMETIMES_CONSUMES ## Variable:L"ErrOut" (The device path of
> error out device)
> gEfiGlobalVariableGuid
>
> gPerformanceProtocolGuid ## SOMETIMES_CONSUMES ##
> Variable:L"PerfDataMemAddr" (The ACPI address of performance data)
> gEdkiiStatusCodeDataTypeVariableGuid ## SOMETIMES_CONSUMES
> ## GUID
> + gEdkiiStatusCodeDataTypeOsLoaderDetailGuid ##
> SOMETIMES_CONSUMES ## GUID
> gEfiDiskInfoAhciInterfaceGuid ## SOMETIMES_CONSUMES ##
> GUID
> gEfiDiskInfoIdeInterfaceGuid ## SOMETIMES_CONSUMES ## GUID
> gEfiDiskInfoScsiInterfaceGuid ## SOMETIMES_CONSUMES ## GUID
> gEfiDiskInfoSdMmcInterfaceGuid ## SOMETIMES_CONSUMES ##
> GUID
>
> [Protocols]
> gEfiPciRootBridgeIoProtocolGuid ## CONSUMES
> gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES
> gEfiLoadFileProtocolGuid ## SOMETIMES_CONSUMES
> gEfiSimpleTextOutProtocolGuid ## SOMETIMES_CONSUMES
> gEfiPciIoProtocolGuid ## SOMETIMES_CONSUMES
> gEfiLoadedImageProtocolGuid ## CONSUMES
> @@ -112,16 +113,17 @@ [Protocols]
> gEfiUsbIoProtocolGuid ## SOMETIMES_CONSUMES
> gEfiNvmExpressPassThruProtocolGuid ## SOMETIMES_CONSUMES
> gEfiDiskInfoProtocolGuid ## SOMETIMES_CONSUMES
> gEfiDriverHealthProtocolGuid ## SOMETIMES_CONSUMES
> gEfiFormBrowser2ProtocolGuid ## SOMETIMES_CONSUMES
> gEfiRamDiskProtocolGuid ## SOMETIMES_CONSUMES
> gEfiDeferredImageLoadProtocolGuid ## SOMETIMES_CONSUMES
>
> [Pcd]
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationC
> hange ## SOMETIMES_CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdProgressCodeOsLoaderLoad
> ## SOMETIMES_CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdProgressCodeOsLoaderStart
> ## SOMETIMES_CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdDebugCodeOsLoaderDetail
> ## SOMETIMES_CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable ##
> SOMETIMES_CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile
> ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdDriverHealthConfigureForm
> ## SOMETIMES_CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxRepairCount ##
> CONSUMES
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
> b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
> index 0224bd34a9ed..ddcb0347aef6 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
> @@ -44,24 +44,25 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
> #include <Protocol/BootLogo.h>
> #include <Protocol/DriverHealth.h>
> #include <Protocol/FormBrowser2.h>
> #include <Protocol/VariableLock.h>
> #include <Protocol/RamDisk.h>
> #include <Protocol/DeferredImageLoad.h>
>
> #include <Guid/MemoryTypeInformation.h> #include <Guid/FileInfo.h>
> #include <Guid/GlobalVariable.h> #include <Guid/Performance.h> #include
> <Guid/StatusCodeDataTypeVariable.h>
> +#include <Guid/StatusCodeDataTypeOsLoaderDetail.h>
>
> #include <Library/PrintLib.h>
> #include <Library/DebugLib.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/UefiRuntimeServicesTableLib.h>
> #include <Library/UefiLib.h>
> #include <Library/MemoryAllocationLib.h> #include
> <Library/DxeServicesTableLib.h> #include <Library/HobLib.h> #include
> <Library/BaseLib.h> #include <Library/DevicePathLib.h> @@ -298,24
> +299,107 @@ BmStopHotkeyService (
>
> @retval EFI_NOT_FOUND The variable trying to be updated or deleted
> was not found.
> **/
> EFI_STATUS
> BmSetVariableAndReportStatusCodeOnError (
> IN CHAR16 *VariableName,
> IN EFI_GUID *VendorGuid,
> IN UINT32 Attributes,
> IN UINTN DataSize,
> IN VOID *Data
> );
>
> +/**
> + Dynamically allocate and initialize an EDKII_OS_LOADER_DETAIL status
> +code
> + payload.
> +
> + @param[in] BootOption Capture the OptionNumber, FilePath and
> + Description fields of BootOption in the
> + EDKII_OS_LOADER_DETAIL payload.
> +
> + @param[out] OsLoaderDetail On successful return, set to the
> + EDKII_OS_LOADER_DETAIL object that has been
> + allocated and initialized. On failure, not
> + modified.
> +
> + @param[out] OsLoaderDetailSize On successful return, set to the size of
> the
> + EDKII_OS_LOADER_DETAIL object that has been
> + allocated and initialized. On failure, not
> + modified.
> +
> + @retval EFI_UNSUPPORTED EFI_DEBUG_CODE reporting is disabled in
> the
> + platform.
> +
> + @retval EFI_INVALID_PARAMETER BootOption->OptionNumber is not in
> + 0x0000..0xFFFF, inclusive.
> +
> + @retval EFI_BAD_BUFFER_SIZE BootOption->FilePath and/or
> + BootOption->Description would exceed the
> + UINT16 size limits presented by
> + EDKII_OS_LOADER_DETAIL or
> + EFI_STATUS_CODE_DATA.
> +
> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
> +
> + @retval EFI_SUCCESS The EDKII_OS_LOADER_DETAIL object has
> been
> + allocated and initialized. The caller is
> + responsible for freeing the object with
> + FreePool().
> +**/
> +EFI_STATUS
> +BmCreateOsLoaderDetail (
> + IN CONST EFI_BOOT_MANAGER_LOAD_OPTION *BootOption,
> + OUT EDKII_OS_LOADER_DETAIL **OsLoaderDetail,
> + OUT UINTN *OsLoaderDetailSize
> + );
> +
> +/**
> + Report an EFI_DEBUG_CODE status code with EDKII_OS_LOADER_DETAIL
> as
> +payload
> + (i.e., extended data).
> +
> + @param[in,out] OsLoaderDetail The EDKII_OS_LOADER_DETAIL payload
> previously
> + created with BmCreateOsLoaderDetail(), and
> + modified zero or more times by
> + BmReportOsLoaderDetail(). If OsLoaderDetail is
> + NULL, the function does nothing. Otherwise,
> + the Type and Status fields are overwritten in
> + OsLoaderDetail, and a status code is reported.
> +
> + @param[in] OsLoaderDetailSize The size returned by
> BmCreateOsLoaderDetail().
> + If OsLoaderDetail is NULL, OsLoaderDetailSize
> + may be zero.
> +
> + @param[in] DetailType OsLoaderDetail->Type is set to DetailType
> + before reporting the status code. The caller
> + is responsible for passing an
> + EDKII_OS_LOADER_DETAIL_TYPE_* value.
> +
> + @param[in] DetailStatus OsLoaderDetail->Status is set to DetailStatus
> + before reporting the status code.
> +
> + @retval EFI_UNSUPPORTED EFI_DEBUG_CODE reporting is disabled in the
> + platform.
> +
> + @retval EFI_ABORTED OsLoaderDetail is NULL.
> +
> + @return Values propagated from REPORT_STATUS_CODE_EX().
> +**/
> +EFI_STATUS
> +BmReportOsLoaderDetail (
> + IN OUT EDKII_OS_LOADER_DETAIL *OsLoaderDetail OPTIONAL,
> + IN UINTN OsLoaderDetailSize,
> + IN UINT32 DetailType,
> + IN EFI_STATUS DetailStatus
> + );
> +
> /**
> Function compares a device path data structure to that of all the nodes of a
> second device path instance.
>
> @param Multi A pointer to a multi-instance device path data
> structure.
> @param Single A pointer to a single-instance device path data
> structure.
>
> @retval TRUE If the Single device path is contained within Multi
> device path.
> @retval FALSE The Single device path is not match within Multi
> device path.
>
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> index d6844823aa55..049afbf7d1f9 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> @@ -1665,34 +1665,39 @@ EfiBootManagerBoot (
> EFI_STATUS Status;
> EFI_HANDLE ImageHandle;
> EFI_LOADED_IMAGE_PROTOCOL *ImageInfo;
> UINT16 Uint16;
> UINTN OptionNumber;
> UINTN OriginalOptionNumber;
> EFI_DEVICE_PATH_PROTOCOL *FilePath;
> EFI_DEVICE_PATH_PROTOCOL *RamDiskDevicePath;
> VOID *FileBuffer;
> UINTN FileSize;
> EFI_BOOT_LOGO_PROTOCOL *BootLogo;
> EFI_EVENT LegacyBootEvent;
> + EDKII_OS_LOADER_DETAIL *OsLoaderDetail;
> + UINTN OsLoaderDetailSize;
>
> if (BootOption == NULL) {
> return;
> }
>
> if (BootOption->FilePath == NULL || BootOption->OptionType !=
> LoadOptionTypeBoot) {
> BootOption->Status = EFI_INVALID_PARAMETER;
> return;
> }
>
> + OsLoaderDetail = NULL;
> + OsLoaderDetailSize = 0;
> +
> //
> // 1. Create Boot#### for a temporary boot if there is no match Boot####
> (i.e. a boot by selected a EFI Shell using "Boot From File")
> //
> OptionNumber = BmFindBootOptionInVariable (BootOption);
> if (OptionNumber == LoadOptionNumberUnassigned) {
> Status = BmGetFreeOptionNumber (LoadOptionTypeBoot, &Uint16);
> if (!EFI_ERROR (Status)) {
> //
> // Save the BootOption->OptionNumber to restore later
> //
> OptionNumber = Uint16;
> OriginalOptionNumber = BootOption->OptionNumber;
> @@ -1751,68 +1756,93 @@ EfiBootManagerBoot (
>
> //
> // 6. Load EFI boot option to ImageHandle
> //
> DEBUG_CODE_BEGIN ();
> if (BootOption->Description == NULL) {
> DEBUG ((DEBUG_INFO | DEBUG_LOAD, "[Bds]Booting from unknown
> device path\n"));
> } else {
> DEBUG ((DEBUG_INFO | DEBUG_LOAD, "[Bds]Booting %s\n", BootOption-
> >Description));
> }
> DEBUG_CODE_END ();
>
> + //
> + // Try to create the status code payload structure for detailed debug
> + // reporting.
> + //
> + Status = BmCreateOsLoaderDetail (BootOption, &OsLoaderDetail,
> + &OsLoaderDetailSize);
> + if (EFI_ERROR (Status) && (Status != EFI_UNSUPPORTED)) {
> + DEBUG ((DEBUG_WARN | DEBUG_LOAD,
> "[Bds]BmCreateOsLoaderDetail(): %r\n",
> + Status));
> + }
> +
> ImageHandle = NULL;
> RamDiskDevicePath = NULL;
> if (DevicePathType (BootOption->FilePath) != BBS_DEVICE_PATH) {
> Status = EFI_NOT_FOUND;
> FilePath = NULL;
> EfiBootManagerConnectDevicePath (BootOption->FilePath, NULL);
> FileBuffer = BmGetNextLoadOptionBuffer (LoadOptionTypeBoot,
> BootOption->FilePath, &FilePath, &FileSize);
> if (FileBuffer != NULL) {
> RamDiskDevicePath = BmGetRamDiskDevicePath (FilePath);
>
> REPORT_STATUS_CODE (EFI_PROGRESS_CODE, PcdGet32
> (PcdProgressCodeOsLoaderLoad));
> + BmReportOsLoaderDetail (
> + OsLoaderDetail,
> + OsLoaderDetailSize,
> + EDKII_OS_LOADER_DETAIL_TYPE_LOAD,
> + 0 // DetailStatus -- unused here
> + );
> +
1. With the BootCurrent, this change is not necessary because
others can hook PcdProgressCodeOsLoaderLoad to get the current
loading boot option.
> Status = gBS->LoadImage (
> TRUE,
> gImageHandle,
> FilePath,
> FileBuffer,
> FileSize,
> &ImageHandle
> );
> }
> if (FileBuffer != NULL) {
> FreePool (FileBuffer);
> }
> if (FilePath != NULL) {
> FreePool (FilePath);
> }
>
> if (EFI_ERROR (Status)) {
> //
> // Report Status Code to indicate that the failure to load boot option
> //
> REPORT_STATUS_CODE (
> EFI_ERROR_CODE | EFI_ERROR_MINOR,
> (EFI_SOFTWARE_DXE_BS_DRIVER |
> EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR)
> );
> + BmReportOsLoaderDetail (
> + OsLoaderDetail,
> + OsLoaderDetailSize,
> + EDKII_OS_LOADER_DETAIL_TYPE_LOAD_ERROR,
> + Status
> + );
> +
2. I think firstly, the OsLoaderDetail is not needed.
Secondly, I prefer to submit a PI spec change to include extended
data for EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR status code.
Instead of inventing a new status code.
> BootOption->Status = Status;
> //
> // Destroy the RAM disk
> //
> if (RamDiskDevicePath != NULL) {
> BmDestroyRamDisk (RamDiskDevicePath);
> FreePool (RamDiskDevicePath);
> }
> - return;
> + goto FreeOsLoaderDetail;
> }
> }
>
> //
> // Check to see if we should legacy BOOT. If yes then do the legacy boot
> // Write boot to OS performance data for Legacy boot
> //
> if ((DevicePathType (BootOption->FilePath) == BBS_DEVICE_PATH) &&
> (DevicePathSubType (BootOption->FilePath) == BBS_BBS_DP)) {
> if (mBmLegacyBoot != NULL) {
> //
> // Write boot to OS performance data for legacy boot.
> //
> @@ -1826,25 +1856,25 @@ EfiBootManagerBoot (
> NULL,
> &LegacyBootEvent
> );
> ASSERT_EFI_ERROR (Status);
> );
>
> mBmLegacyBoot (BootOption);
> } else {
> BootOption->Status = EFI_UNSUPPORTED;
> }
>
> PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32)
> OptionNumber);
> - return;
> + goto FreeOsLoaderDetail;
> }
>
> //
> // Provide the image with its load options
> //
> Status = gBS->HandleProtocol (ImageHandle,
> &gEfiLoadedImageProtocolGuid, (VOID **) &ImageInfo);
> ASSERT_EFI_ERROR (Status);
>
> if (!BmIsAutoCreateBootOption (BootOption)) {
> ImageInfo->LoadOptionsSize = BootOption->OptionalDataSize;
> ImageInfo->LoadOptions = BootOption->OptionalData;
> }
> @@ -1858,36 +1888,48 @@ EfiBootManagerBoot (
> // Before calling the image, enable the Watchdog Timer for 5 minutes
> period
> //
> gBS->SetWatchdogTimer (5 * 60, 0x0000, 0x00, NULL);
>
> //
> // Write boot to OS performance data for UEFI boot
> //
> PERF_CODE (
> BmWriteBootToOsPerformanceData (NULL, NULL);
> );
>
> REPORT_STATUS_CODE (EFI_PROGRESS_CODE, PcdGet32
> (PcdProgressCodeOsLoaderStart));
> + BmReportOsLoaderDetail (
> + OsLoaderDetail,
> + OsLoaderDetailSize,
> + EDKII_OS_LOADER_DETAIL_TYPE_START,
> + 0 // DetailStatus -- unused here
> + );
>
3. As above comment #1, this change is not needed.
> 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 (
> EFI_ERROR_CODE | EFI_ERROR_MINOR,
> (EFI_SOFTWARE_DXE_BS_DRIVER |
> EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED)
> );
> + BmReportOsLoaderDetail (
> + OsLoaderDetail,
> + OsLoaderDetailSize,
> + EDKII_OS_LOADER_DETAIL_TYPE_START_ERROR,
> + Status
> + );
> }
4. Similar comments to #2.
> PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32)
> OptionNumber);
>
> //
> // Destroy the RAM disk
> //
> if (RamDiskDevicePath != NULL) {
> BmDestroyRamDisk (RamDiskDevicePath);
> FreePool (RamDiskDevicePath);
> }
>
> //
> @@ -1912,24 +1954,29 @@ EfiBootManagerBoot (
> L"BootCurrent",
> &gEfiGlobalVariableGuid,
> 0,
> 0,
> NULL
> );
> //
> // Deleting variable with current variable implementation shouldn't fail.
> // When BootXXXX (e.g.: BootManagerMenu) boots BootYYYY, exiting
> BootYYYY causes BootCurrent deleted,
> // exiting BootXXXX causes deleting BootCurrent returns EFI_NOT_FOUND.
> //
> ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_FOUND);
> +
> +FreeOsLoaderDetail:
> + if (OsLoaderDetail != NULL) {
> + FreePool (OsLoaderDetail);
> + }
> }
>
> /**
> Check whether there is a instance in BlockIoDevicePath, which contain multi
> device path
> instances, has the same partition node with HardDriveDevicePath device
> path
>
> @param BlockIoDevicePath Multi device path instances which need to
> check
> @param HardDriveDevicePath A device path which starts with a hard
> drive media
> device path.
>
> @retval TRUE There is a matched device path instance.
> @retval FALSE There is no matched device path instance.
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> index 81d365940043..29da896854b8 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> @@ -369,24 +369,190 @@ BmSetVariableAndReportStatusCodeOnError (
> SetVariableStatus,
> sizeof (EDKII_SET_VARIABLE_STATUS) + NameSize + DataSize
> );
>
> FreePool (SetVariableStatus);
> }
> }
>
> return Status;
> }
>
>
> +/**
> + Dynamically allocate and initialize an EDKII_OS_LOADER_DETAIL status
> +code
> + payload.
> +
> + @param[in] BootOption Capture the OptionNumber, FilePath and
> + Description fields of BootOption in the
> + EDKII_OS_LOADER_DETAIL payload.
> +
> + @param[out] OsLoaderDetail On successful return, set to the
> + EDKII_OS_LOADER_DETAIL object that has been
> + allocated and initialized. On failure, not
> + modified.
> +
> + @param[out] OsLoaderDetailSize On successful return, set to the size of
> the
> + EDKII_OS_LOADER_DETAIL object that has been
> + allocated and initialized. On failure, not
> + modified.
> +
> + @retval EFI_UNSUPPORTED EFI_DEBUG_CODE reporting is disabled in
> the
> + platform.
> +
> + @retval EFI_INVALID_PARAMETER BootOption->OptionNumber is not in
> + 0x0000..0xFFFF, inclusive.
> +
> + @retval EFI_BAD_BUFFER_SIZE BootOption->FilePath and/or
> + BootOption->Description would exceed the
> + UINT16 size limits presented by
> + EDKII_OS_LOADER_DETAIL or
> + EFI_STATUS_CODE_DATA.
> +
> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
> +
> + @retval EFI_SUCCESS The EDKII_OS_LOADER_DETAIL object has
> been
> + allocated and initialized. The caller is
> + responsible for freeing the object with
> + FreePool().
> +**/
> +EFI_STATUS
> +BmCreateOsLoaderDetail (
> + IN CONST EFI_BOOT_MANAGER_LOAD_OPTION *BootOption,
> + OUT EDKII_OS_LOADER_DETAIL **OsLoaderDetail,
> + OUT UINTN *OsLoaderDetailSize
> + )
> +{
> + UINTN DescriptionSize;
> + UINTN DevicePathSize;
> + UINTN PayloadSize;
> + EDKII_OS_LOADER_DETAIL *Payload;
> + UINT8 *VariableSizeData;
> +
> + if (!ReportDebugCodeEnabled ()) {
> + return EFI_UNSUPPORTED;
> + }
> +
> + if (BootOption->OptionNumber >= LoadOptionNumberMax) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + DescriptionSize = (BootOption->Description == NULL) ?
> + 0 :
> + StrSize (BootOption->Description); DevicePathSize
> + = GetDevicePathSize (BootOption->FilePath); PayloadSize = sizeof
> + *Payload + DescriptionSize + DevicePathSize;
> +
> + if (DescriptionSize > MAX_UINT16 ||
> + DevicePathSize > MAX_UINT16 ||
> + PayloadSize > MAX_UINT16) {
> + return EFI_BAD_BUFFER_SIZE;
> + }
> +
> + Payload = AllocateZeroPool (PayloadSize); if (Payload == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> + }
> + VariableSizeData = (UINT8 *)(Payload + 1);
> +
> + //
> + // Populate the variable size fields at the end of the payload.
> + //
> + CopyMem (VariableSizeData, BootOption->Description, DescriptionSize);
> + VariableSizeData += DescriptionSize;
> +
> + CopyMem (VariableSizeData, BootOption->FilePath, DevicePathSize);
> + VariableSizeData += DevicePathSize;
> +
> + ASSERT (VariableSizeData - (UINT8 *)Payload == PayloadSize);
> +
> + //
> + // Populate the fixed fields in the payload. Any members not listed
> + below // remain zero-filled at this point.
> + //
> + Payload->BootOptionNumber = (UINT16)BootOption->OptionNumber;
> + Payload->DescriptionSize = (UINT16)DescriptionSize;
> + Payload->DevicePathSize = (UINT16)DevicePathSize;
> +
> + *OsLoaderDetail = Payload;
> + *OsLoaderDetailSize = PayloadSize;
> + return EFI_SUCCESS;
> +}
> +
> +
> +/**
> + Report an EFI_DEBUG_CODE status code with EDKII_OS_LOADER_DETAIL
> as
> +payload
> + (i.e., extended data).
> +
> + @param[in,out] OsLoaderDetail The EDKII_OS_LOADER_DETAIL payload
> previously
> + created with BmCreateOsLoaderDetail(), and
> + modified zero or more times by
> + BmReportOsLoaderDetail(). If OsLoaderDetail is
> + NULL, the function does nothing. Otherwise,
> + the Type and Status fields are overwritten in
> + OsLoaderDetail, and a status code is reported.
> +
> + @param[in] OsLoaderDetailSize The size returned by
> BmCreateOsLoaderDetail().
> + If OsLoaderDetail is NULL, OsLoaderDetailSize
> + may be zero.
> +
> + @param[in] DetailType OsLoaderDetail->Type is set to DetailType
> + before reporting the status code. The caller
> + is responsible for passing an
> + EDKII_OS_LOADER_DETAIL_TYPE_* value.
> +
> + @param[in] DetailStatus OsLoaderDetail->Status is set to DetailStatus
> + before reporting the status code.
> +
> + @retval EFI_UNSUPPORTED EFI_DEBUG_CODE reporting is disabled in the
> + platform.
> +
> + @retval EFI_ABORTED OsLoaderDetail is NULL.
> +
> + @return Values propagated from REPORT_STATUS_CODE_EX().
> +**/
> +EFI_STATUS
> +BmReportOsLoaderDetail (
> + IN OUT EDKII_OS_LOADER_DETAIL *OsLoaderDetail OPTIONAL,
> + IN UINTN OsLoaderDetailSize,
> + IN UINT32 DetailType,
> + IN EFI_STATUS DetailStatus
> + )
> +{
> + EFI_STATUS Status;
> +
> + if (!ReportDebugCodeEnabled ()) {
> + return EFI_UNSUPPORTED;
> + }
> +
> + if (OsLoaderDetail == NULL) {
> + return EFI_ABORTED;
> + }
> +
> + OsLoaderDetail->Type = DetailType;
> + OsLoaderDetail->Status = DetailStatus;
> +
> + Status = REPORT_STATUS_CODE_EX (
> + EFI_DEBUG_CODE, // Type
> + PcdGet32 (PcdDebugCodeOsLoaderDetail), // Value
> + 0, // Instance
> + &gEfiCallerIdGuid, // CallerId
> + &gEdkiiStatusCodeDataTypeOsLoaderDetailGuid, //
> ExtendedDataGuid
> + OsLoaderDetail, // ExtendedData
> + OsLoaderDetailSize // ExtendedDataSize
> + );
> + return Status;
> +}
> +
> +
> /**
> Print the device path info.
>
> @param DevicePath The device path need to print.
> **/
> VOID
> BmPrintDp (
> EFI_DEVICE_PATH_PROTOCOL *DevicePath
> )
> {
> CHAR16 *Str;
>
> --
> 2.14.1.3.gb7cf6e02401b
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] MdeModulePkg/BdsDxe: fall back to a Boot Manager Menu loop before hanging
2017-11-23 3:43 ` Ni, Ruiyu
@ 2017-11-23 13:09 ` Laszlo Ersek
2017-11-27 16:27 ` Laszlo Ersek
0 siblings, 1 reply; 14+ messages in thread
From: Laszlo Ersek @ 2017-11-23 13:09 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel-01
Cc: Ard Biesheuvel, Justen, Jordan L, Dong, Eric, Zeng, Star
On 11/23/17 04:43, Ni, Ruiyu wrote:
> Thanks for the patch. It follows the idea described in:
> https://lists.01.org/pipermail/edk2-devel/2017-April/010294.html.
>
> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
Thank you, Ray! I might commit this first patch in isolation (to close
#513) while we continue discussing the rest. Not sure yet. (Depends on
my workload.)
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report EDKII_OS_LOADER_DETAIL status code
2017-11-23 5:21 ` Ni, Ruiyu
@ 2017-11-23 14:03 ` Laszlo Ersek
2017-11-23 14:53 ` Ni, Ruiyu
0 siblings, 1 reply; 14+ messages in thread
From: Laszlo Ersek @ 2017-11-23 14:03 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel-01
Cc: Ard Biesheuvel, Justen, Jordan L, Dong, Eric, Zeng, Star,
Doran, Mark
Hello Ray,
(CC Mark Doran for the last part of my email)
On 11/23/17 06:21, Ni, Ruiyu wrote:
> Laszlo,
> When booting a boot option, UefiBootManagerBoot() sets a Boot#### variable
> and saves #### to BootCurrent variable.
> So all the details (except the EDKII_OS_LOADER_DETAIL.Status) can be retrieved
> from reading Boot#### variable.
I have two counter-arguments:
(1) I would like to minimize the number of accesses to non-volatile UEFI
variables. Dependent on the virtualization platform and the (virtual)
flash chip structure (for example, flash block size), flash access can
be very slow.
I'm not 100% sure whether this performance problem affects flash *reads*
as well, or if it affects flash writes only. (It definitely affects
flash writes.)
(2) The delivery of status codes to registered handlers is asynchronous,
not synchronous. Handlers are registered at various task priority
levels, and if the reporting occurs at the same or higher TPL, then the
delivery will be delayed. (According to the PI spec, it is even possible
to report several status codes before the first will be delivered --
basically the codes are queued until the TPL is reduced sufficiently.)
This is why all data has to be embedded in the status code structure itself.
Like in any other notify function's case, it is prudent to register
status code handlers at the least strict TPL that can work for the
actual processing. (The internals of the handler function can even put
an upper limit on the TPL; for example using Simple Text Output prevents
us from going higher than TPL_NOTIFY.) For this reason I chose
TPL_CALLBACK for the OVMF status code handler.
Now, if we can *guarantee* that these status codes are only reported at
the TPL_APPLICATION level, then a TPL_CALLBACK status code handler will
be synchronous in effect, and then the status code structure can carry
references to other (external) things in the system too, such as UEFI
variables.
Can we guarantee that EfiBootManagerBoot() is never invoked above
TPL_APPLICATION?
>
> 4 more comments below.
[snip]
>> + BmReportOsLoaderDetail (
>> + OsLoaderDetail,
>> + OsLoaderDetailSize,
>> + EDKII_OS_LOADER_DETAIL_TYPE_LOAD,
>> + 0 // DetailStatus -- unused here
>> + );
>> +
>
> 1. With the BootCurrent, this change is not necessary because
> others can hook PcdProgressCodeOsLoaderLoad to get the current
> loading boot option.
As long as we can clear my points (1) and (2) above, I'd be fine with
this approach.
>
>> Status = gBS->LoadImage (
>> TRUE,
>> gImageHandle,
>> FilePath,
>> FileBuffer,
>> FileSize,
>> &ImageHandle
>> );
>> }
>> if (FileBuffer != NULL) {
>> FreePool (FileBuffer);
>> }
>> if (FilePath != NULL) {
>> FreePool (FilePath);
>> }
>>
>> if (EFI_ERROR (Status)) {
>> //
>> // Report Status Code to indicate that the failure to load boot option
>> //
>> REPORT_STATUS_CODE (
>> EFI_ERROR_CODE | EFI_ERROR_MINOR,
>> (EFI_SOFTWARE_DXE_BS_DRIVER |
>> EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR)
>> );
>> + BmReportOsLoaderDetail (
>> + OsLoaderDetail,
>> + OsLoaderDetailSize,
>> + EDKII_OS_LOADER_DETAIL_TYPE_LOAD_ERROR,
>> + Status
>> + );
>> +
>
> 2. I think firstly, the OsLoaderDetail is not needed.
> Secondly, I prefer to submit a PI spec change to include extended
> data for EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR status code.
> Instead of inventing a new status code.
I understand your point, and in theory I agree with it, but in practice,
I cannot.
My experience with Mantis tickets is not good:
(a) First, I would have to write up the proposal. That's fine, I can do
that; have done it before.
(b) Second, I'd have to *call* the meetings. Please locate the email titled
a reminder about MANTIS usage
from Mark, (date: 6 Sep 2017), cross-posted to the PIWG/USWG/ASWG lists.
I'm not allowed to quote the email verbatim -- all of these lists are
confidential --, but if you look up the paragraph starting with "Best
practice", Mark basically implied, "file the the Mantis ticket, then
dial in to the next meeting and sell your proposal to the WG *verbally*".
I *cannot* do that. The WG meetings always take place early afternoon in
Pacific (US West Coast) timezone, which is around *midnight* in my timezone.
In addition, I don't even have time to attend the confcalls that I'm
invited to within Red Hat!
I don't understand why a carefully written Mantis ticket is not
sufficient for discussion, but apparently it isn't. :/
(c) Case in point, please see Mantis ticket #1736, titled
lacking specification of EFI_BOOT_SCRIPT_WIDTH in PI 1.5 / Vol 5 /
8.7.1 Save State Write
which I had originally filed in December 2016.
In February 2017, I was asked for more details. I said, "fair enough",
and I spent half a day writing up the requested change in *full* detail.
I gave editing instructions of the form
change this to that
and
add/delete the following
as recommended in Mark's email (b).
You can see my write-up in note 0005044. After that note, there have
been *zero* updates to the ticket; since February 2017.
So the fact is, unless you have time to call the WG meetings every week,
and push *live* for the change that you want, you have no chance at
getting stuff into the specs.
I mean, in his email (b), Mark suggests asking a "proxy" for
representing the change request, to anyone that cannot dial in. Well,
can *you* be my proxy on the PIWG meeting, if I write up the change
request? You certainly understand the problem space, and you maintain
the corresponding reference code in edk2. I just doubt you have time for
conference calls, same as I.
UefiBootManagerLib already reports an extended status code, with
EDKII_SET_VARIABLE_STATUS payload. Introducing another edk2-specific
status code requires no slow negotiation, and it can be adopted by
others, in practice, if they like it. Once it is wide-spread practice,
it can be more easily codified in the spec. The specifics for the first
implementation can be -- should be -- discussed on this open list; we
had better not standardize something that has zero implementations just yet.
Anyway, I'm willing to go through the standardization process, but only
if it doesn't require me to pick up the phone every week (esp. at midnight).
Thanks
Laszlo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report EDKII_OS_LOADER_DETAIL status code
2017-11-23 14:03 ` Laszlo Ersek
@ 2017-11-23 14:53 ` Ni, Ruiyu
2017-11-23 17:08 ` Laszlo Ersek
0 siblings, 1 reply; 14+ messages in thread
From: Ni, Ruiyu @ 2017-11-23 14:53 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Ard Biesheuvel, Justen, Jordan L, Dong, Eric, Zeng, Star,
Doran, Mark
Comments below.
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, November 23, 2017 10:03 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Doran, Mark <mark.doran@intel.com>
> Subject: Re: [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report
> EDKII_OS_LOADER_DETAIL status code
>
> Hello Ray,
>
> (CC Mark Doran for the last part of my email)
>
> On 11/23/17 06:21, Ni, Ruiyu wrote:
> > Laszlo,
> > When booting a boot option, UefiBootManagerBoot() sets a Boot####
> > variable and saves #### to BootCurrent variable.
> > So all the details (except the EDKII_OS_LOADER_DETAIL.Status) can be
> > retrieved from reading Boot#### variable.
>
> I have two counter-arguments:
>
> (1) I would like to minimize the number of accesses to non-volatile UEFI
> variables. Dependent on the virtualization platform and the (virtual) flash chip
> structure (for example, flash block size), flash access can be very slow.
>
> I'm not 100% sure whether this performance problem affects flash *reads* as
> well, or if it affects flash writes only. (It definitely affects flash writes.)
NV *read* should have no performance impact since EDKII variable driver's
implementation caches the flash in memory. I am not sure about that
in OVMF. But I think a good implementation of variable driver should
consider such *read* optimization.
Performance needs to be considered, but simpler/easy-maintain
code takes higher priority.
>
>
> (2) The delivery of status codes to registered handlers is asynchronous, not
> synchronous. Handlers are registered at various task priority levels, and if the
> reporting occurs at the same or higher TPL, then the delivery will be delayed.
> (According to the PI spec, it is even possible to report several status codes
> before the first will be delivered -- basically the codes are queued until the TPL is
> reduced sufficiently.) This is why all data has to be embedded in the status code
> structure itself.
>
> Like in any other notify function's case, it is prudent to register status code
> handlers at the least strict TPL that can work for the actual processing. (The
> internals of the handler function can even put an upper limit on the TPL; for
> example using Simple Text Output prevents us from going higher than
> TPL_NOTIFY.) For this reason I chose TPL_CALLBACK for the OVMF status code
> handler.
>
> Now, if we can *guarantee* that these status codes are only reported at the
> TPL_APPLICATION level, then a TPL_CALLBACK status code handler will be
> synchronous in effect, and then the status code structure can carry references
> to other (external) things in the system too, such as UEFI variables.
>
> Can we guarantee that EfiBootManagerBoot() is never invoked above
> TPL_APPLICATION?
Yes. It should be. Because gBS->LoadImage()/StartImage() per UEFI spec TPL
restriction rule, should be run at TPL_APPLICATION.
And if there is no special reason (handler runs too slow), the status handler
is registered at TPL_HIGH so that the handler is called synchronously.
>
> >
> > 4 more comments below.
>
> [snip]
>
> >> + BmReportOsLoaderDetail (
> >> + OsLoaderDetail,
> >> + OsLoaderDetailSize,
> >> + EDKII_OS_LOADER_DETAIL_TYPE_LOAD,
> >> + 0 // DetailStatus -- unused here
> >> + );
> >> +
> >
> > 1. With the BootCurrent, this change is not necessary because others
> > can hook PcdProgressCodeOsLoaderLoad to get the current loading boot
> > option.
>
> As long as we can clear my points (1) and (2) above, I'd be fine with this
> approach.
>
> >
> >> Status = gBS->LoadImage (
> >> TRUE,
> >> gImageHandle,
> >> FilePath,
> >> FileBuffer,
> >> FileSize,
> >> &ImageHandle
> >> );
> >> }
> >> if (FileBuffer != NULL) {
> >> FreePool (FileBuffer);
> >> }
> >> if (FilePath != NULL) {
> >> FreePool (FilePath);
> >> }
> >>
> >> if (EFI_ERROR (Status)) {
> >> //
> >> // Report Status Code to indicate that the failure to load boot option
> >> //
> >> REPORT_STATUS_CODE (
> >> EFI_ERROR_CODE | EFI_ERROR_MINOR,
> >> (EFI_SOFTWARE_DXE_BS_DRIVER |
> >> EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR)
> >> );
> >> + BmReportOsLoaderDetail (
> >> + OsLoaderDetail,
> >> + OsLoaderDetailSize,
> >> + EDKII_OS_LOADER_DETAIL_TYPE_LOAD_ERROR,
> >> + Status
> >> + );
> >> +
> >
> > 2. I think firstly, the OsLoaderDetail is not needed.
> > Secondly, I prefer to submit a PI spec change to include extended data
> > for EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR status code.
> > Instead of inventing a new status code.
>
> I understand your point, and in theory I agree with it, but in practice, I cannot.
If there is no similar status code for a certain purpose, I agree to invent a new status
code. And when the new status code is proven by time to be mature enough, it can go
to PI spec.
But for this case, since PI spec already has such status code, I prefer to at least try
to change the spec. If PIWG doesn't agree, we can go on using the new status code.
>
> My experience with Mantis tickets is not good:
>
> (a) First, I would have to write up the proposal. That's fine, I can do that; have
> done it before.
>
>
> (b) Second, I'd have to *call* the meetings. Please locate the email titled
>
> a reminder about MANTIS usage
>
> from Mark, (date: 6 Sep 2017), cross-posted to the PIWG/USWG/ASWG lists.
>
> I'm not allowed to quote the email verbatim -- all of these lists are confidential -
> -, but if you look up the paragraph starting with "Best practice", Mark basically
> implied, "file the the Mantis ticket, then dial in to the next meeting and sell your
> proposal to the WG *verbally*".
>
> I *cannot* do that. The WG meetings always take place early afternoon in
> Pacific (US West Coast) timezone, which is around *midnight* in my timezone.
>
> In addition, I don't even have time to attend the confcalls that I'm invited to
> within Red Hat!
>
> I don't understand why a carefully written Mantis ticket is not sufficient for
> discussion, but apparently it isn't. :/
>
>
> (c) Case in point, please see Mantis ticket #1736, titled
>
> lacking specification of EFI_BOOT_SCRIPT_WIDTH in PI 1.5 / Vol 5 /
> 8.7.1 Save State Write
>
> which I had originally filed in December 2016.
>
> In February 2017, I was asked for more details. I said, "fair enough", and I spent
> half a day writing up the requested change in *full* detail.
> I gave editing instructions of the form
>
> change this to that
>
> and
>
> add/delete the following
>
> as recommended in Mark's email (b).
>
> You can see my write-up in note 0005044. After that note, there have been
> *zero* updates to the ticket; since February 2017.
>
>
> So the fact is, unless you have time to call the WG meetings every week, and
> push *live* for the change that you want, you have no chance at getting stuff
> into the specs.
>
>
> I mean, in his email (b), Mark suggests asking a "proxy" for representing the
> change request, to anyone that cannot dial in. Well, can *you* be my proxy on
> the PIWG meeting, if I write up the change request? You certainly understand
> the problem space, and you maintain the corresponding reference code in edk2.
> I just doubt you have time for conference calls, same as I.
>
> UefiBootManagerLib already reports an extended status code, with
> EDKII_SET_VARIABLE_STATUS payload. Introducing another edk2-specific status
> code requires no slow negotiation, and it can be adopted by others, in practice,
> if they like it. Once it is wide-spread practice, it can be more easily codified in
> the spec. The specifics for the first implementation can be -- should be --
> discussed on this open list; we had better not standardize something that has
> zero implementations just yet.
>
> Anyway, I'm willing to go through the standardization process, but only if it
> doesn't require me to pick up the phone every week (esp. at midnight).
>
> Thanks
> Laszlo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report EDKII_OS_LOADER_DETAIL status code
2017-11-23 14:53 ` Ni, Ruiyu
@ 2017-11-23 17:08 ` Laszlo Ersek
2017-11-23 23:00 ` Ni, Ruiyu
0 siblings, 1 reply; 14+ messages in thread
From: Laszlo Ersek @ 2017-11-23 17:08 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel-01
Cc: Ard Biesheuvel, Justen, Jordan L, Dong, Eric, Zeng, Star,
Doran, Mark
On 11/23/17 15:53, Ni, Ruiyu wrote:
> Comments below.
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Thursday, November 23, 2017 10:03 PM
>> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star
>> <star.zeng@intel.com>; Doran, Mark <mark.doran@intel.com>
>> Subject: Re: [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report
>> EDKII_OS_LOADER_DETAIL status code
>>
>> Hello Ray,
>>
>> (CC Mark Doran for the last part of my email)
>>
>> On 11/23/17 06:21, Ni, Ruiyu wrote:
>>> Laszlo,
>>> When booting a boot option, UefiBootManagerBoot() sets a Boot####
>>> variable and saves #### to BootCurrent variable.
>>> So all the details (except the EDKII_OS_LOADER_DETAIL.Status) can be
>>> retrieved from reading Boot#### variable.
>>
>> I have two counter-arguments:
>>
>> (1) I would like to minimize the number of accesses to non-volatile UEFI
>> variables. Dependent on the virtualization platform and the (virtual) flash chip
>> structure (for example, flash block size), flash access can be very slow.
>>
>> I'm not 100% sure whether this performance problem affects flash *reads* as
>> well, or if it affects flash writes only. (It definitely affects flash writes.)
>
> NV *read* should have no performance impact since EDKII variable driver's
> implementation caches the flash in memory. I am not sure about that
> in OVMF.
Yeah it should work the same. Thanks!
> But I think a good implementation of variable driver should
> consider such *read* optimization.
> Performance needs to be considered, but simpler/easy-maintain
> code takes higher priority.
>
>>
>>
>> (2) The delivery of status codes to registered handlers is asynchronous, not
>> synchronous. Handlers are registered at various task priority levels, and if the
>> reporting occurs at the same or higher TPL, then the delivery will be delayed.
>> (According to the PI spec, it is even possible to report several status codes
>> before the first will be delivered -- basically the codes are queued until the TPL is
>> reduced sufficiently.) This is why all data has to be embedded in the status code
>> structure itself.
>>
>> Like in any other notify function's case, it is prudent to register status code
>> handlers at the least strict TPL that can work for the actual processing. (The
>> internals of the handler function can even put an upper limit on the TPL; for
>> example using Simple Text Output prevents us from going higher than
>> TPL_NOTIFY.) For this reason I chose TPL_CALLBACK for the OVMF status code
>> handler.
>>
>> Now, if we can *guarantee* that these status codes are only reported at the
>> TPL_APPLICATION level, then a TPL_CALLBACK status code handler will be
>> synchronous in effect, and then the status code structure can carry references
>> to other (external) things in the system too, such as UEFI variables.
>>
>> Can we guarantee that EfiBootManagerBoot() is never invoked above
>> TPL_APPLICATION?
> Yes. It should be. Because gBS->LoadImage()/StartImage() per UEFI spec TPL
> restriction rule, should be run at TPL_APPLICATION.
> And if there is no special reason (handler runs too slow), the status handler
> is registered at TPL_HIGH so that the handler is called synchronously.
OK, the TPL_APPLICATION promise will work for me. I missed the LoadImage
/ StartImage implications; good point!
(I can't register the handler at TPL_HIGH, because the handler calls
AsciiPrint, which internally uses gST->ConOut, and that (i.e.,
SimpleTextOut) cannot be used above TPL_NOTIFY. But the TPL_CALLBACK
handler will work fine with the TPL_APPLICATION report.)
>
>>
>>>
>>> 4 more comments below.
>>
>> [snip]
>>
>>>> + BmReportOsLoaderDetail (
>>>> + OsLoaderDetail,
>>>> + OsLoaderDetailSize,
>>>> + EDKII_OS_LOADER_DETAIL_TYPE_LOAD,
>>>> + 0 // DetailStatus -- unused here
>>>> + );
>>>> +
>>>
>>> 1. With the BootCurrent, this change is not necessary because others
>>> can hook PcdProgressCodeOsLoaderLoad to get the current loading boot
>>> option.
>>
>> As long as we can clear my points (1) and (2) above, I'd be fine with this
>> approach.
>>
>>>
>>>> Status = gBS->LoadImage (
>>>> TRUE,
>>>> gImageHandle,
>>>> FilePath,
>>>> FileBuffer,
>>>> FileSize,
>>>> &ImageHandle
>>>> );
>>>> }
>>>> if (FileBuffer != NULL) {
>>>> FreePool (FileBuffer);
>>>> }
>>>> if (FilePath != NULL) {
>>>> FreePool (FilePath);
>>>> }
>>>>
>>>> if (EFI_ERROR (Status)) {
>>>> //
>>>> // Report Status Code to indicate that the failure to load boot option
>>>> //
>>>> REPORT_STATUS_CODE (
>>>> EFI_ERROR_CODE | EFI_ERROR_MINOR,
>>>> (EFI_SOFTWARE_DXE_BS_DRIVER |
>>>> EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR)
>>>> );
>>>> + BmReportOsLoaderDetail (
>>>> + OsLoaderDetail,
>>>> + OsLoaderDetailSize,
>>>> + EDKII_OS_LOADER_DETAIL_TYPE_LOAD_ERROR,
>>>> + Status
>>>> + );
>>>> +
>>>
>>> 2. I think firstly, the OsLoaderDetail is not needed.
>>> Secondly, I prefer to submit a PI spec change to include extended data
>>> for EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR status code.
>>> Instead of inventing a new status code.
>>
>> I understand your point, and in theory I agree with it, but in practice, I cannot.
>
> If there is no similar status code for a certain purpose, I agree to invent a new status
> code. And when the new status code is proven by time to be mature enough, it can go
> to PI spec.
> But for this case, since PI spec already has such status code, I prefer to at least try
> to change the spec. If PIWG doesn't agree, we can go on using the new status code.
Oh I'm not worried about PIWG disagreement. Getting strong disagreement
*quickly* is actually a very good outcome, because it tells us how to
proceed.
What I'm worried about is a drawn-out process that takes forever (and
requires me to spend a disproportionate amount of time on the phone, and
at bad times).
Unless there is some kind of promise that I can work with the PIWG in a
timely manner *without* the phone, it doesn't make sense for me to start
the process. I'll ask on the PIWG list first.
Thanks,
Laszlo
>
>>
>> My experience with Mantis tickets is not good:
>>
>> (a) First, I would have to write up the proposal. That's fine, I can do that; have
>> done it before.
>>
>>
>> (b) Second, I'd have to *call* the meetings. Please locate the email titled
>>
>> a reminder about MANTIS usage
>>
>> from Mark, (date: 6 Sep 2017), cross-posted to the PIWG/USWG/ASWG lists.
>>
>> I'm not allowed to quote the email verbatim -- all of these lists are confidential -
>> -, but if you look up the paragraph starting with "Best practice", Mark basically
>> implied, "file the the Mantis ticket, then dial in to the next meeting and sell your
>> proposal to the WG *verbally*".
>>
>> I *cannot* do that. The WG meetings always take place early afternoon in
>> Pacific (US West Coast) timezone, which is around *midnight* in my timezone.
>>
>> In addition, I don't even have time to attend the confcalls that I'm invited to
>> within Red Hat!
>>
>> I don't understand why a carefully written Mantis ticket is not sufficient for
>> discussion, but apparently it isn't. :/
>>
>>
>> (c) Case in point, please see Mantis ticket #1736, titled
>>
>> lacking specification of EFI_BOOT_SCRIPT_WIDTH in PI 1.5 / Vol 5 /
>> 8.7.1 Save State Write
>>
>> which I had originally filed in December 2016.
>>
>> In February 2017, I was asked for more details. I said, "fair enough", and I spent
>> half a day writing up the requested change in *full* detail.
>> I gave editing instructions of the form
>>
>> change this to that
>>
>> and
>>
>> add/delete the following
>>
>> as recommended in Mark's email (b).
>>
>> You can see my write-up in note 0005044. After that note, there have been
>> *zero* updates to the ticket; since February 2017.
>>
>>
>> So the fact is, unless you have time to call the WG meetings every week, and
>> push *live* for the change that you want, you have no chance at getting stuff
>> into the specs.
>>
>>
>> I mean, in his email (b), Mark suggests asking a "proxy" for representing the
>> change request, to anyone that cannot dial in. Well, can *you* be my proxy on
>> the PIWG meeting, if I write up the change request? You certainly understand
>> the problem space, and you maintain the corresponding reference code in edk2.
>> I just doubt you have time for conference calls, same as I.
>>
>> UefiBootManagerLib already reports an extended status code, with
>> EDKII_SET_VARIABLE_STATUS payload. Introducing another edk2-specific status
>> code requires no slow negotiation, and it can be adopted by others, in practice,
>> if they like it. Once it is wide-spread practice, it can be more easily codified in
>> the spec. The specifics for the first implementation can be -- should be --
>> discussed on this open list; we had better not standardize something that has
>> zero implementations just yet.
>>
>> Anyway, I'm willing to go through the standardization process, but only if it
>> doesn't require me to pick up the phone every week (esp. at midnight).
>>
>> Thanks
>> Laszlo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report EDKII_OS_LOADER_DETAIL status code
2017-11-23 17:08 ` Laszlo Ersek
@ 2017-11-23 23:00 ` Ni, Ruiyu
0 siblings, 0 replies; 14+ messages in thread
From: Ni, Ruiyu @ 2017-11-23 23:00 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel-01, Ard Biesheuvel, Justen, Jordan L, Dong, Eric,
Zeng, Star, Doran, Mark
Sent from a small-screen device
在 2017年11月24日，上午1:08，Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> 写道：
On 11/23/17 15:53, Ni, Ruiyu wrote:
Comments below.
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Thursday, November 23, 2017 10:03 PM
To: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>; Justen, Jordan L
<jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Zeng, Star
<star.zeng@intel.com<mailto:star.zeng@intel.com>>; Doran, Mark <mark.doran@intel.com<mailto:mark.doran@intel.com>>
Subject: Re: [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report
EDKII_OS_LOADER_DETAIL status code
Hello Ray,
(CC Mark Doran for the last part of my email)
On 11/23/17 06:21, Ni, Ruiyu wrote:
Laszlo,
When booting a boot option, UefiBootManagerBoot() sets a Boot####
variable and saves #### to BootCurrent variable.
So all the details (except the EDKII_OS_LOADER_DETAIL.Status) can be
retrieved from reading Boot#### variable.
I have two counter-arguments:
(1) I would like to minimize the number of accesses to non-volatile UEFI
variables. Dependent on the virtualization platform and the (virtual) flash chip
structure (for example, flash block size), flash access can be very slow.
I'm not 100% sure whether this performance problem affects flash *reads* as
well, or if it affects flash writes only. (It definitely affects flash writes.)
NV *read* should have no performance impact since EDKII variable driver's
implementation caches the flash in memory. I am not sure about that
in OVMF.
Yeah it should work the same. Thanks!
But I think a good implementation of variable driver should
consider such *read* optimization.
Performance needs to be considered, but simpler/easy-maintain
code takes higher priority.
(2) The delivery of status codes to registered handlers is asynchronous, not
synchronous. Handlers are registered at various task priority levels, and if the
reporting occurs at the same or higher TPL, then the delivery will be delayed.
(According to the PI spec, it is even possible to report several status codes
before the first will be delivered -- basically the codes are queued until the TPL is
reduced sufficiently.) This is why all data has to be embedded in the status code
structure itself.
Like in any other notify function's case, it is prudent to register status code
handlers at the least strict TPL that can work for the actual processing. (The
internals of the handler function can even put an upper limit on the TPL; for
example using Simple Text Output prevents us from going higher than
TPL_NOTIFY.) For this reason I chose TPL_CALLBACK for the OVMF status code
handler.
Now, if we can *guarantee* that these status codes are only reported at the
TPL_APPLICATION level, then a TPL_CALLBACK status code handler will be
synchronous in effect, and then the status code structure can carry references
to other (external) things in the system too, such as UEFI variables.
Can we guarantee that EfiBootManagerBoot() is never invoked above
TPL_APPLICATION?
Yes. It should be. Because gBS->LoadImage()/StartImage() per UEFI spec TPL
restriction rule, should be run at TPL_APPLICATION.
And if there is no special reason (handler runs too slow), the status handler
is registered at TPL_HIGH so that the handler is called synchronously.
OK, the TPL_APPLICATION promise will work for me. I missed the LoadImage
/ StartImage implications; good point!
(I can't register the handler at TPL_HIGH, because the handler calls
AsciiPrint, which internally uses gST->ConOut, and that (i.e.,
SimpleTextOut) cannot be used above TPL_NOTIFY. But the TPL_CALLBACK
handler will work fine with the TPL_APPLICATION report.)
TPL_HIGH is just an indicator to tell status router to call the handler in blocking or synchronized way. So handler actually works in the status code reporter’s TPL, which is TPL_APPLICATION.
4 more comments below.
[snip]
+ BmReportOsLoaderDetail (
+ OsLoaderDetail,
+ OsLoaderDetailSize,
+ EDKII_OS_LOADER_DETAIL_TYPE_LOAD,
+ 0 // DetailStatus -- unused here
+ );
+
1. With the BootCurrent, this change is not necessary because others
can hook PcdProgressCodeOsLoaderLoad to get the current loading boot
option.
As long as we can clear my points (1) and (2) above, I'd be fine with this
approach.
Status = gBS->LoadImage (
TRUE,
gImageHandle,
FilePath,
FileBuffer,
FileSize,
&ImageHandle
);
}
if (FileBuffer != NULL) {
FreePool (FileBuffer);
}
if (FilePath != NULL) {
FreePool (FilePath);
}
if (EFI_ERROR (Status)) {
//
// Report Status Code to indicate that the failure to load boot option
//
REPORT_STATUS_CODE (
EFI_ERROR_CODE | EFI_ERROR_MINOR,
(EFI_SOFTWARE_DXE_BS_DRIVER |
EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR)
);
+ BmReportOsLoaderDetail (
+ OsLoaderDetail,
+ OsLoaderDetailSize,
+ EDKII_OS_LOADER_DETAIL_TYPE_LOAD_ERROR,
+ Status
+ );
+
2. I think firstly, the OsLoaderDetail is not needed.
Secondly, I prefer to submit a PI spec change to include extended data
for EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR status code.
Instead of inventing a new status code.
I understand your point, and in theory I agree with it, but in practice, I cannot.
If there is no similar status code for a certain purpose, I agree to invent a new status
code. And when the new status code is proven by time to be mature enough, it can go
to PI spec.
But for this case, since PI spec already has such status code, I prefer to at least try
to change the spec. If PIWG doesn't agree, we can go on using the new status code.
Oh I'm not worried about PIWG disagreement. Getting strong disagreement
*quickly* is actually a very good outcome, because it tells us how to
proceed.
What I'm worried about is a drawn-out process that takes forever (and
requires me to spend a disproportionate amount of time on the phone, and
at bad times).
Unless there is some kind of promise that I can work with the PIWG in a
timely manner *without* the phone, it doesn't make sense for me to start
the process. I'll ask on the PIWG list first.
Thanks,
Laszlo
My experience with Mantis tickets is not good:
(a) First, I would have to write up the proposal. That's fine, I can do that; have
done it before.
(b) Second, I'd have to *call* the meetings. Please locate the email titled
a reminder about MANTIS usage
from Mark, (date: 6 Sep 2017), cross-posted to the PIWG/USWG/ASWG lists.
I'm not allowed to quote the email verbatim -- all of these lists are confidential -
-, but if you look up the paragraph starting with "Best practice", Mark basically
implied, "file the the Mantis ticket, then dial in to the next meeting and sell your
proposal to the WG *verbally*".
I *cannot* do that. The WG meetings always take place early afternoon in
Pacific (US West Coast) timezone, which is around *midnight* in my timezone.
In addition, I don't even have time to attend the confcalls that I'm invited to
within Red Hat!
I don't understand why a carefully written Mantis ticket is not sufficient for
discussion, but apparently it isn't. :/
(c) Case in point, please see Mantis ticket #1736, titled
lacking specification of EFI_BOOT_SCRIPT_WIDTH in PI 1.5 / Vol 5 /
8.7.1 Save State Write
which I had originally filed in December 2016.
In February 2017, I was asked for more details. I said, "fair enough", and I spent
half a day writing up the requested change in *full* detail.
I gave editing instructions of the form
change this to that
and
add/delete the following
as recommended in Mark's email (b).
You can see my write-up in note 0005044. After that note, there have been
*zero* updates to the ticket; since February 2017.
So the fact is, unless you have time to call the WG meetings every week, and
push *live* for the change that you want, you have no chance at getting stuff
into the specs.
I mean, in his email (b), Mark suggests asking a "proxy" for representing the
change request, to anyone that cannot dial in. Well, can *you* be my proxy on
the PIWG meeting, if I write up the change request? You certainly understand
the problem space, and you maintain the corresponding reference code in edk2.
I just doubt you have time for conference calls, same as I.
UefiBootManagerLib already reports an extended status code, with
EDKII_SET_VARIABLE_STATUS payload. Introducing another edk2-specific status
code requires no slow negotiation, and it can be adopted by others, in practice,
if they like it. Once it is wide-spread practice, it can be more easily codified in
the spec. The specifics for the first implementation can be -- should be --
discussed on this open list; we had better not standardize something that has
zero implementations just yet.
Anyway, I'm willing to go through the standardization process, but only if it
doesn't require me to pick up the phone every week (esp. at midnight).
Thanks
Laszlo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] MdeModulePkg/BdsDxe: fall back to a Boot Manager Menu loop before hanging
2017-11-23 13:09 ` Laszlo Ersek
@ 2017-11-27 16:27 ` Laszlo Ersek
0 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-11-27 16:27 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel-01
Cc: Justen, Jordan L, Dong, Eric, Zeng, Star, Ard Biesheuvel
Hi Ray,
On 11/23/17 14:09, Laszlo Ersek wrote:
> On 11/23/17 04:43, Ni, Ruiyu wrote:
>> Thanks for the patch. It follows the idea described in:
>> https://lists.01.org/pipermail/edk2-devel/2017-April/010294.html.
>>
>> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
>
> Thank you, Ray! I might commit this first patch in isolation (to close
> #513) while we continue discussing the rest. Not sure yet. (Depends on
> my workload.)
thanks again for your original suggestion and your review; I've now
committed this single patch, isolated the rest of the series. Commit
d1de487dd2e7.
Cheers,
Laszlo
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-11-27 16:22 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-22 23:58 [PATCH 0/5] MdeModulePkg, OvmfPkg: more easily visible boot progress reporting Laszlo Ersek
2017-11-22 23:58 ` [PATCH 1/5] MdeModulePkg/BdsDxe: fall back to a Boot Manager Menu loop before hanging Laszlo Ersek
2017-11-23 3:43 ` Ni, Ruiyu
2017-11-23 13:09 ` Laszlo Ersek
2017-11-27 16:27 ` Laszlo Ersek
2017-11-22 23:58 ` [PATCH 2/5] MdeModulePkg: introduce the EDKII_OS_LOADER_DETAIL status code payload Laszlo Ersek
2017-11-22 23:58 ` [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report EDKII_OS_LOADER_DETAIL status code Laszlo Ersek
2017-11-23 5:21 ` Ni, Ruiyu
2017-11-23 14:03 ` Laszlo Ersek
2017-11-23 14:53 ` Ni, Ruiyu
2017-11-23 17:08 ` Laszlo Ersek
2017-11-23 23:00 ` Ni, Ruiyu
2017-11-22 23:58 ` [PATCH 4/5] OvmfPkg/PlatformBootManagerLib: print EDKII_OS_LOADER_DETAIL to ConOut Laszlo Ersek
2017-11-22 23:58 ` [PATCH 5/5] OvmfPkg: disable EFI_DEBUG_CODE reporting in RELEASE builds Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox