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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-