[edk2-devel] [PATCH v2 0/6] Reduce one round BSP & AP sync

[edk2-devel] [PATCH v2 0/6] Reduce one round BSP & AP sync

[Wu, Jiaxin]

This version is updated according Ray's comments on
https://github.com/tianocore/edk2/pull/5175

Jiaxin Wu (6):
  SourceLevelDebugPkg/Library: Indicate SMM Debug Agent support or not
  MdeModulePkg/DebugAgentLibNull: Indicate SMM Debug Agent support or
    not
  UefiCpuPkg/PiSmmCpuDxeSmm: Check SMM Debug Agent support or not
  UefiCpuPkg/PiSmmCpuDxeSmm: Align BSP and AP sync logic for SMI exit
  UefiCpuPkg/PiSmmCpuDxeSmm: Invert ReleaseAllAPs & InitializeDebugAgent
  UefiCpuPkg/PiSmmCpuDxeSmm: Reduce one round BSP & AP sync

 .../Library/DebugAgentLibNull/DebugAgentLibNull.c  |  9 +++
 .../DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c    | 14 +++--
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                  |  4 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c              | 64 ++++++++++++----------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c         |  7 ++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  1 +
 6 files changed, 65 insertions(+), 34 deletions(-)

-- 
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112885): https://edk2.groups.io/g/devel/message/112885
Mute This Topic: https://groups.io/mt/103360801/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 1/6] SourceLevelDebugPkg/Library: Indicate SMM Debug Agent support or not

[Wu, Jiaxin]

This patch is to use the Context to indicate SMM Debug Agent support
or not if InitFlag is DEBUG_AGENT_INIT_SMM. Context must point to a
BOOLEAN if it's not NULL.

Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 .../Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c    | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c b/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c
index f49a592d27..1f34a0edc8 100644
--- a/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c
+++ b/SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgent/SmmDebugAgentLib.c
@@ -157,12 +157,13 @@ RestoreDebugRegister (
   in SMM code.
 
   If InitFlag is DEBUG_AGENT_INIT_SMM, it will override IDT table entries
   and initialize debug port. It will get debug agent Mailbox from GUIDed HOB,
   it it exists, debug agent wiil copied it into the local Mailbox in SMM space.
-  it will override IDT table entries and initialize debug port. Context will be
-  NULL.
+  it will override IDT table entries and initialize debug port. Context must
+  point to a BOOLEAN if it's not NULL, which indicates SMM Debug Agent supported
+  or not.
   If InitFlag is DEBUG_AGENT_INIT_ENTER_SMI, debug agent will save Debug
   Registers and get local Mailbox in SMM space. Context will be NULL.
   If InitFlag is DEBUG_AGENT_INIT_EXIT_SMI, debug agent will restore Debug
   Registers. Context will be NULL.
 
@@ -203,29 +204,32 @@ InitializeDebugAgent (
                         (VOID *)&mVectorHandoffInfoDebugAgent[0],
                         sizeof (EFI_VECTOR_HANDOFF_INFO) * mVectorHandoffInfoCount
                         );
       if (EFI_ERROR (Status)) {
         DEBUG ((DEBUG_ERROR, "DebugAgent: Cannot install configuration table for persisted vector handoff info!\n"));
+        *(BOOLEAN *)Context = FALSE;
         CpuDeadLoop ();
       }
 
       //
       // Check if Debug Agent initialized in DXE phase
       //
       Status = EfiGetSystemConfigurationTable (&gEfiDebugAgentGuid, (VOID **)&Mailbox);
       if ((Status == EFI_SUCCESS) && (Mailbox != NULL)) {
         VerifyMailboxChecksum (Mailbox);
-        mMailboxPointer = Mailbox;
+        mMailboxPointer     = Mailbox;
+        *(BOOLEAN *)Context = TRUE;
         break;
       }
 
       //
       // Check if Debug Agent initialized in SEC/PEI phase
       //
       Mailbox = GetMailboxFromHob ();
       if (Mailbox != NULL) {
-        mMailboxPointer = Mailbox;
+        mMailboxPointer     = Mailbox;
+        *(BOOLEAN *)Context = TRUE;
         break;
       }
 
       //
       // Debug Agent was not initialized before, use the local mailbox.
@@ -273,10 +277,12 @@ InitializeDebugAgent (
       //
       // Restore saved IDT entries
       //
       CopyMem ((VOID *)IdtDescriptor.Base, &IdtEntry, 33 * sizeof (IA32_IDT_GATE_DESCRIPTOR));
 
+      *(BOOLEAN *)Context = TRUE;
+
       break;
 
     case DEBUG_AGENT_INIT_ENTER_SMI:
       SaveDebugRegister ();
       if (!mSmmDebugIdtInitFlag) {
-- 
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112886): https://edk2.groups.io/g/devel/message/112886
Mute This Topic: https://groups.io/mt/103360802/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/6] SourceLevelDebugPkg/Library: Indicate SMM Debug Agent support or not

[Ni, Ray]

Jiaxin,
I missed one minor issue in offline review.
Check below.

Thanks,
Ray

> +  it will override IDT table entries and initialize debug port. Context must
> +  point to a BOOLEAN if it's not NULL, which indicates SMM Debug Agent
> supported

The comment says Context must point to a BOOLEAN "if it's not NULL".
So, it's ok to be NULL.
Then, the code below should modify the Context memory only when it's not NULL.

With this change added, Reviewed-by: Ray Ni <ray.ni@intel.com>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112899): https://edk2.groups.io/g/devel/message/112899
Mute This Topic: https://groups.io/mt/103360802/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 2/6] MdeModulePkg/DebugAgentLibNull: Indicate SMM Debug Agent support or not

[Wu, Jiaxin]

This patch is to use the Context to indicate SMM Debug Agent support or
not if InitFlag is DEBUG_AGENT_INIT_SMM. Context must point to a
BOOLEAN if it's not NULL.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.c b/MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.c
index 8e70705eb6..5596ee3bf9 100644
--- a/MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.c
+++ b/MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.c
@@ -22,10 +22,13 @@
   passing in the Context to be its parameter.
 
   If Function() is NULL, Debug Agent Library instance will return after setup debug
   environment.
 
+  If InitFlag is DEBUG_AGENT_INIT_SMM, Context must point to a BOOLEAN if it's not
+  NULL, which indicates SMM Debug Agent supported or not.
+
   @param[in] InitFlag     Init flag is used to decide the initialize process.
   @param[in] Context      Context needed according to InitFlag; it was optional.
   @param[in] Function     Continue function called by debug agent library; it was
                           optional.
 
@@ -36,10 +39,16 @@ InitializeDebugAgent (
   IN UINT32                InitFlag,
   IN VOID                  *Context  OPTIONAL,
   IN DEBUG_AGENT_CONTINUE  Function  OPTIONAL
   )
 {
+  switch (InitFlag) {
+    case DEBUG_AGENT_INIT_SMM:
+      *(BOOLEAN *)Context = FALSE;
+      return;
+  }
+
   if (Function != NULL) {
     Function (Context);
   }
 }
 
-- 
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112887): https://edk2.groups.io/g/devel/message/112887
Mute This Topic: https://groups.io/mt/103360803/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/6] MdeModulePkg/DebugAgentLibNull: Indicate SMM Debug Agent support or not

