[edk2-devel] [PATCH 0/6] Support to unregister SMI handler in SMI handlers

[edk2-devel] [PATCH 0/6] Support to unregister SMI handler in SMI handlers

[Zhiguang Liu]

Months ago, I sent patch set to fix potential issues in the usage of
SMI handler unregistering SMI handler. Discussion can be found in below
link:
https://edk2.groups.io/g/devel/topic/103925794#114251

The conclusion was to only support SMI handler unregistering itself,
and not allow  SMI handler unregistering other handlers, because we
thought there would be no such usage.
However, recently, I find in some platform, there is kind of usage.
To also support SMI handler unregistering other handlers, this patch
set use a totally different design. So I revert the former ones first
to make the code more readable.

Thank Laszlo for bring up the initial idea for the new design.

Zhiguang Liu (6):
  Revert 2ec8f0c6407f062441b205b900038933865c7b3c
  Revert 049ff6c39c73edd3709c05bd0e46184320471358
  Revert 17b28722008eab745ce186b72cd325944cbe6bf0
  Revert ae1079b386a597108a8070652bf7cdaa4ec3dda3
  MdeModulePkg/SMM: Support to unregister SMI handler in SMI handlers
  StandaloneMmPkg: Support to unregister MMI handler in MMI handlers

 MdeModulePkg/Core/PiSmmCore/PiSmmCore.h |   1 +
 MdeModulePkg/Core/PiSmmCore/Smi.c       | 142 ++++++++++++++++++------
 StandaloneMmPkg/Core/Mmi.c              | 141 +++++++++++++++++------
 3 files changed, 217 insertions(+), 67 deletions(-)

-- 
2.31.1.windows.1



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

[edk2-devel] [PATCH 1/6] Revert 2ec8f0c6407f062441b205b900038933865c7b3c

[Zhiguang Liu]

This reverts commit "StandaloneMmPkg: Disallow unregister MMI
handler in other MMI handler" for better design later.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 StandaloneMmPkg/Core/Mmi.c | 32 +++++++++-----------------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/StandaloneMmPkg/Core/Mmi.c b/StandaloneMmPkg/Core/Mmi.c
index 9e52072bf7..c1a1d76e85 100644
--- a/StandaloneMmPkg/Core/Mmi.c
+++ b/StandaloneMmPkg/Core/Mmi.c
@@ -36,9 +36,8 @@ typedef struct {
   MMI_ENTRY                     *MmiEntry;
 } MMI_HANDLER;
 
