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

[Laszlo Ersek <lersek@redhat.com>]

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);
>  
>