[Ni, Ray]

Similar comments as #1.
Context could be NULL. Please add check before the assignment.
With that, Reviewed-by: Ray Ni <ray.ni@intel.com>

Thanks,
Ray
> -----Original Message-----
> From: Wu, Jiaxin <jiaxin.wu@intel.com>
> Sent: Tuesday, December 26, 2023 12:21 AM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>
> Subject: [PATCH v2 2/6] MdeModulePkg/DebugAgentLibNull: Indicate SMM
> Debug Agent support or not
> 
> This patch is to use the Context to indicate SMM Debug Agent support or
> not if InitFlag is DEBUG_AGENT_INIT_SMM. Context must point to a
> BOOLEAN if it's not NULL.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
>  MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.c | 9
> +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.c
> b/MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.c
> index 8e70705eb6..5596ee3bf9 100644
> --- a/MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.c
> +++ b/MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.c
> @@ -22,10 +22,13 @@
>    passing in the Context to be its parameter.
> 
>    If Function() is NULL, Debug Agent Library instance will return after setup
> debug
>    environment.
> 
> +  If InitFlag is DEBUG_AGENT_INIT_SMM, Context must point to a BOOLEAN
> if it's not
> +  NULL, which indicates SMM Debug Agent supported or not.
> +
>    @param[in] InitFlag     Init flag is used to decide the initialize process.
>    @param[in] Context      Context needed according to InitFlag; it was optional.
>    @param[in] Function     Continue function called by debug agent library; it
> was
>                            optional.
> 
> @@ -36,10 +39,16 @@ InitializeDebugAgent (
>    IN UINT32                InitFlag,
>    IN VOID                  *Context  OPTIONAL,
>    IN DEBUG_AGENT_CONTINUE  Function  OPTIONAL
>    )
>  {
> +  switch (InitFlag) {
> +    case DEBUG_AGENT_INIT_SMM:
> +      *(BOOLEAN *)Context = FALSE;
> +      return;
> +  }
> +
>    if (Function != NULL) {
>      Function (Context);
>    }
>  }
> 
> --
> 2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112900): https://edk2.groups.io/g/devel/message/112900
Mute This Topic: https://groups.io/mt/103360803/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 3/6] UefiCpuPkg/PiSmmCpuDxeSmm: Check SMM Debug Agent support or not