-LIST_ENTRY   mRootMmiHandlerList = INITIALIZE_LIST_HEAD_VARIABLE (mRootMmiHandlerList);
-LIST_ENTRY   mMmiEntryList       = INITIALIZE_LIST_HEAD_VARIABLE (mMmiEntryList);
-MMI_HANDLER  *mCurrentMmiHandler = NULL;
+LIST_ENTRY  mRootMmiHandlerList = INITIALIZE_LIST_HEAD_VARIABLE (mRootMmiHandlerList);
+LIST_ENTRY  mMmiEntryList       = INITIALIZE_LIST_HEAD_VARIABLE (mMmiEntryList);
 
 /**
   Finds the MMI entry for the requested handler type.
@@ -162,19 +161,13 @@ MmiManage (
     // get next node before handler is executed, since LIST_ENTRY that
     // Link points to may be freed if unregister MMI handler.
     //
-    Link = Link->ForwardLink;
-    //
-    // Assign gCurrentMmiHandle before calling the MMI handler and
-    // set to NULL when it returns.
-    //
-    mCurrentMmiHandler = MmiHandler;
-    Status             = MmiHandler->Handler (
-                                       (EFI_HANDLE)MmiHandler,
-                                       Context,
-                                       CommBuffer,
-                                       CommBufferSize
-                                       );
-    mCurrentMmiHandler = NULL;
+    Link   = Link->ForwardLink;
+    Status = MmiHandler->Handler (
+                           (EFI_HANDLE)MmiHandler,
+                           Context,
+                           CommBuffer,
+                           CommBufferSize
+                           );
 
     switch (Status) {
       case EFI_INTERRUPT_PENDING:
@@ -321,13 +314,6 @@ MmiHandlerUnRegister (
     return EFI_INVALID_PARAMETER;
   }
 
-  //
-  // Do not allow to unregister MMI Handler inside other MMI Handler
-  //
-  if ((mCurrentMmiHandler != NULL) && (mCurrentMmiHandler != MmiHandler)) {
-    return EFI_INVALID_PARAMETER;
-  }
-
   MmiEntry = MmiHandler->MmiEntry;
 
   RemoveEntryList (&MmiHandler->Link);
-- 
2.31.1.windows.1



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

[edk2-devel] [PATCH 2/6] Revert 049ff6c39c73edd3709c05bd0e46184320471358

[Zhiguang Liu]

This reverts commit "StandaloneMmPkg: Support to unregister
MMI handler inside MMI handler" for better design later.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 StandaloneMmPkg/Core/Mmi.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/StandaloneMmPkg/Core/Mmi.c b/StandaloneMmPkg/Core/Mmi.c
index c1a1d76e85..0de6fd17fc 100644
--- a/StandaloneMmPkg/Core/Mmi.c
+++ b/StandaloneMmPkg/Core/Mmi.c
@@ -154,14 +154,9 @@ MmiManage (
     Head = &MmiEntry->MmiHandlers;
   }
 
-  for (Link = Head->ForwardLink; Link != Head;) {
+  for (Link = Head->ForwardLink; Link != Head; Link = Link->ForwardLink) {
     MmiHandler = CR (Link, MMI_HANDLER, Link, MMI_HANDLER_SIGNATURE);
-    //
-    // To support unregister MMI handler inside MMI handler itself,
-    // get next node before handler is executed, since LIST_ENTRY that
-    // Link points to may be freed if unregister MMI handler.
-    //
-    Link   = Link->ForwardLink;
+
     Status = MmiHandler->Handler (
                            (EFI_HANDLE)MmiHandler,
                            Context,
-- 
2.31.1.windows.1



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

[edk2-devel] [PATCH 3/6] Revert 17b28722008eab745ce186b72cd325944cbe6bf0

[Zhiguang Liu]

This reverts commit "MdeModulePkg/SMM: Disallow unregister
 SMI handler in other SMI handler" for better design later.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdeModulePkg/Core/PiSmmCore/Smi.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c b/MdeModulePkg/Core/PiSmmCore/Smi.c
index b3a81ac877..3489c130fd 100644
--- a/MdeModulePkg/Core/PiSmmCore/Smi.c
+++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
@@ -8,8 +8,7 @@
 
 #include "PiSmmCore.h"
 
-SMI_HANDLER  *mCurrentSmiHandler = NULL;
-LIST_ENTRY   mSmiEntryList       = INITIALIZE_LIST_HEAD_VARIABLE (mSmiEntryList);
+LIST_ENTRY  mSmiEntryList = INITIALIZE_LIST_HEAD_VARIABLE (mSmiEntryList);
 
 SMI_ENTRY  mRootSmiEntry = {
   SMI_ENTRY_SIGNATURE,
@@ -143,18 +142,13 @@ SmiManage (
     // Link points to may be freed if unregister SMI handler.
     //
     Link = Link->ForwardLink;
-    //
-    // Assign gCurrentSmiHandle before calling the SMI handler and
-    // set to NULL when it returns.
-    //
-    mCurrentSmiHandler = SmiHandler;
-    Status             = SmiHandler->Handler (
-                                       (EFI_HANDLE)SmiHandler,
-                                       Context,
-                                       CommBuffer,
-                                       CommBufferSize
-                                       );
-    mCurrentSmiHandler = NULL;
+
+    Status = SmiHandler->Handler (
+                           (EFI_HANDLE)SmiHandler,
+                           Context,
+                           CommBuffer,
+                           CommBufferSize
+                           );
 
     switch (Status) {
       case EFI_INTERRUPT_PENDING:
@@ -334,13 +328,6 @@ SmiHandlerUnRegister (
     return EFI_INVALID_PARAMETER;
   }
 
-  //
-  // Do not allow to unregister SMI Handler inside other SMI Handler
-  //
-  if ((mCurrentSmiHandler != NULL) && (mCurrentSmiHandler != SmiHandler)) {
-    return EFI_INVALID_PARAMETER;
-  }
-
   SmiEntry = SmiHandler->SmiEntry;
 
   RemoveEntryList (&SmiHandler->Link);
-- 
2.31.1.windows.1



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

[edk2-devel] [PATCH 4/6] Revert ae1079b386a597108a8070652bf7cdaa4ec3dda3

[Zhiguang Liu]

This reverts "MdeModulePkg/SMM: Support to unregister
SMI handler inside SMI handler" for better design later.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdeModulePkg/Core/PiSmmCore/Smi.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c b/MdeModulePkg/Core/PiSmmCore/Smi.c
index 3489c130fd..2985f989c3 100644
--- a/MdeModulePkg/Core/PiSmmCore/Smi.c
+++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
@@ -134,14 +134,8 @@ SmiManage (
 
   Head = &SmiEntry->SmiHandlers;
 
-  for (Link = Head->ForwardLink; Link != Head;) {
+  for (Link = Head->ForwardLink; Link != Head; Link = Link->ForwardLink) {
     SmiHandler = CR (Link, SMI_HANDLER, Link, SMI_HANDLER_SIGNATURE);
-    //
-    // To support unregister SMI handler inside SMI handler itself,
-    // get next node before handler is executed, since LIST_ENTRY that
-    // Link points to may be freed if unregister SMI handler.
-    //
-    Link = Link->ForwardLink;
 
     Status = SmiHandler->Handler (
                            (EFI_HANDLE)SmiHandler,
-- 
2.31.1.windows.1



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

[edk2-devel] [PATCH 5/6] MdeModulePkg/SMM: Support to unregister SMI handler in SMI handlers

[Zhiguang Liu]

Unregistering SMI handler will free the SMI_HANDLER. However, the
SmiManage() may be using the link node from SMI_HANDLER for loop if
the unregistering happens in SMI handlers.
To avoid that, the idea is to inform SmiHandlerUnRegister() whether
it's running or not running on the stack of SmiManage(), and to
postpone SMI_HANDLER deletion before SmiManage() returns.
Noted SmiManage() may be called recursively, the SmiHandlerUnRegister()
won't take effect until the root SmiManage() returns.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.h |   1 +
 MdeModulePkg/Core/PiSmmCore/Smi.c       | 105 ++++++++++++++++++++++--
 2 files changed, 100 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
index b8a490a8c3..6cc32e94d8 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
@@ -93,6 +93,7 @@ typedef struct {
   SMI_ENTRY                       *SmiEntry;
   VOID                            *Context;    // for profile
   UINTN                           ContextSize; // for profile
+  BOOLEAN                         NeedDeleted; // To delete this SMI_HANDLER later
 } SMI_HANDLER;
 
 //
diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c b/MdeModulePkg/Core/PiSmmCore/Smi.c
index 2985f989c3..b76aaf67e4 100644
--- a/MdeModulePkg/Core/PiSmmCore/Smi.c
+++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
@@ -8,6 +8,11 @@
 
 #include "PiSmmCore.h"
 
+//
+// mSmiManageCallingDepth is used to track the depth of recursive calls of SmiManage.
+//
+UINTN  mSmiManageCallingDepth = 0;
+
 LIST_ENTRY  mSmiEntryList = INITIALIZE_LIST_HEAD_VARIABLE (mSmiEntryList);
 
 SMI_ENTRY  mRootSmiEntry = {
@@ -104,13 +109,15 @@ SmiManage (
 {
   LIST_ENTRY   *Link;
   LIST_ENTRY   *Head;
+  LIST_ENTRY   *EntryLink;
   SMI_ENTRY    *SmiEntry;
   SMI_HANDLER  *SmiHandler;
   BOOLEAN      SuccessReturn;
+  BOOLEAN      CanReturn;
   EFI_STATUS   Status;
 
   PERF_FUNCTION_BEGIN ();
-
+  mSmiManageCallingDepth++;
   Status        = EFI_NOT_FOUND;
   SuccessReturn = FALSE;
   if (HandlerType == NULL) {
@@ -152,7 +159,12 @@ SmiManage (
         //
         if (HandlerType != NULL) {
           PERF_FUNCTION_END ();
-          return EFI_INTERRUPT_PENDING;
+          //
+          // Won't go to next Handler, and will return with EFI_INTERRUPT_PENDING later
+          //
+          SuccessReturn = FALSE;
+          Status        = EFI_INTERRUPT_PENDING;
+          CanReturn     = TRUE;
         }
 
         break;
@@ -165,7 +177,10 @@ SmiManage (
         //
         if (HandlerType != NULL) {
           PERF_FUNCTION_END ();
-          return EFI_SUCCESS;
+          //
+          // Won't go to next Handler, and return with EFI_SUCCESS
+          //
+          CanReturn = TRUE;
         }
 
         SuccessReturn = TRUE;
@@ -193,12 +208,79 @@ SmiManage (
         ASSERT (FALSE);
         break;
     }
+
+    if (CanReturn) {
+      break;
+    }
   }
 
   if (SuccessReturn) {
     Status = EFI_SUCCESS;
   }
 
+  ASSERT (mSmiManageCallingDepth > 0);
+  mSmiManageCallingDepth--;
+
+  //
+  // The SmiHandlerUnRegister won't take effect inside SmiManage.
+  // Before returned from SmiManage, delete the SmiHandler which is
+  // marked as NeedDeleted.
+  // Note that SmiManage can be called recursively.
+  //
+  if (mSmiManageCallingDepth == 0) {
+    //
+    // Go through all SmiHandler in root SMI handlers
+    //
+    SmiHandler = NULL;
+    for ( Link = GetFirstNode (&mRootSmiEntry.SmiHandlers)
+          ; !IsNull (&mRootSmiEntry.SmiHandlers, Link);
+          )
+    {
+      SmiHandler = CR (Link, SMI_HANDLER, Link, SMI_HANDLER_SIGNATURE);
+      Link       = GetNextNode (&mRootSmiEntry.SmiHandlers, Link);
+      if (SmiHandler->NeedDeleted) {
+        //
+        // Remove SmiHandler if the NeedDeleted is set.
+        //
+        RemoveEntryList (&SmiHandler->Link);
+        FreePool (SmiHandler);
+      }
+    }
+
+    //
+    // Go through all SmiHandler in non-root SMI handlers
+    //
+    for ( EntryLink = GetFirstNode (&mSmiEntryList)
+          ; !IsNull (&mSmiEntryList, EntryLink);
+          )
+    {
+      SmiEntry  = CR (EntryLink, SMI_ENTRY, AllEntries, SMI_ENTRY_SIGNATURE);
+      EntryLink = GetNextNode (&mSmiEntryList, EntryLink);
+      for ( Link = GetFirstNode (&SmiEntry->SmiHandlers)
+            ; !IsNull (&SmiEntry->SmiHandlers, Link);
+            )
+      {
+        SmiHandler = CR (Link, SMI_HANDLER, Link, SMI_HANDLER_SIGNATURE);
+        Link       = GetNextNode (&SmiEntry->SmiHandlers, Link);
+        if (SmiHandler->NeedDeleted) {
+          //
+          // Remove SmiHandler if the NeedDeleted is set.
+          //
+          RemoveEntryList (&SmiHandler->Link);
+          FreePool (SmiHandler);
+        }
+      }
+
+      if (IsListEmpty (&SmiEntry->SmiHandlers)) {
+        //
+        // No handler registered for this SmiEntry now, remove the SMI_ENTRY
+        //
+        RemoveEntryList (&SmiEntry->AllEntries);
+        FreePool (SmiEntry);
+      }
+    }
+  }
+
   PERF_FUNCTION_END ();
   return Status;
 }
@@ -235,9 +317,10 @@ SmiHandlerRegister (
     return EFI_OUT_OF_RESOURCES;
   }
 
-  SmiHandler->Signature  = SMI_HANDLER_SIGNATURE;
-  SmiHandler->Handler    = Handler;
-  SmiHandler->CallerAddr = (UINTN)RETURN_ADDRESS (0);
+  SmiHandler->Signature   = SMI_HANDLER_SIGNATURE;
+  SmiHandler->Handler     = Handler;
+  SmiHandler->CallerAddr  = (UINTN)RETURN_ADDRESS (0);
+  SmiHandler->NeedDeleted = FALSE;
 
   if (HandlerType == NULL) {
     //
@@ -324,6 +407,16 @@ SmiHandlerUnRegister (
 
   SmiEntry = SmiHandler->SmiEntry;
 
+  if (mSmiManageCallingDepth > 0) {
+    //
+    // This function is called from SmiManage()
+    // Do not delete or remove SmiHandler or SmiEntry now.
+    // Set the NeedDeleted field in SmiHandler so that SmiManage will delete it later
+    //
+    SmiHandler->NeedDeleted = TRUE;
+    return EFI_SUCCESS;
+  }
+
   RemoveEntryList (&SmiHandler->Link);
   FreePool (SmiHandler);
 
-- 
2.31.1.windows.1



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

[edk2-devel] [PATCH 6/6] StandaloneMmPkg: Support to unregister MMI handler in MMI handlers

[Zhiguang Liu]

Unregistering MMI handler will free the MMI_HANDLER. However, the
MmiManage() may be using the link node from MMI_HANDLER for loop if
the unregistering happens in MMI handlers.
To avoid that, the idea is to inform MmiHandlerUnRegister() whether
it's running or not running on the stack of MmiManage(), and to
postpone MMI_HANDLER deletion until after the loop finishes.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 StandaloneMmPkg/Core/Mmi.c | 102 +++++++++++++++++++++++++++++++++++--
 1 file changed, 98 insertions(+), 4 deletions(-)

diff --git a/StandaloneMmPkg/Core/Mmi.c b/StandaloneMmPkg/Core/Mmi.c
index 0de6fd17fc..d9cdd1f1a7 100644
--- a/StandaloneMmPkg/Core/Mmi.c
+++ b/StandaloneMmPkg/Core/Mmi.c
@@ -34,8 +34,14 @@ typedef struct {
   LIST_ENTRY                    Link;        // Link on MMI_ENTRY.MmiHandlers
   EFI_MM_HANDLER_ENTRY_POINT    Handler;     // The mm handler's entry point
   MMI_ENTRY                     *MmiEntry;
+  BOOLEAN                       NeedDeleted; // To delete this MMI_HANDLER later
 } MMI_HANDLER;
 
+//
+// mMmiManageCallingDepth is used to track the depth of recursive calls of MmiManage.
+//
+UINTN  mMmiManageCallingDepth = 0;
+
 LIST_ENTRY  mRootMmiHandlerList = INITIALIZE_LIST_HEAD_VARIABLE (mRootMmiHandlerList);
 LIST_ENTRY  mMmiEntryList       = INITIALIZE_LIST_HEAD_VARIABLE (mMmiEntryList);
 
@@ -126,11 +132,14 @@ MmiManage (
 {
   LIST_ENTRY   *Link;
   LIST_ENTRY   *Head;
+  LIST_ENTRY   *EntryLink;
   MMI_ENTRY    *MmiEntry;
   MMI_HANDLER  *MmiHandler;
   BOOLEAN      SuccessReturn;
+  BOOLEAN      CanReturn;
   EFI_STATUS   Status;
 
+  mMmiManageCallingDepth++;
   Status        = EFI_NOT_FOUND;
   SuccessReturn = FALSE;
   if (HandlerType == NULL) {
@@ -171,7 +180,12 @@ MmiManage (
         // no additional handlers will be processed and EFI_INTERRUPT_PENDING will be returned.
         //
         if (HandlerType != NULL) {
-          return EFI_INTERRUPT_PENDING;
+          //
+          // Won't go to next Handler, and will return with EFI_INTERRUPT_PENDING later
+          //
+          SuccessReturn = FALSE;
+          Status        = EFI_INTERRUPT_PENDING;
+          CanReturn     = TRUE;
         }
 
         break;
@@ -183,7 +197,10 @@ MmiManage (
         // additional handlers will be processed.
         //
         if (HandlerType != NULL) {
-          return EFI_SUCCESS;
+          //
+          // Won't go to next Handler, and return with EFI_SUCCESS
+          //
+          CanReturn = TRUE;
         }
 
         SuccessReturn = TRUE;
@@ -211,12 +228,78 @@ MmiManage (
         ASSERT_EFI_ERROR (Status);
         break;
     }
+
+    if (CanReturn) {
+      break;
+    }
   }
 
   if (SuccessReturn) {
     Status = EFI_SUCCESS;
   }
 
+  ASSERT (mMmiManageCallingDepth > 0);
+  mMmiManageCallingDepth--;
+
+  //
+  // The MmiHandlerUnRegister won't take effect inside MmiManage.
+  // Before returned from MmiManage, delete the MmiHandler which is
+  // marked as NeedDeleted.
+  // Note that MmiManage can be called recursively.
+  //
+  if (mMmiManageCallingDepth == 0) {
+    //
+    // Go through all MmiHandler in root Mmi handlers
+    //
+    for ( Link = GetFirstNode (&mRootMmiHandlerList)
+          ; !IsNull (&mRootMmiHandlerList, Link);
+          )
+    {
+      MmiHandler = CR (Link, MMI_HANDLER, Link, MMI_HANDLER_SIGNATURE);
+      Link       = GetNextNode (&mRootMmiHandlerList, Link);
+      if (MmiHandler->NeedDeleted) {
+        //
+        // Remove MmiHandler if the NeedDeleted is set.
+        //
+        RemoveEntryList (&MmiHandler->Link);
+        FreePool (MmiHandler);
+      }
+    }
+
+    //
+    // Go through all MmiHandler in non-root MMI handlers
+    //
+    for ( EntryLink = GetFirstNode (&mMmiEntryList)
+          ; !IsNull (&mMmiEntryList, EntryLink);
+          )
+    {
+      MmiEntry  = CR (EntryLink, MMI_ENTRY, AllEntries, MMI_ENTRY_SIGNATURE);
+      EntryLink = GetNextNode (&mMmiEntryList, EntryLink);
+      for ( Link = GetFirstNode (&MmiEntry->MmiHandlers)
+            ; !IsNull (&MmiEntry->MmiHandlers, Link);
+            )
+      {
+        MmiHandler = CR (Link, MMI_HANDLER, Link, MMI_HANDLER_SIGNATURE);
+        Link       = GetNextNode (&MmiEntry->MmiHandlers, Link);
+        if (MmiHandler->NeedDeleted) {
+          //
+          // Remove MmiHandler if the NeedDeleted is set.
+          //
+          RemoveEntryList (&MmiHandler->Link);
+          FreePool (MmiHandler);
+        }
+      }
+
+      if (IsListEmpty (&MmiEntry->MmiHandlers)) {
+        //
+        // No handler registered for this MmiEntry now, remove the MMI_ENTRY
+        //
+        RemoveEntryList (&MmiEntry->AllEntries);
+        FreePool (MmiEntry);
+      }
+    }
+  }
+
   return Status;
 }
 
@@ -252,8 +335,9 @@ MmiHandlerRegister (
     return EFI_OUT_OF_RESOURCES;
   }
 
-  MmiHandler->Signature = MMI_HANDLER_SIGNATURE;
-  MmiHandler->Handler   = Handler;
+  MmiHandler->Signature   = MMI_HANDLER_SIGNATURE;
+  MmiHandler->Handler     = Handler;
+  MmiHandler->NeedDeleted = FALSE;
 
   if (HandlerType == NULL) {
     //
@@ -311,6 +395,16 @@ MmiHandlerUnRegister (
 
   MmiEntry = MmiHandler->MmiEntry;
 
+  if (mMmiManageCallingDepth > 0) {
+    //
+    // This function is called from MmiManage()
+    // Do not delete or remove MmiHandler or MmiEntry now.
+    // Set the NeedDeleted field in MmiHandler so that MmiManage will delete it later
+    //
+    MmiHandler->NeedDeleted = TRUE;
+    return EFI_SUCCESS;
+  }
+
   RemoveEntryList (&MmiHandler->Link);
   FreePool (MmiHandler);
 
-- 
2.31.1.windows.1



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

Re: [edk2-devel] [PATCH 6/6] StandaloneMmPkg: Support to unregister MMI handler in MMI handlers

[Ni, Ray]

[-- Attachment #1: Type: text/plain, Size: 7271 bytes --]

Zhiguang,
I feel that you could simplify the logic in MmiManage () by adding a new local variable to cache the return status.
Can you think about that and come up with a new version of patch?

Also, the list maninpulation logics are duplicated in MmiManage () and Unregister(). can you try to avoid it?

Thanks,
Ray

________________________________
From: Liu, Zhiguang <zhiguang.liu@intel.com>
Sent: Monday, March 18, 2024 10:19
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.com>
Subject: [PATCH 6/6] StandaloneMmPkg: Support to unregister MMI handler in MMI handlers

Unregistering MMI handler will free the MMI_HANDLER. However, the
MmiManage() may be using the link node from MMI_HANDLER for loop if
the unregistering happens in MMI handlers.
To avoid that, the idea is to inform MmiHandlerUnRegister() whether
it's running or not running on the stack of MmiManage(), and to
postpone MMI_HANDLER deletion until after the loop finishes.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 StandaloneMmPkg/Core/Mmi.c | 102 +++++++++++++++++++++++++++++++++++--
 1 file changed, 98 insertions(+), 4 deletions(-)

diff --git a/StandaloneMmPkg/Core/Mmi.c b/StandaloneMmPkg/Core/Mmi.c
index 0de6fd17fc..d9cdd1f1a7 100644
--- a/StandaloneMmPkg/Core/Mmi.c
+++ b/StandaloneMmPkg/Core/Mmi.c
@@ -34,8 +34,14 @@ typedef struct {
   LIST_ENTRY                    Link;        // Link on MMI_ENTRY.MmiHandlers
   EFI_MM_HANDLER_ENTRY_POINT    Handler;     // The mm handler's entry point
   MMI_ENTRY                     *MmiEntry;
+  BOOLEAN                       NeedDeleted; // To delete this MMI_HANDLER later
 } MMI_HANDLER;

+//
+// mMmiManageCallingDepth is used to track the depth of recursive calls of MmiManage.
+//
+UINTN  mMmiManageCallingDepth = 0;
+
 LIST_ENTRY  mRootMmiHandlerList = INITIALIZE_LIST_HEAD_VARIABLE (mRootMmiHandlerList);
 LIST_ENTRY  mMmiEntryList       = INITIALIZE_LIST_HEAD_VARIABLE (mMmiEntryList);

@@ -126,11 +132,14 @@ MmiManage (
 {
   LIST_ENTRY   *Link;
   LIST_ENTRY   *Head;
+  LIST_ENTRY   *EntryLink;
   MMI_ENTRY    *MmiEntry;
   MMI_HANDLER  *MmiHandler;
   BOOLEAN      SuccessReturn;
+  BOOLEAN      CanReturn;
   EFI_STATUS   Status;

+  mMmiManageCallingDepth++;
   Status        = EFI_NOT_FOUND;
   SuccessReturn = FALSE;
   if (HandlerType == NULL) {
@@ -171,7 +180,12 @@ MmiManage (
         // no additional handlers will be processed and EFI_INTERRUPT_PENDING will be returned.
         //
         if (HandlerType != NULL) {
-          return EFI_INTERRUPT_PENDING;
+          //
+          // Won't go to next Handler, and will return with EFI_INTERRUPT_PENDING later
+          //
+          SuccessReturn = FALSE;
+          Status        = EFI_INTERRUPT_PENDING;
+          CanReturn     = TRUE;
         }

         break;
@@ -183,7 +197,10 @@ MmiManage (
         // additional handlers will be processed.
         //
         if (HandlerType != NULL) {
-          return EFI_SUCCESS;
+          //
+          // Won't go to next Handler, and return with EFI_SUCCESS
+          //
+          CanReturn = TRUE;
         }

         SuccessReturn = TRUE;
@@ -211,12 +228,78 @@ MmiManage (
         ASSERT_EFI_ERROR (Status);
         break;
     }
+
+    if (CanReturn) {
+      break;
+    }
   }

   if (SuccessReturn) {
     Status = EFI_SUCCESS;
   }

+  ASSERT (mMmiManageCallingDepth > 0);
+  mMmiManageCallingDepth--;
+
+  //
+  // The MmiHandlerUnRegister won't take effect inside MmiManage.
+  // Before returned from MmiManage, delete the MmiHandler which is
+  // marked as NeedDeleted.
+  // Note that MmiManage can be called recursively.
+  //
+  if (mMmiManageCallingDepth == 0) {
+    //
+    // Go through all MmiHandler in root Mmi handlers
+    //
+    for ( Link = GetFirstNode (&mRootMmiHandlerList)
+          ; !IsNull (&mRootMmiHandlerList, Link);
+          )
+    {
+      MmiHandler = CR (Link, MMI_HANDLER, Link, MMI_HANDLER_SIGNATURE);
+      Link       = GetNextNode (&mRootMmiHandlerList, Link);
+      if (MmiHandler->NeedDeleted) {
+        //
+        // Remove MmiHandler if the NeedDeleted is set.
+        //
+        RemoveEntryList (&MmiHandler->Link);
+        FreePool (MmiHandler);
+      }
+    }
+
+    //
+    // Go through all MmiHandler in non-root MMI handlers
+    //
+    for ( EntryLink = GetFirstNode (&mMmiEntryList)
+          ; !IsNull (&mMmiEntryList, EntryLink);
+          )
+    {
+      MmiEntry  = CR (EntryLink, MMI_ENTRY, AllEntries, MMI_ENTRY_SIGNATURE);
+      EntryLink = GetNextNode (&mMmiEntryList, EntryLink);
+      for ( Link = GetFirstNode (&MmiEntry->MmiHandlers)
+            ; !IsNull (&MmiEntry->MmiHandlers, Link);
+            )
+      {
+        MmiHandler = CR (Link, MMI_HANDLER, Link, MMI_HANDLER_SIGNATURE);
+        Link       = GetNextNode (&MmiEntry->MmiHandlers, Link);
+        if (MmiHandler->NeedDeleted) {
+          //
+          // Remove MmiHandler if the NeedDeleted is set.
+          //
+          RemoveEntryList (&MmiHandler->Link);
+          FreePool (MmiHandler);
+        }
+      }
+
+      if (IsListEmpty (&MmiEntry->MmiHandlers)) {
+        //
+        // No handler registered for this MmiEntry now, remove the MMI_ENTRY
+        //
+        RemoveEntryList (&MmiEntry->AllEntries);
+        FreePool (MmiEntry);
+      }
+    }
+  }
+
   return Status;
 }

@@ -252,8 +335,9 @@ MmiHandlerRegister (
     return EFI_OUT_OF_RESOURCES;
   }

-  MmiHandler->Signature = MMI_HANDLER_SIGNATURE;
-  MmiHandler->Handler   = Handler;
+  MmiHandler->Signature   = MMI_HANDLER_SIGNATURE;
+  MmiHandler->Handler     = Handler;
+  MmiHandler->NeedDeleted = FALSE;

   if (HandlerType == NULL) {
     //
@@ -311,6 +395,16 @@ MmiHandlerUnRegister (

   MmiEntry = MmiHandler->MmiEntry;

+  if (mMmiManageCallingDepth > 0) {
+    //
+    // This function is called from MmiManage()
+    // Do not delete or remove MmiHandler or MmiEntry now.
+    // Set the NeedDeleted field in MmiHandler so that MmiManage will delete it later
+    //
+    MmiHandler->NeedDeleted = TRUE;
+    return EFI_SUCCESS;
+  }
+
   RemoveEntryList (&MmiHandler->Link);
   FreePool (MmiHandler);

--
2.31.1.windows.1



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



[-- Attachment #2: Type: text/html, Size: 15160 bytes --]