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 68AA6222A334B for ; Wed, 24 Jan 2018 07:39:49 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3B695A342E; Wed, 24 Jan 2018 15:45:17 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-13.rdu2.redhat.com [10.10.121.13]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0F4AE600CC; Wed, 24 Jan 2018 15:45:15 +0000 (UTC) To: Dandan Bi , edk2-devel@lists.01.org Cc: Eric Dong , Liming Gao References: <1516780771-6560-1-git-send-email-dandan.bi@intel.com> From: Laszlo Ersek Message-ID: <09a46ff3-7195-6182-6997-c168f980391f@redhat.com> Date: Wed, 24 Jan 2018 16:45:15 +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: <1516780771-6560-1-git-send-email-dandan.bi@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Wed, 24 Jan 2018 15:45:17 +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: Wed, 24 Jan 2018 15:39:49 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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); > >