[Wu, Jiaxin]

This patch is to check SMM Debug Agent support or not before
InitializeDebugAgent.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c          |  4 +++-
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c      | 22 +++++++++++++---------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c |  7 ++++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |  1 +
 4 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index 0bae0e33f1..b14c289a27 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -835,11 +835,13 @@ SmmRestoreCpu (
     ASSERT_EFI_ERROR (Status);
 
     //
     // Initialize Debug Agent to support source level debug
     //
-    InitializeDebugAgent (DEBUG_AGENT_INIT_THUNK_PEI_IA32TOX64, (VOID *)&Ia32Idtr, NULL);
+    if (mSmmDebugAgentSupport) {
+      InitializeDebugAgent (DEBUG_AGENT_INIT_THUNK_PEI_IA32TOX64, (VOID *)&Ia32Idtr, NULL);
+    }
   }
 
   mBspApicId = GetApicId ();
   //
   // Skip AP initialization if mAcpiCpuData is not valid
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 4fbb0bba87..324e85d6b5 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -474,14 +474,16 @@ BSPHandler (
   //
   // Flag BSP's presence
   //
   *mSmmMpSyncData->InsideSmm = TRUE;
 
-  //
-  // Initialize Debug Agent to start source level debug in BSP handler
-  //
-  InitializeDebugAgent (DEBUG_AGENT_INIT_ENTER_SMI, NULL, NULL);
+  if (mSmmDebugAgentSupport) {
+    //
+    // Initialize Debug Agent to start source level debug in BSP handler
+    //
+    InitializeDebugAgent (DEBUG_AGENT_INIT_ENTER_SMI, NULL, NULL);
+  }
 
   //
   // Mark this processor's presence
   //
   *(mSmmMpSyncData->CpuData[CpuIndex].Present) = TRUE;
@@ -646,15 +648,17 @@ BSPHandler (
     // Wait for all APs to complete MTRR programming
     //
     SmmCpuSyncWaitForAPs (mSmmMpSyncData->SyncContext, ApCount, CpuIndex);
   }
 
-  //
-  // Stop source level debug in BSP handler, the code below will not be
-  // debugged.
-  //
-  InitializeDebugAgent (DEBUG_AGENT_INIT_EXIT_SMI, NULL, NULL);
+  if (mSmmDebugAgentSupport) {
+    //
+    // Stop source level debug in BSP handler, the code below will not be
+    // debugged.
+    //
+    InitializeDebugAgent (DEBUG_AGENT_INIT_EXIT_SMI, NULL, NULL);
+  }
 
   //
   // Signal APs to Reset states/semaphore for this processor
   //
   ReleaseAllAPs ();
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index 209a2e4810..9b230772cb 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -110,10 +110,15 @@ BOOLEAN  mSmmReadyToLock = FALSE;
 //
 // Global used to cache PCD for SMM Code Access Check enable
 //
 BOOLEAN  mSmmCodeAccessCheckEnable = FALSE;
 
+//
+// Global used to cache SMM Debug Agent Supported ot not
+//
+BOOLEAN  mSmmDebugAgentSupport = FALSE;
+
 //
 // Global copy of the PcdPteMemoryEncryptionAddressOrMask
 //
 UINT64  mAddressEncMask = 0;
 
@@ -895,11 +900,11 @@ PiCpuSmmEntry (
   PiSmmCpuSmiEntryFixupAddress ();
 
   //
   // Initialize Debug Agent to support source level debug in SMM code
   //
-  InitializeDebugAgent (DEBUG_AGENT_INIT_SMM, NULL, NULL);
+  InitializeDebugAgent (DEBUG_AGENT_INIT_SMM, &mSmmDebugAgentSupport, NULL);
 
   //
   // Report the start of CPU SMM initialization.
   //
   REPORT_STATUS_CODE (
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index a2fa4f6734..7f244ea803 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -481,10 +481,11 @@ extern UINTN                         mSemaphoreSize;
 extern SPIN_LOCK                     *mPFLock;
 extern SPIN_LOCK                     *mConfigSmmCodeAccessCheckLock;
 extern EFI_SMRAM_DESCRIPTOR          *mSmmCpuSmramRanges;
 extern UINTN                         mSmmCpuSmramRangeCount;
 extern UINT8                         mPhysicalAddressBits;
+extern BOOLEAN                       mSmmDebugAgentSupport;
 
 //
 // Copy of the PcdPteMemoryEncryptionAddressOrMask
 //
 extern UINT64  mAddressEncMask;
-- 
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112888): https://edk2.groups.io/g/devel/message/112888
Mute This Topic: https://groups.io/mt/103360804/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 3/6] UefiCpuPkg/PiSmmCpuDxeSmm: Check SMM Debug Agent support or not

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@Intel.com>

Thanks,
Ray
> -----Original Message-----
> From: Wu, Jiaxin <jiaxin.wu@intel.com>
> Sent: Tuesday, December 26, 2023 12:21 AM
> To: devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: [PATCH v2 3/6] UefiCpuPkg/PiSmmCpuDxeSmm: Check SMM Debug
> Agent support or not
> 
> This patch is to check SMM Debug Agent support or not before
> InitializeDebugAgent.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zeng Star <star.zeng@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c          |  4 +++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c      | 22 +++++++++++++--------
> -
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c |  7 ++++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |  1 +
>  4 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index 0bae0e33f1..b14c289a27 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -835,11 +835,13 @@ SmmRestoreCpu (
>      ASSERT_EFI_ERROR (Status);
> 
>      //
>      // Initialize Debug Agent to support source level debug
>      //
> -    InitializeDebugAgent (DEBUG_AGENT_INIT_THUNK_PEI_IA32TOX64, (VOID
> *)&Ia32Idtr, NULL);
> +    if (mSmmDebugAgentSupport) {
> +      InitializeDebugAgent (DEBUG_AGENT_INIT_THUNK_PEI_IA32TOX64,
> (VOID *)&Ia32Idtr, NULL);
> +    }
>    }
> 
>    mBspApicId = GetApicId ();
>    //
>    // Skip AP initialization if mAcpiCpuData is not valid
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 4fbb0bba87..324e85d6b5 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -474,14 +474,16 @@ BSPHandler (
>    //
>    // Flag BSP's presence
>    //
>    *mSmmMpSyncData->InsideSmm = TRUE;
> 
> -  //
> -  // Initialize Debug Agent to start source level debug in BSP handler
> -  //
> -  InitializeDebugAgent (DEBUG_AGENT_INIT_ENTER_SMI, NULL, NULL);
> +  if (mSmmDebugAgentSupport) {
> +    //
> +    // Initialize Debug Agent to start source level debug in BSP handler
> +    //
> +    InitializeDebugAgent (DEBUG_AGENT_INIT_ENTER_SMI, NULL, NULL);
> +  }
> 
>    //
>    // Mark this processor's presence
>    //
>    *(mSmmMpSyncData->CpuData[CpuIndex].Present) = TRUE;
> @@ -646,15 +648,17 @@ BSPHandler (
>      // Wait for all APs to complete MTRR programming
>      //
>      SmmCpuSyncWaitForAPs (mSmmMpSyncData->SyncContext, ApCount,
> CpuIndex);
>    }
> 
> -  //
> -  // Stop source level debug in BSP handler, the code below will not be
> -  // debugged.
> -  //
> -  InitializeDebugAgent (DEBUG_AGENT_INIT_EXIT_SMI, NULL, NULL);
> +  if (mSmmDebugAgentSupport) {
> +    //
> +    // Stop source level debug in BSP handler, the code below will not be
> +    // debugged.
> +    //
> +    InitializeDebugAgent (DEBUG_AGENT_INIT_EXIT_SMI, NULL, NULL);
> +  }
> 
>    //
>    // Signal APs to Reset states/semaphore for this processor
>    //
>    ReleaseAllAPs ();
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index 209a2e4810..9b230772cb 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -110,10 +110,15 @@ BOOLEAN  mSmmReadyToLock = FALSE;
>  //
>  // Global used to cache PCD for SMM Code Access Check enable
>  //
>  BOOLEAN  mSmmCodeAccessCheckEnable = FALSE;
> 
> +//
> +// Global used to cache SMM Debug Agent Supported ot not
> +//
> +BOOLEAN  mSmmDebugAgentSupport = FALSE;
> +
>  //
>  // Global copy of the PcdPteMemoryEncryptionAddressOrMask
>  //
>  UINT64  mAddressEncMask = 0;
> 
> @@ -895,11 +900,11 @@ PiCpuSmmEntry (
>    PiSmmCpuSmiEntryFixupAddress ();
> 
>    //
>    // Initialize Debug Agent to support source level debug in SMM code
>    //
> -  InitializeDebugAgent (DEBUG_AGENT_INIT_SMM, NULL, NULL);
> +  InitializeDebugAgent (DEBUG_AGENT_INIT_SMM,
> &mSmmDebugAgentSupport, NULL);
> 
>    //
>    // Report the start of CPU SMM initialization.
>    //
>    REPORT_STATUS_CODE (
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index a2fa4f6734..7f244ea803 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -481,10 +481,11 @@ extern UINTN                         mSemaphoreSize;
>  extern SPIN_LOCK                     *mPFLock;
>  extern SPIN_LOCK                     *mConfigSmmCodeAccessCheckLock;
>  extern EFI_SMRAM_DESCRIPTOR          *mSmmCpuSmramRanges;
>  extern UINTN                         mSmmCpuSmramRangeCount;
>  extern UINT8                         mPhysicalAddressBits;
> +extern BOOLEAN                       mSmmDebugAgentSupport;
> 
>  //
>  // Copy of the PcdPteMemoryEncryptionAddressOrMask
>  //
>  extern UINT64  mAddressEncMask;
> --
> 2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112901): https://edk2.groups.io/g/devel/message/112901
Mute This Topic: https://groups.io/mt/103360804/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Align BSP and AP sync logic for SMI exit

[Wu, Jiaxin]

Below piece of code is the BSP and AP sync logic for SMI exit.
1. AP after finish the scheduled procedure:
  if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
    SmmCpuSyncReleaseBsp ();
    SmmCpuSyncWaitForBsp ();
    ...
  }
  SmmCpuSyncReleaseBsp ();
  SmmCpuSyncWaitForBsp ();
  SmmCpuSyncReleaseBsp ();

2. BSP after return from SmmCoreEntry:
  SmmCpuSyncWaitForAPs ();
  if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
    ReleaseAllAPs ();
    ...
    SmmCpuSyncWaitForAPs ();
  }
  ReleaseAllAPs ();
  SmmCpuSyncWaitForAPs();

This patch is to make BSP same as AP sync logic:
  if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
    SmmCpuSyncWaitForAPs ();
    ReleaseAllAPs ();
    ...
  }
  SmmCpuSyncWaitForAPs ();
  ReleaseAllAPs ();
  SmmCpuSyncWaitForAPs();

With the change, it will be easy to understand the sync flow as
below:
BSP: SmmCpuSyncWaitForAPs  <--  AP: SmmCpuSyncReleaseBsp
BSP: ReleaseAllAPs         -->  AP: SmmCpuSyncWaitForBsp

This patch doesn't have function impact.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 324e85d6b5..bd2c9f841b 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -625,33 +625,33 @@ BSPHandler (
   // Notify all APs to exit
   //
   *mSmmMpSyncData->InsideSmm = FALSE;
   ReleaseAllAPs ();
 
-  //
-  // Wait for all APs to complete their pending tasks
-  //
-  SmmCpuSyncWaitForAPs (mSmmMpSyncData->SyncContext, ApCount, CpuIndex);
-
   if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
+    //
+    // Wait for all APs the readiness to program MTRRs
+    //
+    SmmCpuSyncWaitForAPs (mSmmMpSyncData->SyncContext, ApCount, CpuIndex);
+
     //
     // Signal APs to restore MTRRs
     //
     ReleaseAllAPs ();
 
     //
     // Restore OS MTRRs
     //
     SmmCpuFeaturesReenableSmrr ();
     MtrrSetAllMtrrs (&Mtrrs);
-
-    //
-    // Wait for all APs to complete MTRR programming
-    //
-    SmmCpuSyncWaitForAPs (mSmmMpSyncData->SyncContext, ApCount, CpuIndex);
   }
 
