[PATCH 0/6] Change the performance code

[PATCH 0/6] Change the performance code

[Gao, Zhichao]

Add and rdjust the performance code.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Turner <Michael.Turner@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>

Bret Barkelew (6):
  MdeModulePkg/DxeMain: Add performance code
  MdeModule/PeiMain: Add performance code
  MdeModulePkg/UefiBootManagerLib: Change performance code
  SecurityPkg/Tcg2Dxe: Change performance code
  SecurityPkg/Tcg2Pei: Change performance code
  IntelSiliconPkg/IntelVtdDxe: Change the performance code

 .../Feature/VTd/IntelVTdDxe/IntelVTdDxe.c           | 13 +++----------
 MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c       |  2 ++
 MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c       | 10 ++++++++++
 MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c    |  9 ++++-----
 SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c                   |  6 ++----
 SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c                   |  8 +++-----
 6 files changed, 24 insertions(+), 24 deletions(-)

-- 
2.21.0.windows.1

[PATCH 1/6] MdeModulePkg/DxeMain: Add performance code

[Gao, Zhichao]

From: Bret Barkelew <Bret.Barkelew@microsoft.com>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888

Add missing PERF_FUNCTION_END before the return statement.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Turner <Michael.Turner@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c b/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
index c310b40b52..001a26aa62 100644
--- a/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
+++ b/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
@@ -418,6 +418,7 @@ CoreDispatcher (
     //
     // If the dispatcher is running don't let it be restarted.
     //
+    PERF_FUNCTION_END ();
     return EFI_ALREADY_STARTED;
   }
 
@@ -432,6 +433,7 @@ CoreDispatcher (
              &DxeDispatchEvent
              );
   if (EFI_ERROR (Status)) {
+    PERF_FUNCTION_END ();
     return Status;
   }
 
-- 
2.21.0.windows.1

Re: [PATCH 1/6] MdeModulePkg/DxeMain: Add performance code

[Wu, Hao A]

> -----Original Message-----
> From: Gao, Zhichao
> Sent: Monday, June 10, 2019 3:29 PM
> To: devel@edk2.groups.io
> Cc: Bret Barkelew; Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Gao, Liming;
> Sean Brogan; Michael Turner
> Subject: [PATCH 1/6] MdeModulePkg/DxeMain: Add performance code


I suggest to change the title to:
MdeModulePkg/DxeMain: Add missing PERF_FUNCTION_END for error paths

With the change,
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> 
> From: Bret Barkelew <Bret.Barkelew@microsoft.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888
> 
> Add missing PERF_FUNCTION_END before the return statement.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Turner <Michael.Turner@microsoft.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
> b/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
> index c310b40b52..001a26aa62 100644
> --- a/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
> +++ b/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
> @@ -418,6 +418,7 @@ CoreDispatcher (
>      //
>      // If the dispatcher is running don't let it be restarted.
>      //
> +    PERF_FUNCTION_END ();
>      return EFI_ALREADY_STARTED;
>    }
> 
> @@ -432,6 +433,7 @@ CoreDispatcher (
>               &DxeDispatchEvent
>               );
>    if (EFI_ERROR (Status)) {
> +    PERF_FUNCTION_END ();
>      return Status;
>    }
> 
> --
> 2.21.0.windows.1

[PATCH 2/6] MdeModule/PeiMain: Add performance code

[Gao, Zhichao]

From: Bret Barkelew <Bret.Barkelew@microsoft.com>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888

