Re: [edk2] [PATCH v3 1/5] MdeModulePkg/UefiBootManagerLib: fix LoadImage/StartImage status code rep.
On 02/25/19 09:27, Ni, Ray wrote: > On 2/23/2019 1:16 AM, Laszlo Ersek wrote: >> Yes, I was fully aware of that. >> >> However: >> >> The issue is that, in the BmReportLoadFailure() function, we do some >> work*before* we call REPORT_STATUS_CODE_EX(). We have an ASSERT(), a >> ZeroMem(), and a field assignment. >> >> If status code reporting is disabled for EFI_ERROR_CODE in the platform, >> then said work will be wasted. We can optimize this by checking for >> ReportErrorCodeEnabled() up-front, because we know for sure that later >> on we will report the status code with EFI_ERROR_CODE type. >> >> In other words, this approach is similar to DEBUG_CODE(). In some cases, >> logging a piece of information with DEBUG() takes non-trivial >> computation. And it would be a waste, for example in RELEASE builds, to >> perform the computation, and then throw away only the result (the log >> message). Therefore the DEBUG_CODE macro is used, and the whole work is >> eliminated in RELEASE builds. >> >> The idea is the same here. If the compiler can statically deduce that >> ReportErrorCodeEnabled() will always return FALSE -- for example because >> the ReportStatusCodeLib instance in question looks at >> "PcdReportStatusCodePropertyMask", and the PCD is Fixed-at-Build, and >> the corresponding bit is clear --, then the compiler can eliminate the >> entire BmReportLoadFailure() function. This is good for both flash usage >> and for performance. >> >> I'm fine either way, but first, please confirm again that you really >> want me to remove the ReportErrorCodeEnabled() check, before pushing. >> >> Thanks! >> Laszlo > > Thanks for the explanation. I am fine with both. > Reviewed-by: Ray Ni > Thank you, Ray. I opted for pushing v3 as-is. Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v3 1/5] MdeModulePkg/UefiBootManagerLib: fix LoadImage/StartImage status code rep.
On 2/23/2019 1:16 AM, Laszlo Ersek wrote: Yes, I was fully aware of that. However: The issue is that, in the BmReportLoadFailure() function, we do some work*before* we call REPORT_STATUS_CODE_EX(). We have an ASSERT(), a ZeroMem(), and a field assignment. If status code reporting is disabled for EFI_ERROR_CODE in the platform, then said work will be wasted. We can optimize this by checking for ReportErrorCodeEnabled() up-front, because we know for sure that later on we will report the status code with EFI_ERROR_CODE type. In other words, this approach is similar to DEBUG_CODE(). In some cases, logging a piece of information with DEBUG() takes non-trivial computation. And it would be a waste, for example in RELEASE builds, to perform the computation, and then throw away only the result (the log message). Therefore the DEBUG_CODE macro is used, and the whole work is eliminated in RELEASE builds. The idea is the same here. If the compiler can statically deduce that ReportErrorCodeEnabled() will always return FALSE -- for example because the ReportStatusCodeLib instance in question looks at "PcdReportStatusCodePropertyMask", and the PCD is Fixed-at-Build, and the corresponding bit is clear --, then the compiler can eliminate the entire BmReportLoadFailure() function. This is good for both flash usage and for performance. I'm fine either way, but first, please confirm again that you really want me to remove the ReportErrorCodeEnabled() check, before pushing. Thanks! Laszlo Thanks for the explanation. I am fine with both. Reviewed-by: Ray Ni -- Thanks, Ray ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v3 1/5] MdeModulePkg/UefiBootManagerLib: fix LoadImage/StartImage status code rep.
On 02/22/19 12:50, Ni, Ray wrote:
>
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Thursday, February 21, 2019 6:41 PM
>> To: edk2-devel@lists.01.org
>> Cc: Bi, Dandan ; Wu, Hao A ;
>> Wang, Jian J ; Ni, Ray ; Sean Brogan
>> ; Zeng, Star
>> Subject: [PATCH v3 1/5] MdeModulePkg/UefiBootManagerLib: fix
>> LoadImage/StartImage status code rep.
>
>
>> + if (!ReportErrorCodeEnabled ()) {
>> +return;
>> + }
>
> Sorry I didn't notice this piece of code in V2.
> The if-check-return code is not needed here.
> Because the implementation of ReportStatusCodeLib is
> responsible to do the filter.
> See below:
>
> EFI_STATUS
> InternalReportStatusCode (
> IN EFI_STATUS_CODE_TYPE Type,
> IN EFI_STATUS_CODE_VALUEValue,
> IN UINT32 Instance,
> IN CONST EFI_GUID *CallerId OPTIONAL,
> IN EFI_STATUS_CODE_DATA *Data OPTIONAL
> )
> {
> if ((ReportProgressCodeEnabled() && ((Type) & EFI_STATUS_CODE_TYPE_MASK) ==
> EFI_PROGRESS_CODE) ||
> (ReportErrorCodeEnabled() && ((Type) & EFI_STATUS_CODE_TYPE_MASK) ==
> EFI_ERROR_CODE) ||
> (ReportDebugCodeEnabled() && ((Type) & EFI_STATUS_CODE_TYPE_MASK) ==
> EFI_DEBUG_CODE)) {
> ...
Yes, I was fully aware of that.
However:
The issue is that, in the BmReportLoadFailure() function, we do some
work *before* we call REPORT_STATUS_CODE_EX(). We have an ASSERT(), a
ZeroMem(), and a field assignment.
If status code reporting is disabled for EFI_ERROR_CODE in the platform,
then said work will be wasted. We can optimize this by checking for
ReportErrorCodeEnabled() up-front, because we know for sure that later
on we will report the status code with EFI_ERROR_CODE type.
In other words, this approach is similar to DEBUG_CODE(). In some cases,
logging a piece of information with DEBUG() takes non-trivial
computation. And it would be a waste, for example in RELEASE builds, to
perform the computation, and then throw away only the result (the log
message). Therefore the DEBUG_CODE macro is used, and the whole work is
eliminated in RELEASE builds.
The idea is the same here. If the compiler can statically deduce that
ReportErrorCodeEnabled() will always return FALSE -- for example because
the ReportStatusCodeLib instance in question looks at
"PcdReportStatusCodePropertyMask", and the PCD is Fixed-at-Build, and
the corresponding bit is clear --, then the compiler can eliminate the
entire BmReportLoadFailure() function. This is good for both flash usage
and for performance.
I'm fine either way, but first, please confirm again that you really
want me to remove the ReportErrorCodeEnabled() check, before pushing.
Thanks!
Laszlo
>
>
> With the removal of the three lines code, Reviewed-by: Ray Ni
>
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v3 1/5] MdeModulePkg/UefiBootManagerLib: fix LoadImage/StartImage status code rep.
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, February 21, 2019 6:41 PM
> To: edk2-devel@lists.01.org
> Cc: Bi, Dandan ; Wu, Hao A ;
> Wang, Jian J ; Ni, Ray ; Sean Brogan
> ; Zeng, Star
> Subject: [PATCH v3 1/5] MdeModulePkg/UefiBootManagerLib: fix
> LoadImage/StartImage status code rep.
> + if (!ReportErrorCodeEnabled ()) {
> +return;
> + }
Sorry I didn't notice this piece of code in V2.
The if-check-return code is not needed here.
Because the implementation of ReportStatusCodeLib is
responsible to do the filter.
See below:
EFI_STATUS
InternalReportStatusCode (
IN EFI_STATUS_CODE_TYPE Type,
IN EFI_STATUS_CODE_VALUEValue,
IN UINT32 Instance,
IN CONST EFI_GUID *CallerId OPTIONAL,
IN EFI_STATUS_CODE_DATA *Data OPTIONAL
)
{
if ((ReportProgressCodeEnabled() && ((Type) & EFI_STATUS_CODE_TYPE_MASK) ==
EFI_PROGRESS_CODE) ||
(ReportErrorCodeEnabled() && ((Type) & EFI_STATUS_CODE_TYPE_MASK) ==
EFI_ERROR_CODE) ||
(ReportDebugCodeEnabled() && ((Type) & EFI_STATUS_CODE_TYPE_MASK) ==
EFI_DEBUG_CODE)) {
...
With the removal of the three lines code, Reviewed-by: Ray Ni
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v3 1/5] MdeModulePkg/UefiBootManagerLib: fix LoadImage/StartImage status code rep.
On 02/22/19 02:05, Bi, Dandan wrote:
> Hi Laszlo,
>
> Thanks for helping fix it.
> Reviewed-by: Bi Dandan
Thanks!
I'll wait for Ray's review too.
Cheers!
Laszlo
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Thursday, February 21, 2019 6:41 PM
>> To: edk2-devel@lists.01.org
>> Cc: Bi, Dandan ; Wu, Hao A ;
>> Wang, Jian J ; Ni, Ray ; Sean
>> Brogan ; Zeng, Star
>> Subject: [PATCH v3 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 BmReportLoadFailure() 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:
>> v3:
>>
>> - rename BmReportImageFailure() to BmReportLoadFailure() [Ray]
>>
>> - eliminate PaddingStart and PaddingSize; zero out the full ExtendedData
>> struct [Ray]
>>
>> - don't pick up Ard's R-b due to the change above being functional in
>> nature
>>
>> v2:
>> - new in v2
>>
>> MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h | 1 +
>> MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 65
>> ++--
>> 2 files changed, 48 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..02ff354ef6a3 100644
>> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> @@ -1667,6 +1667,51 @@ 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
>> +BmReportLoadFailure (
>> + IN UINT32 ErrorCode,
>> + IN EFI_STATUS FailureStatus
>> + )
>> +{
>> + EFI_RETURN_STATUS_EXTENDED_DATA ExtendedData;
>> +
>> + if (!ReportErrorCodeEnabled ()) {
>> +return;
>> + }
>> +
>> + ASSERT (
>> +(ErrorCode == EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR) ||
>> +(ErrorCode == EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED)
>> +);
>> +
>> + ZeroMem (, sizeof (ExtendedData));
>> + ExtendedData.ReturnStatus = FailureStatus;
>> +
>> + REPORT_STATUS_CODE_EX (
>> +(EFI_ERROR_CODE | EFI_ERROR_MINOR),
>> +(EFI_SOFTWARE_DXE_BS_DRIVER | ErrorCode),
>> +0,
>> +NULL,
>> +NULL,
>> + + 1,
>> +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 +1867,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)
>> -);
>> + BmReportLoadFailure
>> (EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR,
>> + Status);
>>BootOption->Status = Status;
>>
Re: [edk2] [PATCH v3 1/5] MdeModulePkg/UefiBootManagerLib: fix LoadImage/StartImage status code rep.
Hi Laszlo,
Thanks for helping fix it.
Reviewed-by: Bi Dandan
Thanks,
Dandan
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, February 21, 2019 6:41 PM
> To: edk2-devel@lists.01.org
> Cc: Bi, Dandan ; Wu, Hao A ;
> Wang, Jian J ; Ni, Ray ; Sean
> Brogan ; Zeng, Star
> Subject: [PATCH v3 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 BmReportLoadFailure() 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:
> v3:
>
> - rename BmReportImageFailure() to BmReportLoadFailure() [Ray]
>
> - eliminate PaddingStart and PaddingSize; zero out the full ExtendedData
> struct [Ray]
>
> - don't pick up Ard's R-b due to the change above being functional in
> nature
>
> v2:
> - new in v2
>
> MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h | 1 +
> MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 65
> ++--
> 2 files changed, 48 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..02ff354ef6a3 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> @@ -1667,6 +1667,51 @@ 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
> +BmReportLoadFailure (
> + IN UINT32 ErrorCode,
> + IN EFI_STATUS FailureStatus
> + )
> +{
> + EFI_RETURN_STATUS_EXTENDED_DATA ExtendedData;
> +
> + if (!ReportErrorCodeEnabled ()) {
> +return;
> + }
> +
> + ASSERT (
> +(ErrorCode == EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR) ||
> +(ErrorCode == EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED)
> +);
> +
> + ZeroMem (, sizeof (ExtendedData));
> + ExtendedData.ReturnStatus = FailureStatus;
> +
> + REPORT_STATUS_CODE_EX (
> +(EFI_ERROR_CODE | EFI_ERROR_MINOR),
> +(EFI_SOFTWARE_DXE_BS_DRIVER | ErrorCode),
> +0,
> +NULL,
> +NULL,
> + + 1,
> +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 +1867,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)
> -);
> + BmReportLoadFailure
> (EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR,
> + Status);
>BootOption->Status = Status;
>//
>// Destroy the RAM disk
> @@ -1911,15 +1948,7 @@ EfiBootManagerBoot (
> //
> // Report Status Code with the failure status to indicate that boot
> failure
> //
> -REPORT_STATUS_CODE_EX (
>
