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

[Laszlo Ersek <lersek@redhat.com>]

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