+  //
+  // Wait for all APs to complete their pending tasks including MTRR programming if needed.
+  //
+  SmmCpuSyncWaitForAPs (mSmmMpSyncData->SyncContext, ApCount, CpuIndex);
+
   if (mSmmDebugAgentSupport) {
     //
     // Stop source level debug in BSP handler, the code below will not be
     // debugged.
     //
-- 
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112889): https://edk2.groups.io/g/devel/message/112889
Mute This Topic: https://groups.io/mt/103360805/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Align BSP and AP sync logic for SMI exit

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@Intel.com>

Thanks,
Ray
> -----Original Message-----
> From: Wu, Jiaxin <jiaxin.wu@intel.com>
> Sent: Tuesday, December 26, 2023 12:21 AM
> To: devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: [PATCH v2 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Align BSP and AP
> sync logic for SMI exit
> 
> Below piece of code is the BSP and AP sync logic for SMI exit.
> 1. AP after finish the scheduled procedure:
>   if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
>     SmmCpuSyncReleaseBsp ();
>     SmmCpuSyncWaitForBsp ();
>     ...
>   }
>   SmmCpuSyncReleaseBsp ();
>   SmmCpuSyncWaitForBsp ();
>   SmmCpuSyncReleaseBsp ();
> 
> 2. BSP after return from SmmCoreEntry:
>   SmmCpuSyncWaitForAPs ();
>   if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
>     ReleaseAllAPs ();
>     ...
>     SmmCpuSyncWaitForAPs ();
>   }
>   ReleaseAllAPs ();
>   SmmCpuSyncWaitForAPs();
> 
> This patch is to make BSP same as AP sync logic:
>   if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
>     SmmCpuSyncWaitForAPs ();
>     ReleaseAllAPs ();
>     ...
>   }
>   SmmCpuSyncWaitForAPs ();
>   ReleaseAllAPs ();
>   SmmCpuSyncWaitForAPs();
> 
> With the change, it will be easy to understand the sync flow as
> below:
> BSP: SmmCpuSyncWaitForAPs  <--  AP: SmmCpuSyncReleaseBsp
> BSP: ReleaseAllAPs         -->  AP: SmmCpuSyncWaitForBsp
> 
> This patch doesn't have function impact.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zeng Star <star.zeng@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 324e85d6b5..bd2c9f841b 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -625,33 +625,33 @@ BSPHandler (
>    // Notify all APs to exit
>    //
>    *mSmmMpSyncData->InsideSmm = FALSE;
>    ReleaseAllAPs ();
> 
> -  //
> -  // Wait for all APs to complete their pending tasks
> -  //
> -  SmmCpuSyncWaitForAPs (mSmmMpSyncData->SyncContext, ApCount,
> CpuIndex);
> -
>    if (SmmCpuFeaturesNeedConfigureMtrrs ()) {
> +    //
> +    // Wait for all APs the readiness to program MTRRs
> +    //
> +    SmmCpuSyncWaitForAPs (mSmmMpSyncData->SyncContext, ApCount,
> CpuIndex);
> +
>      //
>      // Signal APs to restore MTRRs
>      //
>      ReleaseAllAPs ();
> 
>      //
>      // Restore OS MTRRs
>      //
>      SmmCpuFeaturesReenableSmrr ();
>      MtrrSetAllMtrrs (&Mtrrs);
> -
> -    //
> -    // Wait for all APs to complete MTRR programming
> -    //
> -    SmmCpuSyncWaitForAPs (mSmmMpSyncData->SyncContext, ApCount,
> CpuIndex);
>    }
> 
> +  //
> +  // Wait for all APs to complete their pending tasks including MTRR
> programming if needed.
> +  //
> +  SmmCpuSyncWaitForAPs (mSmmMpSyncData->SyncContext, ApCount,
> CpuIndex);
> +
>    if (mSmmDebugAgentSupport) {
>      //
>      // Stop source level debug in BSP handler, the code below will not be
>      // debugged.
>      //
> --
> 2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112902): https://edk2.groups.io/g/devel/message/112902
Mute This Topic: https://groups.io/mt/103360805/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 5/6] UefiCpuPkg/PiSmmCpuDxeSmm: Invert ReleaseAllAPs & InitializeDebugAgent

