From: "Ni, Ray" <ray.ni@intel.com>
To: "Liu, Zhiguang" <zhiguang.liu@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: Liming Gao <gaoliming@byosoft.com.cn>,
"Wu, Jiaxin" <jiaxin.wu@intel.com>,
Laszlo Ersek <lersek@redhat.com>,
Ard Biesheuvel <ardb+tianocore@kernel.org>,
Sami Mujawar <sami.mujawar@arm.com>
Subject: Re: [edk2-devel] [PATCH 6/6] StandaloneMmPkg: Support to unregister MMI handler in MMI handlers
Date: Mon, 25 Mar 2024 02:48:16 +0000 [thread overview]
Message-ID: <MN6PR11MB82441E87A43EDF98101AFFE58C362@MN6PR11MB8244.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20240318021956.2787-7-zhiguang.liu@intel.com>
[-- 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 --]
prev parent reply other threads:[~2024-03-25 2:48 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-18 2:19 [edk2-devel] [PATCH 0/6] Support to unregister SMI handler in SMI handlers Zhiguang Liu
2024-03-18 2:19 ` [edk2-devel] [PATCH 1/6] Revert 2ec8f0c6407f062441b205b900038933865c7b3c Zhiguang Liu
2024-03-18 2:19 ` [edk2-devel] [PATCH 2/6] Revert 049ff6c39c73edd3709c05bd0e46184320471358 Zhiguang Liu
2024-03-18 2:19 ` [edk2-devel] [PATCH 3/6] Revert 17b28722008eab745ce186b72cd325944cbe6bf0 Zhiguang Liu
2024-03-18 2:19 ` [edk2-devel] [PATCH 4/6] Revert ae1079b386a597108a8070652bf7cdaa4ec3dda3 Zhiguang Liu
2024-03-18 2:19 ` [edk2-devel] [PATCH 5/6] MdeModulePkg/SMM: Support to unregister SMI handler in SMI handlers Zhiguang Liu
2024-03-18 2:19 ` [edk2-devel] [PATCH 6/6] StandaloneMmPkg: Support to unregister MMI handler in MMI handlers Zhiguang Liu
2024-03-25 2:48 ` Ni, Ray [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MN6PR11MB82441E87A43EDF98101AFFE58C362@MN6PR11MB8244.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox