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

["Gao, Liming" <liming.gao@intel.com>]

Laszlo:
  Here is PERF macro PERF_START_EX(Handle, Token, Module, TimeStamp, Identifier). 

  Handle is the image handle or device handle. PerfLib can base on Handle to get its FILE guid. 
  Token is the measured token name. 
  Module is the measured module name. PERF_START macro is added before gEfiCallerBaseName is supported. Some old PERF_START uses Token only. This patch just copies one and add it. So, it doesn't use gEfiCallerBaseName. I agree to use gEfiCallerBaseName better.
  TimeStamp is current tick. 
  Identifier will be mapped into progressId in edk2 extended FPDT record from new performance https://lists.01.org/pipermail/edk2-devel/2018-January/020345.html. New performance will save each PERF into one record. It means START PERF entry is one record, END PERF entry is another record. When DP or OS tool parses record, they need to know the record is Start or End. Then, the different ProgressId will be used for Start and End. 

Thanks
Liming
>-----Original Message-----
>From: Laszlo Ersek [mailto:lersek@redhat.com]
>Sent: Thursday, January 25, 2018 8:44 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/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 <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);
>>>
>>>
>>