Re: [PATCH v2 1/5] MdeModulePkg/UefiBootManagerLib: fix LoadImage/StartImage status code rep.

[Laszlo Ersek <lersek@redhat.com>]

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 <dandan.bi@intel.com>
>> Cc: Hao Wu <hao.a.wu@intel.com>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Sean Brogan <sean.brogan@microsoft.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1539
>> Fixes: c2cf8720a5aad74230767a1f11bade2d86de3745
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> 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 <Guid/MemoryTypeInformation.h>
>>   #include <Guid/FileInfo.h>
>>   #include <Guid/GlobalVariable.h>
>> +#include <Guid/StatusCodeDataTypeId.h>
>>   #include <Guid/StatusCodeDataTypeVariable.h>
>>     #include <Library/PrintLib.h>
>> 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 = &ExtendedData.DataHeader + 1;
>> +  PaddingSize = (UINTN) &ExtendedData.ReturnStatus - (UINTN)
>> PaddingStart;
>> +  ZeroMem (PaddingStart, PaddingSize);
> 
> I prefer "ZeroMem (&ExtendedData, 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,
> &ExtendedData.DataHeader + 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
>>         //
>> -      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,
>> -        &Status,
>> -        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,
>> -      &Status,
>> -      sizeof (EFI_STATUS)
>> -      );
>> +    BmReportImageFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED, Status);
>>     }
>>     PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32)
>> OptionNumber);
>>  
> 
>