Re: [edk2] [PATCH v2 1/5] MdeModulePkg/UefiBootManagerLib: fix LoadImage/StartImage status code rep.
On 02/20/19 14:17, Ni, Ray wrote:
> On 2/20/2019 4:16 PM, Laszlo Ersek wrote:
>> In the EFI_RETURN_STATUS_EXTENDED_DATA structure from PI-1.7, there
>> may be
>> padding between the DataHeader and ReturnStatus members. The
>> REPORT_STATUS_CODE_EX() macro starts populating the structure immediately
>> after DataHeader, therefore the source data must provide for the padding.
>>
>> Extract the BmReportImageFailure() function from EfiBootManagerBoot(),
>> prepare a zero padding (if any) in a temporary
>> EFI_RETURN_STATUS_EXTENDED_DATA object, and fix the
>> REPORT_STATUS_CODE_EX() macro invocation.
>>
>> Cc: Dandan Bi
>> Cc: Hao Wu
>> Cc: Jian J Wang
>> Cc: Ray Ni
>> Cc: Sean Brogan
>> Cc: Star Zeng
>> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1539
>> Fixes: c2cf8720a5aad74230767a1f11bade2d86de3745
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek
>> ---
>>
>> Notes:
>> v2:
>> - new in v2
>>
>> MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h | 1 +
>> MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 69
>> +++-
>> 2 files changed, 52 insertions(+), 18 deletions(-)
>>
>> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
>> b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
>> index 978fbff966f6..0fef63fceedf 100644
>> --- a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
>> +++ b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
>> @@ -51,6 +51,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
>> EITHER EXPRESS OR IMPLIED.
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> index 9be1633b7480..ffb98c6c9b83 100644
>> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> @@ -1667,6 +1667,55 @@ BmIsBootManagerMenuFilePath (
>> return FALSE;
>> }
>> +/**
>> + Report status code with EFI_RETURN_STATUS_EXTENDED_DATA about
>> LoadImage() or
>> + StartImage() failure.
>> +
>> + @param[in] ErrorCode An Error Code in the Software Class, DXE
>> Boot
>> + Service Driver Subclass. ErrorCode will
>> be used to
>> + compose the Value parameter for status code
>> + reporting. Must be one of
>> + EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR and
>> + EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED.
>> +
>> + @param[in] FailureStatus The failure status returned by the boot
>> service
>> + that should be reported.
>> +**/
>> +VOID
>> +BmReportImageFailure (
> Laszlo,
> Thanks for quick fixing this issue.
> To match the status code it reports, how about rename the function as
> "BmReportLoadFailure"?
Sure, I can do that.
> Another minor comments in below.
>> + IN UINT32 ErrorCode,
>> + IN EFI_STATUS FailureStatus
>> + )
>> +{
>> + EFI_RETURN_STATUS_EXTENDED_DATA ExtendedData;
>> + VOID *PaddingStart;
>> + UINTN PaddingSize;
>> +
>> + if (!ReportErrorCodeEnabled ()) {
>> + return;
>> + }
>> +
>> + ASSERT (
>> + (ErrorCode == EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR) ||
>> + (ErrorCode == EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED)
>> + );
>> +
>> + PaddingStart = + 1;
>> + PaddingSize = (UINTN) - (UINTN)
>> PaddingStart;
>> + ZeroMem (PaddingStart, PaddingSize);
>
> I prefer "ZeroMem (, sizeof (ExtendedData))" instead of
> introducing local variables such as PaddingStart/PaddingSize.
> This makes code more good-looking:).
I agree that that looks more natural. I didn't know how much you'd like
me to "optimize" this logic. Because,
(1) an optimizing compiler can eliminate PaddingSize from the ZeroMem()
call (replacing it with a constant); in other words, the PaddingSize
calculation would have no runtime cost,
(2) the ZeroMem() call would have to clear fewer bytes.
I hesitated between the smallest possible ZeroMem() and the
easiest-to-read ZeroMem(). I will rework it in v3.
>
>> + ExtendedData.ReturnStatus = FailureStatus;
>> +
>> + REPORT_STATUS_CODE_EX (
>> + (EFI_ERROR_CODE | EFI_ERROR_MINOR),
>> + (EFI_SOFTWARE_DXE_BS_DRIVER | ErrorCode),
>> + 0,
>> + NULL,
>> + NULL,
>> + PaddingStart,
> + 1
>
>> + PaddingSize + sizeof (ExtendedData.ReturnStatus)
> sizeof (ExtendedData) - sizeof (ExtendedData.DataHeader)
Yes.
Thanks,
Laszlo
>> + );
>> +}
>> +
>> /**
>> Attempt to boot the EFI boot option. This routine sets
>> L"BootCurent" and
>> also signals the EFI ready to boot event. If the device path for
>> the option
>> @@ -1822,15 +1871,7 @@ EfiBootManagerBoot (
>> //
>> // Report Status Code with the failure status to indicate that
>> the failure to load boot option
>> //
>>
Re: [edk2] [PATCH v2 1/5] MdeModulePkg/UefiBootManagerLib: fix LoadImage/StartImage status code rep.
On 2/20/2019 4:16 PM, Laszlo Ersek wrote:
In the EFI_RETURN_STATUS_EXTENDED_DATA structure from PI-1.7, there may be
padding between the DataHeader and ReturnStatus members. The
REPORT_STATUS_CODE_EX() macro starts populating the structure immediately
after DataHeader, therefore the source data must provide for the padding.
Extract the BmReportImageFailure() function from EfiBootManagerBoot(),
prepare a zero padding (if any) in a temporary
EFI_RETURN_STATUS_EXTENDED_DATA object, and fix the
REPORT_STATUS_CODE_EX() macro invocation.
Cc: Dandan Bi
Cc: Hao Wu
Cc: Jian J Wang
Cc: Ray Ni
Cc: Sean Brogan
Cc: Star Zeng
Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1539
Fixes: c2cf8720a5aad74230767a1f11bade2d86de3745
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek
---
Notes:
v2:
- new in v2
MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h | 1 +
MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 69 +++-
2 files changed, 52 insertions(+), 18 deletions(-)
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
index 978fbff966f6..0fef63fceedf 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
+++ b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
@@ -51,6 +51,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
EXPRESS OR IMPLIED.
#include
#include
#include
+#include
#include
#include
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
index 9be1633b7480..ffb98c6c9b83 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
@@ -1667,6 +1667,55 @@ BmIsBootManagerMenuFilePath (
return FALSE;
}
+/**
+ Report status code with EFI_RETURN_STATUS_EXTENDED_DATA about LoadImage() or
+ StartImage() failure.
+
+ @param[in] ErrorCode An Error Code in the Software Class, DXE Boot
+Service Driver Subclass. ErrorCode will be used to
+compose the Value parameter for status code
+reporting. Must be one of
+EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR and
+EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED.
+
+ @param[in] FailureStatus The failure status returned by the boot service
+that should be reported.
+**/
+VOID
+BmReportImageFailure (
Laszlo,
Thanks for quick fixing this issue.
To match the status code it reports, how about rename the function as
"BmReportLoadFailure"?
Another minor comments in below.
+ IN UINT32 ErrorCode,
+ IN EFI_STATUS FailureStatus
+ )
+{
+ EFI_RETURN_STATUS_EXTENDED_DATA ExtendedData;
+ VOID*PaddingStart;
+ UINTN PaddingSize;
+
+ if (!ReportErrorCodeEnabled ()) {
+return;
+ }
+
+ ASSERT (
+(ErrorCode == EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR) ||
+(ErrorCode == EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED)
+);
+
+ PaddingStart = + 1;
+ PaddingSize = (UINTN) - (UINTN) PaddingStart;
+ ZeroMem (PaddingStart, PaddingSize);
I prefer "ZeroMem (, sizeof (ExtendedData))" instead of
introducing local variables such as PaddingStart/PaddingSize.
This makes code more good-looking:).
+ ExtendedData.ReturnStatus = FailureStatus;
+
+ REPORT_STATUS_CODE_EX (
+(EFI_ERROR_CODE | EFI_ERROR_MINOR),
+(EFI_SOFTWARE_DXE_BS_DRIVER | ErrorCode),
+0,
+NULL,
+NULL,
+PaddingStart,
+ 1
+PaddingSize + sizeof (ExtendedData.ReturnStatus)
sizeof (ExtendedData) - sizeof (ExtendedData.DataHeader)
+);
+}
+
/**
Attempt to boot the EFI boot option. This routine sets L"BootCurent" and
also signals the EFI ready to boot event. If the device path for the option
@@ -1822,15 +1871,7 @@ EfiBootManagerBoot (
//
// Report Status Code with the failure status to indicate that the
failure to load boot option
//
- REPORT_STATUS_CODE_EX (
-EFI_ERROR_CODE | EFI_ERROR_MINOR,
-(EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR),
-0,
-NULL,
-NULL,
-,
-sizeof (EFI_STATUS)
-);
+ BmReportImageFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR, Status);
BootOption->Status = Status;
//
// Destroy the RAM disk
@@ -1911,15 +1952,7 @@ EfiBootManagerBoot (
//
// Report Status Code with the failure status to indicate that boot
failure
//
-REPORT_STATUS_CODE_EX (
- EFI_ERROR_CODE | EFI_ERROR_MINOR,
- (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED),
- 0,
- NULL,
- NULL,
- ,
- sizeof (EFI_STATUS)
- );
+BmReportImageFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED, Status);
}
PERF_END_EX (gImageHandle,
[edk2] [PATCH v2 1/5] MdeModulePkg/UefiBootManagerLib: fix LoadImage/StartImage status code rep.
In the EFI_RETURN_STATUS_EXTENDED_DATA structure from PI-1.7, there may be
padding between the DataHeader and ReturnStatus members. The
REPORT_STATUS_CODE_EX() macro starts populating the structure immediately
after DataHeader, therefore the source data must provide for the padding.
Extract the BmReportImageFailure() function from EfiBootManagerBoot(),
prepare a zero padding (if any) in a temporary
EFI_RETURN_STATUS_EXTENDED_DATA object, and fix the
REPORT_STATUS_CODE_EX() macro invocation.
Cc: Dandan Bi
Cc: Hao Wu
Cc: Jian J Wang
Cc: Ray Ni
Cc: Sean Brogan
Cc: Star Zeng
Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1539
Fixes: c2cf8720a5aad74230767a1f11bade2d86de3745
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek
---
Notes:
v2:
- new in v2
MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h | 1 +
MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 69 +++-
2 files changed, 52 insertions(+), 18 deletions(-)
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
index 978fbff966f6..0fef63fceedf 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
+++ b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
@@ -51,6 +51,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
EXPRESS OR IMPLIED.
#include
#include
#include
+#include
#include
#include
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
index 9be1633b7480..ffb98c6c9b83 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
@@ -1667,6 +1667,55 @@ BmIsBootManagerMenuFilePath (
return FALSE;
}
+/**
+ Report status code with EFI_RETURN_STATUS_EXTENDED_DATA about LoadImage() or
+ StartImage() failure.
+
+ @param[in] ErrorCode An Error Code in the Software Class, DXE Boot
+Service Driver Subclass. ErrorCode will be used to
+compose the Value parameter for status code
+reporting. Must be one of
+EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR and
+EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED.
+
+ @param[in] FailureStatus The failure status returned by the boot service
+that should be reported.
+**/
+VOID
+BmReportImageFailure (
+ IN UINT32 ErrorCode,
+ IN EFI_STATUS FailureStatus
+ )
+{
+ EFI_RETURN_STATUS_EXTENDED_DATA ExtendedData;
+ VOID*PaddingStart;
+ UINTN PaddingSize;
+
+ if (!ReportErrorCodeEnabled ()) {
+return;
+ }
+
+ ASSERT (
+(ErrorCode == EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR) ||
+(ErrorCode == EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED)
+);
+
+ PaddingStart = + 1;
+ PaddingSize = (UINTN) - (UINTN) PaddingStart;
+ ZeroMem (PaddingStart, PaddingSize);
+ ExtendedData.ReturnStatus = FailureStatus;
+
+ REPORT_STATUS_CODE_EX (
+(EFI_ERROR_CODE | EFI_ERROR_MINOR),
+(EFI_SOFTWARE_DXE_BS_DRIVER | ErrorCode),
+0,
+NULL,
+NULL,
+PaddingStart,
+PaddingSize + sizeof (ExtendedData.ReturnStatus)
+);
+}
+
/**
Attempt to boot the EFI boot option. This routine sets L"BootCurent" and
also signals the EFI ready to boot event. If the device path for the option
@@ -1822,15 +1871,7 @@ EfiBootManagerBoot (
//
// Report Status Code with the failure status to indicate that the
failure to load boot option
//
- REPORT_STATUS_CODE_EX (
-EFI_ERROR_CODE | EFI_ERROR_MINOR,
-(EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR),
-0,
-NULL,
-NULL,
-,
-sizeof (EFI_STATUS)
-);
+ BmReportImageFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR, Status);
BootOption->Status = Status;
//
// Destroy the RAM disk
@@ -1911,15 +1952,7 @@ EfiBootManagerBoot (
//
// Report Status Code with the failure status to indicate that boot failure
//
-REPORT_STATUS_CODE_EX (
- EFI_ERROR_CODE | EFI_ERROR_MINOR,
- (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED),
- 0,
- NULL,
- NULL,
- ,
- sizeof (EFI_STATUS)
- );
+BmReportImageFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED, Status);
}
PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber);
--
2.19.1.3.g30247aa5d201
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
