1. Patchew
  2. EDK2
  3. View series

[edk2-devel] [Patch V3] UefiCpuPkg/PiSmmCpuDxeSmm: SmmCpuRendezvous ensure all Aps in Present.

Yuanhao Xie posted 1 patch 4 months, 2 weeks ago
Failed in applying to current master (apply log)
UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c     | 27 +++++++++++++++++++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 13 +++++++++++++
2 files changed, 40 insertions(+)
[edk2-devel] [Patch V3] UefiCpuPkg/PiSmmCpuDxeSmm: SmmCpuRendezvous ensure all Aps in Present.
Posted by Yuanhao Xie 4 months, 2 weeks ago 
SMM read save state requires AP must be present.
This patch aim to avoid the AP not ready for save state check.
Furthermore, SmmWaitForApArrival has two callers: SmmCpuRendezvous and
BSPHandler, the behavior of the two callers can be consistent by add
the while loop to check the present of the APs to SmmCpuRendezvous.

Signed-off-by: Zhihao Li <zhihao.li@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c     | 27 +++++++++++++++++++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 13 +++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
index c0485b0519..3e98955319 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
@@ -416,8 +416,15 @@ SmmCpuRendezvous (
   IN BOOLEAN                            BlockingMode
   )
 {
+  UINTN       Index;
+  UINTN       PresentCount;
+  UINT32      BlockedCount;
+  UINT32      DisabledCount;
   EFI_STATUS  Status;
 
+  BlockedCount  = 0;
+  DisabledCount = 0;
+
   //
   // Return success immediately if all CPUs are already synchronized.
   //
@@ -436,6 +443,26 @@ SmmCpuRendezvous (
     // There are some APs outside SMM, Wait for all avaiable APs to arrive.
     //
     SmmWaitForApArrival ();
+
+    //
+    // Make sure all APs have their Present flag set
+    //
+    while (TRUE) {
+      PresentCount = 0;
+      for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
+        if (*(mSmmMpSyncData->CpuData[Index].Present)) {
+          PresentCount++;
+        }
+      }
+
+      GetSmmDelayedBlockedDisabledCount (NULL, &BlockedCount, &DisabledCount);
+      if (PresentCount == mMaxNumberOfCpus - BlockedCount - DisabledCount ) {
+        break;
+      }
+
+      CpuPause ();
+    }
+
     Status = mSmmMpSyncData->AllApArrivedWithException ? EFI_SUCCESS : EFI_TIMEOUT;
   } else {
     //
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index f18345881b..e3538957fb 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -1552,6 +1552,19 @@ SmmWaitForApArrival (
   VOID
   );
 
+/**
+  Returns the Number of SMM Delayed & Blocked & Disabled Thread Count.
+  @param[in,out] DelayedCount  The Number of SMM Delayed Thread Count.
+  @param[in,out] BlockedCount  The Number of SMM Blocked Thread Count.
+  @param[in,out] DisabledCount The Number of SMM Disabled Thread Count.
+**/
+VOID
+GetSmmDelayedBlockedDisabledCount (
+  IN OUT UINT32  *DelayedCount,
+  IN OUT UINT32  *BlockedCount,
+  IN OUT UINT32  *DisabledCount
+  );
+
 /**
   Write unprotect read-only pages if Cr0.Bits.WP is 1.
 
-- 
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112589): https://edk2.groups.io/g/devel/message/112589
Mute This Topic: https://groups.io/mt/103187773/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch V3] UefiCpuPkg/PiSmmCpuDxeSmm: SmmCpuRendezvous ensure all Aps in Present.
Posted by Ni, Ray 4 months, 1 week ago 
Yuanhao, Zhihao,
Can you kindly explain a bit more about this sentence in commit message " SMM read save state requires AP must be present."?

Thanks,
Ray
> -----Original Message-----
> From: Xie, Yuanhao <yuanhao.xie@intel.com>
> Sent: Friday, December 15, 2023 5:30 PM
> To: devel@edk2.groups.io
> Cc: Xie, Yuanhao <yuanhao.xie@intel.com>; Li, Zhihao <zhihao.li@intel.com>;
> Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: [Patch V3] UefiCpuPkg/PiSmmCpuDxeSmm: SmmCpuRendezvous
> ensure all Aps in Present.
> 
> SMM read save state requires AP must be present.
> This patch aim to avoid the AP not ready for save state check.
> Furthermore, SmmWaitForApArrival has two callers: SmmCpuRendezvous and
> BSPHandler, the behavior of the two callers can be consistent by add
> the while loop to check the present of the APs to SmmCpuRendezvous.
> 
> Signed-off-by: Zhihao Li <zhihao.li@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c     | 27
> +++++++++++++++++++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 13 +++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> index c0485b0519..3e98955319 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> @@ -416,8 +416,15 @@ SmmCpuRendezvous (
>    IN BOOLEAN                            BlockingMode
>    )
>  {
> +  UINTN       Index;
> +  UINTN       PresentCount;
> +  UINT32      BlockedCount;
> +  UINT32      DisabledCount;
>    EFI_STATUS  Status;
> 
> +  BlockedCount  = 0;
> +  DisabledCount = 0;
> +
>    //
>    // Return success immediately if all CPUs are already synchronized.
>    //
> @@ -436,6 +443,26 @@ SmmCpuRendezvous (
>      // There are some APs outside SMM, Wait for all avaiable APs to arrive.
>      //
>      SmmWaitForApArrival ();
> +
> +    //
> +    // Make sure all APs have their Present flag set
> +    //
> +    while (TRUE) {
> +      PresentCount = 0;
> +      for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
> +        if (*(mSmmMpSyncData->CpuData[Index].Present)) {
> +          PresentCount++;
> +        }
> +      }
> +
> +      GetSmmDelayedBlockedDisabledCount (NULL, &BlockedCount,
> &DisabledCount);
> +      if (PresentCount == mMaxNumberOfCpus - BlockedCount -
> DisabledCount ) {
> +        break;
> +      }
> +
> +      CpuPause ();
> +    }
> +
>      Status = mSmmMpSyncData->AllApArrivedWithException ?
> EFI_SUCCESS : EFI_TIMEOUT;
>    } else {
>      //
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index f18345881b..e3538957fb 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1552,6 +1552,19 @@ SmmWaitForApArrival (
>    VOID
>    );
> 
> +/**
> +  Returns the Number of SMM Delayed & Blocked & Disabled Thread Count.
> +  @param[in,out] DelayedCount  The Number of SMM Delayed Thread
> Count.
> +  @param[in,out] BlockedCount  The Number of SMM Blocked Thread
> Count.
> +  @param[in,out] DisabledCount The Number of SMM Disabled Thread
> Count.
> +**/
> +VOID
> +GetSmmDelayedBlockedDisabledCount (
> +  IN OUT UINT32  *DelayedCount,
> +  IN OUT UINT32  *BlockedCount,
> +  IN OUT UINT32  *DisabledCount
> +  );
> +
>  /**
>    Write unprotect read-only pages if Cr0.Bits.WP is 1.
> 
> --
> 2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112683): https://edk2.groups.io/g/devel/message/112683
Mute This Topic: https://groups.io/mt/103187773/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch V3] UefiCpuPkg/PiSmmCpuDxeSmm: SmmCpuRendezvous ensure all Aps in Present.
Posted by Yuanhao Xie 4 months, 1 week ago 
Hi Ray,

It means that:
The "SmmCpuRendezvous" function has a bug in that it summons all APs into SMM, but there is a time gap before they are set as "present." During this window, if using the "setVariable" operation, it can cause issues because "setVariable" requires all APs to be in SMM and marked as "present." Therefore, this patch adds extra logic to "SmmCpuRendezvous" to check if the APs are already "present" before proceeding.

Thanks,
Yuanhao
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Tuesday, December 19, 2023 12:01 PM
To: Xie, Yuanhao <yuanhao.xie@intel.com>; devel@edk2.groups.io
Cc: Li, Zhihao <zhihao.li@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
Subject: RE: [Patch V3] UefiCpuPkg/PiSmmCpuDxeSmm: SmmCpuRendezvous ensure all Aps in Present.

Yuanhao, Zhihao,
Can you kindly explain a bit more about this sentence in commit message " SMM read save state requires AP must be present."?

Thanks,
Ray
> -----Original Message-----
> From: Xie, Yuanhao <yuanhao.xie@intel.com>
> Sent: Friday, December 15, 2023 5:30 PM
> To: devel@edk2.groups.io
> Cc: Xie, Yuanhao <yuanhao.xie@intel.com>; Li, Zhihao 
> <zhihao.li@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R 
> <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Wu, 
> Jiaxin <jiaxin.wu@intel.com>
> Subject: [Patch V3] UefiCpuPkg/PiSmmCpuDxeSmm: SmmCpuRendezvous ensure 
> all Aps in Present.
> 
> SMM read save state requires AP must be present.
> This patch aim to avoid the AP not ready for save state check.
> Furthermore, SmmWaitForApArrival has two callers: SmmCpuRendezvous and 
> BSPHandler, the behavior of the two callers can be consistent by add 
> the while loop to check the present of the APs to SmmCpuRendezvous.
> 
> Signed-off-by: Zhihao Li <zhihao.li@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c     | 27
> +++++++++++++++++++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 13 +++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> index c0485b0519..3e98955319 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> @@ -416,8 +416,15 @@ SmmCpuRendezvous (
>    IN BOOLEAN                            BlockingMode
>    )
>  {
> +  UINTN       Index;
> +  UINTN       PresentCount;
> +  UINT32      BlockedCount;
> +  UINT32      DisabledCount;
>    EFI_STATUS  Status;
> 
> +  BlockedCount  = 0;
> +  DisabledCount = 0;
> +
>    //
>    // Return success immediately if all CPUs are already synchronized.
>    //
> @@ -436,6 +443,26 @@ SmmCpuRendezvous (
>      // There are some APs outside SMM, Wait for all avaiable APs to arrive.
>      //
>      SmmWaitForApArrival ();
> +
> +    //
> +    // Make sure all APs have their Present flag set
> +    //
> +    while (TRUE) {
> +      PresentCount = 0;
> +      for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
> +        if (*(mSmmMpSyncData->CpuData[Index].Present)) {
> +          PresentCount++;
> +        }
> +      }
> +
> +      GetSmmDelayedBlockedDisabledCount (NULL, &BlockedCount,
> &DisabledCount);
> +      if (PresentCount == mMaxNumberOfCpus - BlockedCount -
> DisabledCount ) {
> +        break;
> +      }
> +
> +      CpuPause ();
> +    }
> +
>      Status = mSmmMpSyncData->AllApArrivedWithException ?
> EFI_SUCCESS : EFI_TIMEOUT;
>    } else {
>      //
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index f18345881b..e3538957fb 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1552,6 +1552,19 @@ SmmWaitForApArrival (
>    VOID
>    );
> 
> +/**
> +  Returns the Number of SMM Delayed & Blocked & Disabled Thread Count.
> +  @param[in,out] DelayedCount  The Number of SMM Delayed Thread
> Count.
> +  @param[in,out] BlockedCount  The Number of SMM Blocked Thread
> Count.
> +  @param[in,out] DisabledCount The Number of SMM Disabled Thread
> Count.
> +**/
> +VOID
> +GetSmmDelayedBlockedDisabledCount (
> +  IN OUT UINT32  *DelayedCount,
> +  IN OUT UINT32  *BlockedCount,
> +  IN OUT UINT32  *DisabledCount
> +  );
> +
>  /**
>    Write unprotect read-only pages if Cr0.Bits.WP is 1.
> 
> --
> 2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112689): https://edk2.groups.io/g/devel/message/112689
Mute This Topic: https://groups.io/mt/103187773/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.