UefiCpuPkg/S3Resume: Add more perf enrty for S3 phase

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

Posted by Dandan Bi 6 years, 11 months ago

Add more perf entry to hook BootScriptDonePpi/EndOfPeiPpi/
EndOfS3Resume.

Notes: This patch depends on the new performance
infrastructure.

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);
+
   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);
 
-- 
1.9.5.msysgit.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Laszlo Ersek 6 years, 11 months ago

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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Bi, Dandan 6 years, 11 months ago

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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Laszlo Ersek 6 years, 11 months ago

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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Gao, Liming 6 years, 11 months ago

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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Laszlo Ersek 6 years, 11 months ago

On 01/26/18 07:02, Gao, Liming wrote:
> 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. 

The question is how the Start and End records are coupled. They need to
be tied together because otherwise it is not possible to calculate a
time difference. I thought that Identifier partook in the coupling. So,
apparently, only Token is used in the coupling, and Identifier is just
an informative progress code.

Thanks
Laszlo


>> -----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);
>>>>
>>>>
>>>
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel