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 <dandan...@intel.com>; Wu, Hao A <hao.a...@intel.com>; >> Wang, Jian J <jian.j.w...@intel.com>; Ni, Ray <ray...@intel.com>; Sean Brogan >> <sean.bro...@microsoft.com>; Zeng, Star <star.z...@intel.com> >> 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_VALUE Value, > 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 > <ray...@intel.com> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
