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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2026 Red Hat, Inc.