Add performance code for PeiDisaptcher function and Image function.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
index ba2fd0cae1..0caffe653b 100644
--- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
+++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
@@ -983,6 +983,8 @@ PeiDispatcher (
   EFI_FV_FILE_INFO                    FvFileInfo;
   PEI_CORE_FV_HANDLE                  *CoreFvHandle;
 
+  PERF_FUNCTION_BEGIN ();
+
   PeiServices = (CONST EFI_PEI_SERVICES **) &Private->Ps;
   PeimEntryPoint = NULL;
   PeimFileHandle = NULL;
@@ -1004,6 +1006,7 @@ PeiDispatcher (
           Private->CurrentFileHandle   = PeimFileHandle;
           Private->CurrentPeimFvCount  = Index1;
           Private->CurrentPeimCount    = Index2;
+          PERF_LOAD_IMAGE_BEGIN (NULL);
           Status = PeiLoadImage (
                     (CONST EFI_PEI_SERVICES **) &Private->Ps,
                     PeimFileHandle,
@@ -1012,6 +1015,7 @@ PeiDispatcher (
                     &AuthenticationState
                     );
           if (Status == EFI_SUCCESS) {
+            PERF_LOAD_IMAGE_END (PeimFileHandle);
             //
             // PEIM_STATE_REGISTER_FOR_SHADOW move to PEIM_STATE_DONE
             //
@@ -1118,6 +1122,7 @@ PeiDispatcher (
               //
               // For PEIM driver, Load its entry point
               //
+              PERF_LOAD_IMAGE_BEGIN (NULL);
               Status = PeiLoadImage (
                          PeiServices,
                          PeimFileHandle,
@@ -1126,6 +1131,7 @@ PeiDispatcher (
                          &AuthenticationState
                          );
               if (Status == EFI_SUCCESS) {
+                PERF_LOAD_IMAGE_END (PeimFileHandle);
                 //
                 // The PEIM has its dependencies satisfied, and its entry point
                 // has been found, so invoke it.
@@ -1197,6 +1203,7 @@ PeiDispatcher (
                 //
                 // Load PEIM into Memory for Register for shadow PEIM.
                 //
+                PERF_LOAD_IMAGE_BEGIN (NULL);
                 Status = PeiLoadImage (
                            PeiServices,
                            PeimFileHandle,
@@ -1205,6 +1212,7 @@ PeiDispatcher (
                            &AuthenticationState
                            );
                 if (Status == EFI_SUCCESS) {
+                  PERF_LOAD_IMAGE_END (PeimFileHandle);
                   PeimEntryPoint = (EFI_PEIM_ENTRY_POINT2)(UINTN)EntryPoint;
                 }
               }
@@ -1252,6 +1260,8 @@ PeiDispatcher (
     //
   } while (Private->PeimNeedingDispatch && Private->PeimDispatchOnThisPass);
 
+  PERF_FUNCTION_END ();
+
 }
 
 /**
-- 
2.21.0.windows.1

Re: [PATCH 2/6] MdeModule/PeiMain: Add performance code

[Wu, Hao A]

> -----Original Message-----
> From: Gao, Zhichao
> Sent: Monday, June 10, 2019 3:29 PM
> To: devel@edk2.groups.io
> Cc: Bret Barkelew; Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Gao, Liming
> Subject: [PATCH 2/6] MdeModule/PeiMain: Add performance code
> 
> From: Bret Barkelew <Bret.Barkelew@microsoft.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888
> 
> Add performance code for PeiDisaptcher function and Image function.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> index ba2fd0cae1..0caffe653b 100644
> --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> @@ -983,6 +983,8 @@ PeiDispatcher (
>    EFI_FV_FILE_INFO                    FvFileInfo;
>    PEI_CORE_FV_HANDLE                  *CoreFvHandle;
> 
> +  PERF_FUNCTION_BEGIN ();
> +
>    PeiServices = (CONST EFI_PEI_SERVICES **) &Private->Ps;
>    PeimEntryPoint = NULL;
>    PeimFileHandle = NULL;
> @@ -1004,6 +1006,7 @@ PeiDispatcher (
>            Private->CurrentFileHandle   = PeimFileHandle;
>            Private->CurrentPeimFvCount  = Index1;
>            Private->CurrentPeimCount    = Index2;
> +          PERF_LOAD_IMAGE_BEGIN (NULL);
>            Status = PeiLoadImage (
>                      (CONST EFI_PEI_SERVICES **) &Private->Ps,
>                      PeimFileHandle,
> @@ -1012,6 +1015,7 @@ PeiDispatcher (
>                      &AuthenticationState
>                      );
>            if (Status == EFI_SUCCESS) {
> +            PERF_LOAD_IMAGE_END (PeimFileHandle);


One question, what is the reason for the 'PERF_LOAD_IMAGE_END' macro being
only added when PeiLoadImage() returns with no error?

Best Regards,
Hao Wu


>              //
>              // PEIM_STATE_REGISTER_FOR_SHADOW move to
> PEIM_STATE_DONE
>              //
> @@ -1118,6 +1122,7 @@ PeiDispatcher (
>                //
>                // For PEIM driver, Load its entry point
>                //
> +              PERF_LOAD_IMAGE_BEGIN (NULL);
>                Status = PeiLoadImage (
>                           PeiServices,
>                           PeimFileHandle,
> @@ -1126,6 +1131,7 @@ PeiDispatcher (
>                           &AuthenticationState
>                           );
>                if (Status == EFI_SUCCESS) {
> +                PERF_LOAD_IMAGE_END (PeimFileHandle);
>                  //
>                  // The PEIM has its dependencies satisfied, and its entry point
>                  // has been found, so invoke it.
> @@ -1197,6 +1203,7 @@ PeiDispatcher (
>                  //
>                  // Load PEIM into Memory for Register for shadow PEIM.
>                  //
> +                PERF_LOAD_IMAGE_BEGIN (NULL);
>                  Status = PeiLoadImage (
>                             PeiServices,
>                             PeimFileHandle,
> @@ -1205,6 +1212,7 @@ PeiDispatcher (
>                             &AuthenticationState
>                             );
>                  if (Status == EFI_SUCCESS) {
> +                  PERF_LOAD_IMAGE_END (PeimFileHandle);
>                    PeimEntryPoint = (EFI_PEIM_ENTRY_POINT2)(UINTN)EntryPoint;
>                  }
>                }
> @@ -1252,6 +1260,8 @@ PeiDispatcher (
>      //
>    } while (Private->PeimNeedingDispatch && Private-
> >PeimDispatchOnThisPass);
> 
> +  PERF_FUNCTION_END ();
> +
>  }
> 
>  /**
> --
> 2.21.0.windows.1

[PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change performance code

[Gao, Zhichao]

From: Bret Barkelew <Bret.Barkelew@microsoft.com>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888

Use PERF_INMODULE_BEGIN and PERF_INMODULE_END to replace PERF_START_EX,
PERF_CODE and PERF_END_EX.
Use PERF_CROSSMODULE_END and PERF_CROSSMODULE_BEGIN to get the info
of one boot image's performance.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
index 952033fc82..af1024cacd 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
@@ -1812,7 +1812,7 @@ EfiBootManagerBoot (
     BmRepairAllControllers (0);
   }
 
-  PERF_START_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber);
+  PERF_INMODULE_BEGIN ("BdsAttempt");
 
   //
   // 5. Adjust the different type memory page number just before booting
@@ -1932,9 +1932,9 @@ EfiBootManagerBoot (
   //
   // Write boot to OS performance data for UEFI boot
   //
-  PERF_CODE (
-    BmEndOfBdsPerfCode (NULL, NULL);
-  );
+  PERF_INMODULE_END ("BdsAttempt");
+  PERF_CROSSMODULE_END ("BDS");
+  PERF_CROSSMODULE_BEGIN ("BDS");
 
   REPORT_STATUS_CODE (EFI_PROGRESS_CODE, PcdGet32 (PcdProgressCodeOsLoaderStart));
 
@@ -1947,7 +1947,6 @@ EfiBootManagerBoot (
     //
     BmReportLoadFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED, Status);
   }
-  PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber);
 
   //
   // Destroy the RAM disk
-- 
2.21.0.windows.1

Re: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change performance code

[Wu, Hao A]

> -----Original Message-----
> From: Gao, Zhichao
> Sent: Monday, June 10, 2019 3:29 PM
> To: devel@edk2.groups.io
> Cc: Bret Barkelew; Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Gao, Liming
> Subject: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change
> performance code
> 
> From: Bret Barkelew <Bret.Barkelew@microsoft.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888
> 
> Use PERF_INMODULE_BEGIN and PERF_INMODULE_END to replace
> PERF_START_EX,
> PERF_CODE and PERF_END_EX.


Hello Dandan & Liming,

May I know the reason for 'PERF_START_EX' & 'PERF_END_EX' macros are not
being replaced in commit:

Revision: 67e9ab84ef042bd59c4297fdad7f6aece6b7bbca
MdeModulePkg: Use new added Perf macros

Is there a special reason for this?
('OptionNumber' as the identifier?)


> Use PERF_CROSSMODULE_END and PERF_CROSSMODULE_BEGIN to get the
> info
> of one boot image's performance.


Hello Zhichao,

May I know what kind of test has been done for this patch?
Also, some inline comments below:


> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> index 952033fc82..af1024cacd 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> @@ -1812,7 +1812,7 @@ EfiBootManagerBoot (
>      BmRepairAllControllers (0);
>    }
> 
> -  PERF_START_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32)
> OptionNumber);
> +  PERF_INMODULE_BEGIN ("BdsAttempt");
> 
>    //
>    // 5. Adjust the different type memory page number just before booting
> @@ -1932,9 +1932,9 @@ EfiBootManagerBoot (
>    //
>    // Write boot to OS performance data for UEFI boot
>    //
> -  PERF_CODE (
> -    BmEndOfBdsPerfCode (NULL, NULL);
> -  );
> +  PERF_INMODULE_END ("BdsAttempt");


I think the patch missed to replace the below 'PERF_END_EX' macro:

  //
  if ((DevicePathType (BootOption->FilePath) == BBS_DEVICE_PATH) && (DevicePathSubType (BootOption->FilePath) == BBS_BBS_DP)) {
    ...

    PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber);
    ^^^^^^^^^^^
    return;
  }


> +  PERF_CROSSMODULE_END ("BDS");
> +  PERF_CROSSMODULE_BEGIN ("BDS");


Could you help to introduce the purpose for the above
'PERF_CROSSMODULE_BEGIN' in more detail?


> 
>    REPORT_STATUS_CODE (EFI_PROGRESS_CODE, PcdGet32
> (PcdProgressCodeOsLoaderStart));
> 
> @@ -1947,7 +1947,6 @@ EfiBootManagerBoot (
>      //
>      BmReportLoadFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED,
> Status);
>    }
> -  PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32)
> OptionNumber);


The patch excludes the time consumed by StartImage() from performance data
for the "BdsAttempt" token.

If the image starts successfully, there will not be an matching PERF_END
macro for the origin code.

Ray, do you think it is a reasonable change here?

Best Regards,
Hao Wu


> 
>    //
>    // Destroy the RAM disk
> --
> 2.21.0.windows.1

Re: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change performance code

[Dandan Bi]

> -----Original Message-----
> From: Wu, Hao A
> Sent: Friday, June 14, 2019 4:44 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io; Bi,
> Dandan <dandan.bi@intel.com>; Gao, Liming <liming.gao@intel.com>; Ni,
> Ray <ray.ni@intel.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change
> performance code
> 
> > -----Original Message-----
> > From: Gao, Zhichao
> > Sent: Monday, June 10, 2019 3:29 PM
> > To: devel@edk2.groups.io
> > Cc: Bret Barkelew; Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Gao,
> > Liming
> > Subject: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change
> > performance code
> >
> > From: Bret Barkelew <Bret.Barkelew@microsoft.com>
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888
> >
> > Use PERF_INMODULE_BEGIN and PERF_INMODULE_END to replace
> > PERF_START_EX, PERF_CODE and PERF_END_EX.
> 
> 
> Hello Dandan & Liming,
> 
> May I know the reason for 'PERF_START_EX' & 'PERF_END_EX' macros are
> not
> being replaced in commit:
> 
> Revision: 67e9ab84ef042bd59c4297fdad7f6aece6b7bbca
> MdeModulePkg: Use new added Perf macros
> 
> Is there a special reason for this?
> ('OptionNumber' as the identifier?)

Hi Hao and Zhichao

The 'PERF_START_EX' & 'PERF_END_EX'  here specifies 'OptionNumber' as the identifier. We will miss the information of 'OptionNumber' if we replace it with new macros. It may impact current usage model. That's why we kept it before.
For other 'PERF_START_EX' & 'PERF_END_EX'  replacements in this patch series may have the similar issue.

Zhichao, please hold the patches that replace 'PERF_START_EX' & 'PERF_END_EX' firstly, I think it may need more discussion.

Liming, do you have any comments for this?


Thanks,
Dandan
> 
> 
> > Use PERF_CROSSMODULE_END and PERF_CROSSMODULE_BEGIN to get
> the
> > info
> > of one boot image's performance.
> 
> 
> Hello Zhichao,
> 
> May I know what kind of test has been done for this patch?
> Also, some inline comments below:
> 
> 
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > ---
> >  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > index 952033fc82..af1024cacd 100644
> > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > @@ -1812,7 +1812,7 @@ EfiBootManagerBoot (
> >      BmRepairAllControllers (0);
> >    }
> >
> > -  PERF_START_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32)
> > OptionNumber);
> > +  PERF_INMODULE_BEGIN ("BdsAttempt");
> >
> >    //
> >    // 5. Adjust the different type memory page number just before booting
> > @@ -1932,9 +1932,9 @@ EfiBootManagerBoot (
> >    //
> >    // Write boot to OS performance data for UEFI boot
> >    //
> > -  PERF_CODE (
> > -    BmEndOfBdsPerfCode (NULL, NULL);
> > -  );
> > +  PERF_INMODULE_END ("BdsAttempt");
> 
> 
> I think the patch missed to replace the below 'PERF_END_EX' macro:
> 
>   //
>   if ((DevicePathType (BootOption->FilePath) == BBS_DEVICE_PATH) &&
> (DevicePathSubType (BootOption->FilePath) == BBS_BBS_DP)) {
>     ...
> 
>     PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32)
> OptionNumber);
>     ^^^^^^^^^^^
>     return;
>   }
> 
> 
> > +  PERF_CROSSMODULE_END ("BDS");
> > +  PERF_CROSSMODULE_BEGIN ("BDS");
> 
> 
> Could you help to introduce the purpose for the above
> 'PERF_CROSSMODULE_BEGIN' in more detail?
> 
> 
> >
> >    REPORT_STATUS_CODE (EFI_PROGRESS_CODE, PcdGet32
> > (PcdProgressCodeOsLoaderStart));
> >
> > @@ -1947,7 +1947,6 @@ EfiBootManagerBoot (
> >      //
> >      BmReportLoadFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED,
> > Status);
> >    }
> > -  PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32)
> > OptionNumber);
> 
> 
> The patch excludes the time consumed by StartImage() from performance
> data
> for the "BdsAttempt" token.
> 
> If the image starts successfully, there will not be an matching PERF_END
> macro for the origin code.
> 
> Ray, do you think it is a reasonable change here?
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> >    //
> >    // Destroy the RAM disk
> > --
> > 2.21.0.windows.1

Re: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change performance code

[Liming Gao]

>-----Original Message-----
>From: Bi, Dandan
>Sent: Monday, June 17, 2019 12:47 PM
>To: Wu, Hao A <hao.a.wu@intel.com>; Gao, Zhichao
><zhichao.gao@intel.com>; devel@edk2.groups.io; Gao, Liming
><liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>
>Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J
><jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>
>Subject: RE: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change
>performance code
>
>> -----Original Message-----
>> From: Wu, Hao A
>> Sent: Friday, June 14, 2019 4:44 PM
>> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io; Bi,
>> Dandan <dandan.bi@intel.com>; Gao, Liming <liming.gao@intel.com>; Ni,
>> Ray <ray.ni@intel.com>
>> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J
>> <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: RE: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change
>> performance code
>>
>> > -----Original Message-----
>> > From: Gao, Zhichao
>> > Sent: Monday, June 10, 2019 3:29 PM
>> > To: devel@edk2.groups.io
>> > Cc: Bret Barkelew; Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Gao,
>> > Liming
>> > Subject: [PATCH 3/6] MdeModulePkg/UefiBootManagerLib: Change
>> > performance code
>> >
>> > From: Bret Barkelew <Bret.Barkelew@microsoft.com>
>> >
>> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888
>> >
>> > Use PERF_INMODULE_BEGIN and PERF_INMODULE_END to replace
>> > PERF_START_EX, PERF_CODE and PERF_END_EX.
>>
>>
>> Hello Dandan & Liming,
>>
>> May I know the reason for 'PERF_START_EX' & 'PERF_END_EX' macros are
>> not
>> being replaced in commit:
>>
>> Revision: 67e9ab84ef042bd59c4297fdad7f6aece6b7bbca
>> MdeModulePkg: Use new added Perf macros
>>
>> Is there a special reason for this?
>> ('OptionNumber' as the identifier?)
>
>Hi Hao and Zhichao
>
>The 'PERF_START_EX' & 'PERF_END_EX'  here specifies 'OptionNumber' as the
>identifier. We will miss the information of 'OptionNumber' if we replace it with
>new macros. It may impact current usage model. That's why we kept it before.
>For other 'PERF_START_EX' & 'PERF_END_EX'  replacements in this patch
>series may have the similar issue.
>
>Zhichao, please hold the patches that replace 'PERF_START_EX' &
>'PERF_END_EX' firstly, I think it may need more discussion.
>
>Liming, do you have any comments for this?
>

Yes. I don't think the replacement is good. I suggest to keep current, and add new ones. I add my comments below. 

>
>Thanks,
>Dandan
>>
>>
>> > Use PERF_CROSSMODULE_END and PERF_CROSSMODULE_BEGIN to get
>> the
>> > info
>> > of one boot image's performance.
>>
>>
>> Hello Zhichao,
>>
>> May I know what kind of test has been done for this patch?
>> Also, some inline comments below:
>>
>>
>> >
>> > Cc: Jian J Wang <jian.j.wang@intel.com>
>> > Cc: Hao A Wu <hao.a.wu@intel.com>
>> > Cc: Ray Ni <ray.ni@intel.com>
>> > Cc: Star Zeng <star.zeng@intel.com>
>> > Cc: Liming Gao <liming.gao@intel.com>
>> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
>> > ---
>> >  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 9 ++++-----
>> >  1 file changed, 4 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> > b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> > index 952033fc82..af1024cacd 100644
>> > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>> > @@ -1812,7 +1812,7 @@ EfiBootManagerBoot (
>> >      BmRepairAllControllers (0);
>> >    }
>> >
>> > -  PERF_START_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32)
>> > OptionNumber);
>> > +  PERF_INMODULE_BEGIN ("BdsAttempt");
>> >
>> >    //
>> >    // 5. Adjust the different type memory page number just before booting
>> > @@ -1932,9 +1932,9 @@ EfiBootManagerBoot (
>> >    //
>> >    // Write boot to OS performance data for UEFI boot
>> >    //
>> > -  PERF_CODE (
>> > -    BmEndOfBdsPerfCode (NULL, NULL);
>> > -  );

Seemly, this change is not related to this patch. This code is unused any more. I agree to remove it in another separate patch. 

Thanks
Liming
>> > +  PERF_INMODULE_END ("BdsAttempt");
>>
>>
>> I think the patch missed to replace the below 'PERF_END_EX' macro:
>>
>>   //
>>   if ((DevicePathType (BootOption->FilePath) == BBS_DEVICE_PATH) &&
>> (DevicePathSubType (BootOption->FilePath) == BBS_BBS_DP)) {
>>     ...
>>
>>     PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32)
>> OptionNumber);
>>     ^^^^^^^^^^^
>>     return;
>>   }
>>
>>
>> > +  PERF_CROSSMODULE_END ("BDS");
>> > +  PERF_CROSSMODULE_BEGIN ("BDS");
>>
>>
>> Could you help to introduce the purpose for the above
>> 'PERF_CROSSMODULE_BEGIN' in more detail?
>>
>>
>> >
>> >    REPORT_STATUS_CODE (EFI_PROGRESS_CODE, PcdGet32
>> > (PcdProgressCodeOsLoaderStart));
>> >
>> > @@ -1947,7 +1947,6 @@ EfiBootManagerBoot (
>> >      //
>> >      BmReportLoadFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED,
>> > Status);
>> >    }
>> > -  PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32)
>> > OptionNumber);
>>
>>
>> The patch excludes the time consumed by StartImage() from performance
>> data
>> for the "BdsAttempt" token.
>>
>> If the image starts successfully, there will not be an matching PERF_END
>> macro for the origin code.
>>
>> Ray, do you think it is a reasonable change here?
>>
>> Best Regards,
>> Hao Wu
>>
>>
>> >
>> >    //
>> >    // Destroy the RAM disk
>> > --
>> > 2.21.0.windows.1

[PATCH 4/6] SecurityPkg/Tcg2Dxe: Change performance code

[Gao, Zhichao]

From: Bret Barkelew <Bret.Barkelew@microsoft.com>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888

Use PERF_FUNCTION_BEGIN and PERF_FUNCTION_END to replace
PERF_START_EX and PERF_FUNCTION_END.

Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian Wang <jian.j.wang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c b/SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c
index a2729457b7..001e0aac4d 100644
--- a/SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c
+++ b/SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c
@@ -45,8 +45,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/ReportStatusCodeLib.h>
 #include <Library/Tcg2PhysicalPresenceLib.h>
 
-#define PERF_ID_TCG2_DXE  0x3120
-
 typedef struct {
   CHAR16                                 *VariableName;
   EFI_GUID                               *VendorGuid;
@@ -2250,7 +2248,7 @@ OnReadyToBoot (
   EFI_STATUS                        Status;
   TPM_PCRINDEX                      PcrIndex;
 
-  PERF_START_EX (mImageHandle, "EventRec", "Tcg2Dxe", 0, PERF_ID_TCG2_DXE);
+  PERF_FUNCTION_BEGIN ();
   if (mBootAttempts == 0) {
 
     //
@@ -2332,7 +2330,7 @@ OnReadyToBoot (
   // Increase boot attempt counter.
   //
   mBootAttempts++;
-  PERF_END_EX (mImageHandle, "EventRec", "Tcg2Dxe", 0, PERF_ID_TCG2_DXE + 1);
+  PERF_FUNCTION_END ();
 }
 
 /**
-- 
2.21.0.windows.1

[PATCH 5/6] SecurityPkg/Tcg2Pei: Change performance code

[Gao, Zhichao]

From: Bret Barkelew <Bret.Barkelew@microsoft.com>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888

Use PERF_FUNCTION_BEGIN and PERF_FUNCTION_END to replace
PERF_START_EX and PERF_FUNCTION_END.

Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian Wang <jian.j.wang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
index f28f33fdec..4b5388726c 100644
--- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
+++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
@@ -1,7 +1,7 @@
 /** @file
   Initialize TPM2 device and measure FVs before handing off control to DXE.
 
-Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
 Copyright (c) 2017, Microsoft Corporation.  All rights reserved. <BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -38,8 +38,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/ReportStatusCodeLib.h>
 #include <Library/ResetSystemLib.h>
 
-#define PERF_ID_TCG2_PEI  0x3080
-
 typedef struct {
   EFI_GUID                   *EventGuid;
   EFI_TCG2_EVENT_LOG_FORMAT  LogFormat;
@@ -651,7 +649,7 @@ MeasureMainBios (
   EFI_FV_INFO                       VolumeInfo;
   EFI_PEI_FIRMWARE_VOLUME_PPI       *FvPpi;
 
-  PERF_START_EX (mFileHandle, "EventRec", "Tcg2Pei", 0, PERF_ID_TCG2_PEI);
+  PERF_FUNCTION_BEGIN ();
 
   //
   // Only measure BFV at the very beginning. Other parts of Static Core Root of
@@ -682,7 +680,7 @@ MeasureMainBios (
 
   Status = MeasureFvImage ((EFI_PHYSICAL_ADDRESS) (UINTN) VolumeInfo.FvStart, VolumeInfo.FvSize);
 
-  PERF_END_EX (mFileHandle, "EventRec", "Tcg2Pei", 0, PERF_ID_TCG2_PEI + 1);
+  PERF_FUNCTION_END ();
 
   return Status;
 }
-- 
2.21.0.windows.1

[PATCH 6/6] IntelSiliconPkg/IntelVtdDxe: Change the performance code

[Gao, Zhichao]

From: Bret Barkelew <Bret.Barkelew@microsoft.com>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1888

Use PERF_INMODULE_BEGIN and PERF_INMODULE_END to replace
PERF_CODE.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 .../Feature/VTd/IntelVTdDxe/IntelVTdDxe.c           | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c
index a6287be2cf..501933da5c 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c
@@ -230,7 +230,6 @@ VTdSetAttribute (
   UINT16               Segment;
   VTD_SOURCE_ID        SourceId;
   CHAR8                PerfToken[sizeof("VTD(S0000.B00.D00.F00)")];
-  UINT32               Identifier;
 
   DumpVtdIfError ();
 
@@ -257,18 +256,12 @@ VTdSetAttribute (
     }
     Status = RequestAccessAttribute (Segment, SourceId, DeviceAddress, Length, IoMmuAccess);
   } else {
-    PERF_CODE (
-      AsciiSPrint (PerfToken, sizeof(PerfToken), "S%04xB%02xD%02xF%01x", Segment, SourceId.Bits.Bus, SourceId.Bits.Device, SourceId.Bits.Function);
-      Identifier = (Segment << 16) | SourceId.Uint16;
-      PERF_START_EX (gImageHandle, PerfToken, "IntelVTD", 0, Identifier);
-    );
+    AsciiSPrint (PerfToken, sizeof (PerfToken), "S%04xB%02xD%02xF%01x", Segment, SourceId.Bits.Bus, SourceId.Bits.Device, SourceId.Bits.Function);
+    PERF_INMODULE_BEGIN (PerfToken);
 
     Status = SetAccessAttribute (Segment, SourceId, DeviceAddress, Length, IoMmuAccess);
 
-    PERF_CODE (
-      Identifier = (Segment << 16) | SourceId.Uint16;
-      PERF_END_EX (gImageHandle, PerfToken, "IntelVTD", 0, Identifier);
-    );
+    PERF_INMODULE_END (PerfToken);
   }
 
   if (!EFI_ERROR(Status)) {
-- 
2.21.0.windows.1