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 8541E21E256BC for ; Thu, 25 Jan 2018 04:38:29 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 906C012E51A; Thu, 25 Jan 2018 12:43:58 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-47.rdu2.redhat.com [10.10.121.47]) by smtp.corp.redhat.com (Postfix) with ESMTP id 32DAC18035; Thu, 25 Jan 2018 12:43:56 +0000 (UTC) To: "Bi, Dandan" , "edk2-devel@lists.01.org" Cc: "Dong, Eric" , "Gao, Liming" References: <1516780771-6560-1-git-send-email-dandan.bi@intel.com> <09a46ff3-7195-6182-6997-c168f980391f@redhat.com> <3C0D5C461C9E904E8F62152F6274C0BB3BA100A3@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <9f4b4c7f-cec7-919b-5794-94cbfc579707@redhat.com> Date: Thu, 25 Jan 2018 13:43:56 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <3C0D5C461C9E904E8F62152F6274C0BB3BA100A3@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 25 Jan 2018 12:43:58 +0000 (UTC) Subject: Re: [patch] UefiCpuPkg/S3Resume: Add more perf enrty for S3 phase X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Jan 2018 12:38:30 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/25/18 05:56, Bi, Dandan wrote: > Hi Laszlo, > > Thank you for your comments. > > Yes. The PERF_INMODULE_START_ID macro is from "ExtendedFirmwarePerformance.h" which is added in the new performance infrastructure patches([ mail subject: patch 0/8] Update EDKII Performance infrastructure based on ACPI FPDT table). That is the dependency I mean. > > I will send V2 of this patch and the patch which remove Pref code after "new performance infrastructure" patches have been committed . Then I will refine the commit message of these two patches, add commit hash value of "new performance infrastructure" patches if I need to reference it. Thank you. I had some other questions lower down, about the PERF_START_EX() arguments. Can you please help me understand those better? Thanks! Laszlo > > Thanks, > Dandan > > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Wednesday, January 24, 2018 11:45 PM > To: Bi, Dandan ; edk2-devel@lists.01.org > Cc: Dong, Eric ; Gao, Liming > Subject: Re: [patch] UefiCpuPkg/S3Resume: Add more perf enrty for S3 phase > > On 01/24/18 08:59, Dandan Bi wrote: >> Add more perf entry to hook BootScriptDonePpi/EndOfPeiPpi/ >> EndOfS3Resume. >> >> Notes: This patch depends on the new performance infrastructure. > > Since this statement is going into the commit log, please be more specific about the "new performance infrastructure" (TianoCore BZ or commit hashes etc). > > Also, how is the dependency established? For example, OVMF uses BasePerformanceLibNull; so I think it will see no changes. > > Do you mean the PERF_INMODULE_START_ID macro, from "ExtendedFirmwarePerformance.h"? Ah, that's not committed yet. In this case, please add a reference to the mailing list message (subject and archive URL) that adds it. > > (Actually, the best solution is to reference the TianoCore BZ, and then add the mailing list URL for each patch set submission to the TianoCore BZ.) > >> >> Cc: Eric Dong >> Cc: Laszlo Ersek >> Cc: Liming Gao >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Dandan Bi >> --- >> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 15 >> ++++++++++++++- >> 1 file changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c >> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c >> index b597ac7..d7d2a4d 100644 >> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c >> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c >> @@ -2,11 +2,11 @@ >> This module produces the EFI_PEI_S3_RESUME2_PPI. >> This module works with StandAloneBootScriptExecutor to S3 resume to OS. >> This module will execute the boot script saved during last boot and after that, >> control is passed to OS waking up handler. >> >> - Copyright (c) 2006 - 2017, Intel Corporation. All rights >> reserved.
>> + Copyright (c) 2006 - 2018, Intel Corporation. All rights >> + reserved.
>> Copyright (c) 2017, AMD Incorporated. All rights reserved.
>> >> This program and the accompanying materials >> are licensed and made available under the terms and conditions >> of the BSD License which accompanies this distribution. The @@ >> -21,10 +21,11 @@ #include >> >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> #include >> #include >> @@ -551,13 +552,17 @@ S3ResumeBootOs ( >> PERF_END (NULL, "ScriptExec", NULL, 0); >> >> // >> // Install BootScriptDonePpi >> // >> + PERF_START_EX (NULL, "BootScriptDonePpi", NULL, 0, >> + PERF_INMODULE_START_ID); >> + > > My question here is not really specific to this patch, but I guess asking it this time is just as good as any other time: > > - Why do we pass NULL for Module, rather than gEfiCallerBaseName? > > - We already have START and END calls for the performance measurement; why do we use different Identifier values for the records added? > > (These questions are more for my education than about possible issues in the patch.) > > Thanks! > Laszlo > > >> Status = PeiServicesInstallPpi (&mPpiListPostScriptTable); >> ASSERT_EFI_ERROR (Status); >> >> + PERF_END_EX (NULL, "BootScriptDonePpi", NULL, 0, >> + PERF_INMODULE_END_ID); >> + >> // >> // Get ACPI Table Address >> // >> Facs = (EFI_ACPI_4_0_FIRMWARE_ACPI_CONTROL_STRUCTURE *) ((UINTN) >> (AcpiS3Context->AcpiFacsTable)); >> >> @@ -576,18 +581,26 @@ S3ResumeBootOs ( >> } >> >> // >> // Install EndOfPeiPpi >> // >> + PERF_START_EX (NULL, "EndOfPeiPpi", NULL, 0, >> + PERF_INMODULE_START_ID); >> + >> Status = PeiServicesInstallPpi (&mPpiListEndOfPeiTable); >> ASSERT_EFI_ERROR (Status); >> >> + PERF_END_EX (NULL, "EndOfPeiPpi", NULL, 0, PERF_INMODULE_END_ID); >> + >> // >> // Signal EndOfS3Resume event. >> // >> + PERF_START_EX (NULL, "EndOfS3Resume", NULL, 0, >> + PERF_INMODULE_START_ID); >> + >> SignalEndOfS3Resume (); >> >> + PERF_END_EX (NULL, "EndOfS3Resume", NULL, 0, PERF_INMODULE_END_ID); >> + >> // >> // report status code on S3 resume >> // >> REPORT_STATUS_CODE (EFI_PROGRESS_CODE, EFI_SOFTWARE_PEI_MODULE | >> EFI_SW_PEI_PC_OS_WAKE); >> >> >