[Wu, Jiaxin]

Existing BSP handler stops source level debug, then call ReleaseAllAPs
to tell all APs can reset the Present flag to FALSE:
  InitializeDebugAgent (); /// Stop source level debug
  ReleaseAllAPs ();        /// Tell APs can reset "Present" flag.

This patch is to invert ReleaseAllAPs & InitializeDebugAgent:
  ReleaseAllAPs ();        /// Tell APs can reset "Present" flag.
  InitializeDebugAgent (); /// Stop source level debug

After this change, there is no negative impact since SMM source level
debug feature doesn't depend on AP's "Present" flag, no impact to the
SMM source level debug capability.

Instead, the change will benefit the AP source level debug capability
to trace its "Present" flag change for SMI exit since the source
level debug feature will be stopped after each AP has the chance to
reset the state.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index bd2c9f841b..9aa9908863 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -648,23 +648,23 @@ BSPHandler (
   //
   // Wait for all APs to complete their pending tasks including MTRR programming if needed.
   //
   SmmCpuSyncWaitForAPs (mSmmMpSyncData->SyncContext, ApCount, CpuIndex);
 
+  //
+  // Signal APs to Reset states/semaphore for this processor
+  //
+  ReleaseAllAPs ();
+
   if (mSmmDebugAgentSupport) {
     //
     // Stop source level debug in BSP handler, the code below will not be
     // debugged.
     //
     InitializeDebugAgent (DEBUG_AGENT_INIT_EXIT_SMI, NULL, NULL);
   }
 
-  //
-  // Signal APs to Reset states/semaphore for this processor
-  //
-  ReleaseAllAPs ();
-
   //
   // Perform pending operations for hot-plug
   //
   SmmCpuUpdate ();
 
-- 
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112890): https://edk2.groups.io/g/devel/message/112890
Mute This Topic: https://groups.io/mt/103360806/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 5/6] UefiCpuPkg/PiSmmCpuDxeSmm: Invert ReleaseAllAPs & InitializeDebugAgent

