Re: [patch] UefiCpuPkg/S3Resume: Add more perf enrty for S3 phase

["Bi, Dandan" <dandan.bi@intel.com>]

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. 

Thanks,
Dandan

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, January 24, 2018 11:45 PM
To: Bi, Dandan <dandan.bi@intel.com>; edk2-devel@lists.01.org
Cc: Dong, Eric <eric.dong@intel.com>; Gao, Liming <liming.gao@intel.com>
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 <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> ---
>  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.<BR>
> +  Copyright (c) 2006 - 2018, Intel Corporation. All rights 
> + reserved.<BR>
>    Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
>  
>    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 <PiPei.h>
>  
>  #include <Guid/AcpiS3Context.h>
>  #include <Guid/BootScriptExecutorVariable.h>
>  #include <Guid/Performance.h>
> +#include <Guid/ExtendedFirmwarePerformance.h>
>  #include <Guid/EndOfS3Resume.h>
>  #include <Ppi/ReadOnlyVariable2.h>
>  #include <Ppi/S3Resume2.h>
>  #include <Ppi/SmmAccess.h>
>  #include <Ppi/PostBootScriptTable.h>
> @@ -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);
>  
>