From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2D6E3211CD610 for ; Fri, 22 Feb 2019 09:16:12 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4AFF530BC5F5; Fri, 22 Feb 2019 17:16:12 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-126-105.rdu2.redhat.com [10.10.126.105]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5D1AA5D717; Fri, 22 Feb 2019 17:16:10 +0000 (UTC) To: "Ni, Ray" , "edk2-devel@lists.01.org" Cc: "Bi, Dandan" , "Wu, Hao A" , "Wang, Jian J" , Sean Brogan , "Zeng, Star" References: <20190221104112.14995-1-lersek@redhat.com> <20190221104112.14995-2-lersek@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5C02CEB3@SHSMSX104.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <4e739d1b-44a5-f2a2-3f0f-84f3c700e2b2@redhat.com> Date: Fri, 22 Feb 2019 18:16:09 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C02CEB3@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Fri, 22 Feb 2019 17:16:12 +0000 (UTC) Subject: Re: [PATCH v3 1/5] MdeModulePkg/UefiBootManagerLib: fix LoadImage/StartImage status code rep. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 22 Feb 2019 17:16:13 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/22/19 12:50, Ni, Ray wrote: > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@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_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 >