[Ni, Ray]

Source level debugging in SMM doesn't enable timer interrupt so it does not support break-in from HOST debugger.
It only supports debugging AP code which is stopped by breakpoints.
I agree that with this change, the debug window of APs is a bit increased.

Reviewed-by: Ray Ni <ray.ni@intel.com>

Thanks,
Ray
> -----Original Message-----
> From: Wu, Jiaxin <jiaxin.wu@intel.com>
> Sent: Tuesday, December 26, 2023 12:21 AM
> To: devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: [PATCH v2 5/6] UefiCpuPkg/PiSmmCpuDxeSmm: Invert
> ReleaseAllAPs & InitializeDebugAgent
> 
> Existing BSP handler stops source level debug, then call ReleaseAllAPs
> to tell all APs can reset the Present flag to FALSE:
>   InitializeDebugAgent (); /// Stop source level debug
>   ReleaseAllAPs ();        /// Tell APs can reset "Present" flag.
> 
> This patch is to invert ReleaseAllAPs & InitializeDebugAgent:
>   ReleaseAllAPs ();        /// Tell APs can reset "Present" flag.
>   InitializeDebugAgent (); /// Stop source level debug
> 
> After this change, there is no negative impact since SMM source level
> debug feature doesn't depend on AP's "Present" flag, no impact to the
> SMM source level debug capability.
> 
> Instead, the change will benefit the AP source level debug capability
> to trace its "Present" flag change for SMI exit since the source
> level debug feature will be stopped after each AP has the chance to
> reset the state.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zeng Star <star.zeng@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index bd2c9f841b..9aa9908863 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -648,23 +648,23 @@ BSPHandler (
>    //
>    // Wait for all APs to complete their pending tasks including MTRR
> programming if needed.
>    //
>    SmmCpuSyncWaitForAPs (mSmmMpSyncData->SyncContext, ApCount,
> CpuIndex);
> 
> +  //
> +  // Signal APs to Reset states/semaphore for this processor
> +  //
> +  ReleaseAllAPs ();
> +
>    if (mSmmDebugAgentSupport) {
>      //
>      // Stop source level debug in BSP handler, the code below will not be
>      // debugged.
>      //
>      InitializeDebugAgent (DEBUG_AGENT_INIT_EXIT_SMI, NULL, NULL);
>    }
> 
> -  //
> -  // Signal APs to Reset states/semaphore for this processor
> -  //
> -  ReleaseAllAPs ();
> -
>    //
>    // Perform pending operations for hot-plug
>    //
>    SmmCpuUpdate ();
> 
> --
> 2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112903): https://edk2.groups.io/g/devel/message/112903
Mute This Topic: https://groups.io/mt/103360806/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 6/6] UefiCpuPkg/PiSmmCpuDxeSmm: Reduce one round BSP & AP sync

[Wu, Jiaxin]

After BSP returned from SmmCoreEntry, there are several rounds BSP
and AP sync in BSP handler:

1 .ReleaseAllAPs();  /// Notify all APs to exit.
if (SmmCpuFeaturesNeedConfigureMtrrs()) {
  2. SmmCpuSyncWaitForAPs(); /// Wait for all APs to program MTRRs.
  3. ReleaseAllAPs(); /// Signal APs to restore MTRRs.
}

4. SmmCpuSyncWaitForAPs(); /// Wait for all APs to complete pending
                               tasks including MTRR.
5. ReleaseAllAPs(); /// Signal APs to Reset states.

6. SmmCpuSyncWaitForAPs(); /// Gather APs to exit SMM synchronously.

Before step 6 and after step 5, BSP performs below items:
A. InitializeDebugAgent() /// Stop source level debug.
B. SmmCpuUpdate() /// Perform pending operations for hot-plug.
C. Present = FALSE; /// Clear the Present flag of BSP.

For InitializeDebugAgent(), BSP needs to wait all APs complete
pending tasks and then notify all APs to stop source level debug.
So, above step 4 & step 5 are required for InitializeDebugAgent().

For SmmCpuUpdate(), it's to perform pending operations for
hot-plug CPUs take effect in next SMI. Existing APs in SMI do not
reply on the CPU switch & hot-add & hot-remove operations. So, no
need step 4 and step 5 for additional one round BSP & AP sync.
Step 6 can make sure all APs are ready to exit SMM, then hot-plug
operation can take effect in next SMI.

For BSP "Present" flag, AP does not reply on it. No need step 4
and step 5 for additional one round BSP & AP sync.

Based on above analysis, step 4 and step 5 are only required if
need configure MTRR and support SMM source level debug. So, we can
reduce one round BSP and AP sync if both are unsupported. With
this change, SMI performance can be improved.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 36 +++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 9aa9908863..e988ce0542 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -643,19 +643,21 @@ BSPHandler (
     //
     SmmCpuFeaturesReenableSmrr ();
     MtrrSetAllMtrrs (&Mtrrs);
   }
 
-  //
-  // Wait for all APs to complete their pending tasks including MTRR programming if needed.
-  //
-  SmmCpuSyncWaitForAPs (mSmmMpSyncData->SyncContext, ApCount, CpuIndex);
+  if (SmmCpuFeaturesNeedConfigureMtrrs () || mSmmDebugAgentSupport) {
+    //
+    // Wait for all APs to complete their pending tasks including MTRR programming if needed.
+    //
+    SmmCpuSyncWaitForAPs (mSmmMpSyncData->SyncContext, ApCount, CpuIndex);
 
-  //
-  // Signal APs to Reset states/semaphore for this processor
-  //
-  ReleaseAllAPs ();
+    //
+    // Signal APs to Reset states/semaphore for this processor
+    //
+    ReleaseAllAPs ();
+  }
 
   if (mSmmDebugAgentSupport) {
     //
     // Stop source level debug in BSP handler, the code below will not be
     // debugged.
@@ -894,19 +896,21 @@ APHandler (
     //
     SmmCpuFeaturesReenableSmrr ();
     MtrrSetAllMtrrs (&Mtrrs);
   }
 
-  //
-  // Notify BSP the readiness of this AP to Reset states/semaphore for this processor
-  //
-  SmmCpuSyncReleaseBsp (mSmmMpSyncData->SyncContext, CpuIndex, BspIndex);
+  if (SmmCpuFeaturesNeedConfigureMtrrs () || mSmmDebugAgentSupport) {
+    //
+    // Notify BSP the readiness of this AP to Reset states/semaphore for this processor
+    //
+    SmmCpuSyncReleaseBsp (mSmmMpSyncData->SyncContext, CpuIndex, BspIndex);
 
-  //
-  // Wait for the signal from BSP to Reset states/semaphore for this processor
-  //
-  SmmCpuSyncWaitForBsp (mSmmMpSyncData->SyncContext, CpuIndex, BspIndex);
+    //
+    // Wait for the signal from BSP to Reset states/semaphore for this processor
+    //
+    SmmCpuSyncWaitForBsp (mSmmMpSyncData->SyncContext, CpuIndex, BspIndex);
+  }
 
   //
   // Reset states/semaphore for this processor
   //
   *(mSmmMpSyncData->CpuData[CpuIndex].Present) = FALSE;
-- 
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112891): https://edk2.groups.io/g/devel/message/112891
Mute This Topic: https://groups.io/mt/103360807/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 6/6] UefiCpuPkg/PiSmmCpuDxeSmm: Reduce one round BSP & AP sync

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112905): https://edk2.groups.io/g/devel/message/112905
Mute This Topic: https://groups.io/mt/103360807/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-