* [edk2-devel] [PATCH 1/2] StandaloneMmPkg: Add Standalone Mm Core platform hook lib.
2023-10-27 3:28 [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore Xu, Wei6
@ 2023-10-27 3:28 ` Xu, Wei6
2023-10-27 3:28 ` [edk2-devel] [PATCH 2/2] StandaloneMmPkg/Core: Consumes Standalone Mm Core Platform Hook Lib Xu, Wei6
2023-10-27 16:08 ` [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore Michael Kubacki
2 siblings, 0 replies; 9+ messages in thread
From: Xu, Wei6 @ 2023-10-27 3:28 UTC (permalink / raw)
To: devel; +Cc: Wei6 Xu, Ard Biesheuvel, Sami Mujawar, Ray Ni
Add header file for the definition of StandaloneMmCorePlatformHookLib.
Add NULL instance for StandaloneMmCorePlatformHookLib.
This library class defines a set of platform hooks called by the
Standalone Mm Core. With this library, platform can perform specific
tasks before and after invoking registered MMI handlers.
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Wei6 Xu <wei6.xu@intel.com>
---
.../StandaloneMmCorePlatformHookLibNull.c | 45 +++++++++++++++++++
.../Library/StandaloneMmCorePlatformHookLib.h | 44 ++++++++++++++++++
.../StandaloneMmCorePlatformHookLibNull.inf | 30 +++++++++++++
StandaloneMmPkg/StandaloneMmPkg.dec | 4 ++
StandaloneMmPkg/StandaloneMmPkg.dsc | 2 +
5 files changed, 125 insertions(+)
create mode 100644 StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/StandaloneMmCorePlatformHookLibNull.c
create mode 100644 StandaloneMmPkg/Include/Library/StandaloneMmCorePlatformHookLib.h
create mode 100644 StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/StandaloneMmCorePlatformHookLibNull.inf
diff --git a/StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/StandaloneMmCorePlatformHookLibNull.c b/StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/StandaloneMmCorePlatformHookLibNull.c
new file mode 100644
index 000000000000..dfd781e8c947
--- /dev/null
+++ b/StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/StandaloneMmCorePlatformHookLibNull.c
@@ -0,0 +1,45 @@
+/** @file
+ NULL instance of StandaloneMmCorePlatformHookLib.
+
+ Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Library/StandaloneMmCorePlatformHookLib.h>
+
+/**
+ Performs platform specific tasks before invoking registered SMI handlers.
+
+ This function performs platform specific tasks before invoking registered SMI handlers.
+
+ @retval EFI_SUCCESS The platform hook completes successfully.
+ @retval Other values The platform hook cannot complete due to some error.
+
+**/
+EFI_STATUS
+EFIAPI
+PlatformHookBeforeMmDispatch (
+ VOID
+ )
+{
+ return EFI_SUCCESS;
+}
+
+/**
+ Performs platform specific tasks after invoking registered SMI handlers.
+
+ This function performs platform specific tasks after invoking registered SMI handlers.
+
+ @retval EFI_SUCCESS The platform hook completes successfully.
+ @retval Other values The platform hook cannot complete due to some error.
+
+**/
+EFI_STATUS
+EFIAPI
+PlatformHookAfterMmDispatch (
+ VOID
+ )
+{
+ return EFI_SUCCESS;
+}
diff --git a/StandaloneMmPkg/Include/Library/StandaloneMmCorePlatformHookLib.h b/StandaloneMmPkg/Include/Library/StandaloneMmCorePlatformHookLib.h
new file mode 100644
index 000000000000..37eaa4e8946c
--- /dev/null
+++ b/StandaloneMmPkg/Include/Library/StandaloneMmCorePlatformHookLib.h
@@ -0,0 +1,44 @@
+/** @file
+ Standalone Mm Core Platform Hook Library. This library class defines a set of platform
+ hooks called by the Standalone Mm Core.
+
+ Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef STANDALONE_MM_CORE_PLATFORM_HOOK_LIB_
+#define STANDALONE_MM_CORE_PLATFORM_HOOK_LIB_
+
+/**
+ Performs platform specific tasks before invoking registered MMI handlers.
+
+ This function performs platform specific tasks before invoking registered MMI handlers.
+
+ @retval EFI_SUCCESS The platform hook completes successfully.
+ @retval Other values The platform hook cannot complete due to some error.
+
+**/
+EFI_STATUS
+EFIAPI
+PlatformHookBeforeMmDispatch (
+ VOID
+ );
+
+/**
+ Performs platform specific tasks after invoking registered MMI handlers.
+
+ This function performs platform specific tasks after invoking registered MMI handlers.
+
+ @retval EFI_SUCCESS The platform hook completes successfully.
+ @retval Other values The platform hook cannot complete due to some error.
+
+**/
+EFI_STATUS
+EFIAPI
+PlatformHookAfterMmDispatch (
+ VOID
+ );
+
+#endif
diff --git a/StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/StandaloneMmCorePlatformHookLibNull.inf b/StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/StandaloneMmCorePlatformHookLibNull.inf
new file mode 100644
index 000000000000..917f2983c938
--- /dev/null
+++ b/StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/StandaloneMmCorePlatformHookLibNull.inf
@@ -0,0 +1,30 @@
+## @file
+# Standalone MM Core Platform Hook NULL Library instance.
+#
+# Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = StandaloneMmCorePlatformHookLibNull
+ FILE_GUID = A5924660-072B-4188-A850-C45FBED9FCE0
+ MODULE_TYPE = MM_CORE_STANDALONE
+ VERSION_STRING = 1.0
+ PI_SPECIFICATION_VERSION = 0x00010032
+ LIBRARY_CLASS = StandaloneMmCorePlatformHookLib|MM_CORE_STANDALONE
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+# VALID_ARCHITECTURES = IA32 X64
+#
+
+[Sources]
+ StandaloneMmCorePlatformHookLibNull.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ StandaloneMmPkg/StandaloneMmPkg.dec
diff --git a/StandaloneMmPkg/StandaloneMmPkg.dec b/StandaloneMmPkg/StandaloneMmPkg.dec
index 46784d94e421..1e847ebd0635 100644
--- a/StandaloneMmPkg/StandaloneMmPkg.dec
+++ b/StandaloneMmPkg/StandaloneMmPkg.dec
@@ -29,6 +29,10 @@ [LibraryClasses]
## MM Memory Operation.
MemLib|Include/Library/StandaloneMmMemLib.h
+ ## @libraryclass Defines a set of platform hooks called by the Standalone
+ ## Mm Core.
+ StandaloneMmCorePlatformHookLib|Include/Library/StandaloneMmCorePlatformHookLib.h
+
[LibraryClasses.AArch64, LibraryClasses.ARM]
## @libraryclass Defines a set of interfaces for the MM core entrypoint for
## AArch64 and ARM.
diff --git a/StandaloneMmPkg/StandaloneMmPkg.dsc b/StandaloneMmPkg/StandaloneMmPkg.dsc
index 8012f93b7dcc..bfa1760f8e6f 100644
--- a/StandaloneMmPkg/StandaloneMmPkg.dsc
+++ b/StandaloneMmPkg/StandaloneMmPkg.dsc
@@ -72,6 +72,7 @@ [LibraryClasses.AARCH64, LibraryClasses.ARM]
[LibraryClasses.common.MM_CORE_STANDALONE]
HobLib|StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf
+ StandaloneMmCorePlatformHookLib|StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/StandaloneMmCorePlatformHookLibNull.inf
[LibraryClasses.common.MM_STANDALONE]
MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.inf
@@ -117,6 +118,7 @@ [Components.common]
StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf
StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.inf
StandaloneMmPkg/Library/VariableMmDependency/VariableMmDependency.inf
+ StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/StandaloneMmCorePlatformHookLibNull.inf
[Components.AARCH64, Components.ARM]
StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf
--
2.29.2.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110178): https://edk2.groups.io/g/devel/message/110178
Mute This Topic: https://groups.io/mt/102214567/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 9+ messages in thread* [edk2-devel] [PATCH 2/2] StandaloneMmPkg/Core: Consumes Standalone Mm Core Platform Hook Lib.
2023-10-27 3:28 [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore Xu, Wei6
2023-10-27 3:28 ` [edk2-devel] [PATCH 1/2] StandaloneMmPkg: Add Standalone Mm Core platform hook lib Xu, Wei6
@ 2023-10-27 3:28 ` Xu, Wei6
2023-10-27 16:08 ` [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore Michael Kubacki
2 siblings, 0 replies; 9+ messages in thread
From: Xu, Wei6 @ 2023-10-27 3:28 UTC (permalink / raw)
To: devel; +Cc: Wei6 Xu, Ard Biesheuvel, Sami Mujawar, Ray Ni
Call platform hooks before and after invoking registered MMI handlers.
Platform can perform specific tasks in the hooks.
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Wei6 Xu <wei6.xu@intel.com>
---
StandaloneMmPkg/Core/StandaloneMmCore.c | 7 ++++++-
StandaloneMmPkg/Core/StandaloneMmCore.h | 1 +
StandaloneMmPkg/Core/StandaloneMmCore.inf | 1 +
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.c b/StandaloneMmPkg/Core/StandaloneMmCore.c
index d221f1d1115d..43d8694fa9ef 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.c
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.c
@@ -351,7 +351,7 @@ MmEntryPoint (
//
// Call platform hook before Mm Dispatch
//
- // PlatformHookBeforeMmDispatch ();
+ PlatformHookBeforeMmDispatch ();
//
// If a legacy boot has occurred, then make sure gMmCorePrivate is not accessed
@@ -400,6 +400,11 @@ MmEntryPoint (
//
MmiManage (NULL, NULL, NULL, NULL);
+ //
+ // Call platform hook after Mm Dispatch
+ //
+ PlatformHookAfterMmDispatch ();
+
//
// TBD: Do not use private data structure ?
//
diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.h b/StandaloneMmPkg/Core/StandaloneMmCore.h
index 822d95358c39..08ea178bfe76 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.h
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.h
@@ -40,6 +40,7 @@
#include <Library/ReportStatusCodeLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/PcdLib.h>
+#include <Library/StandaloneMmCorePlatformHookLib.h>
#include <Library/StandaloneMmMemLib.h>
#include <Library/HobLib.h>
diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf b/StandaloneMmPkg/Core/StandaloneMmCore.inf
index c44b9ff33303..ac2f07610d04 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
@@ -52,6 +52,7 @@ [LibraryClasses]
PeCoffLib
ReportStatusCodeLib
StandaloneMmCoreEntryPoint
+ StandaloneMmCorePlatformHookLib
[Protocols]
gEfiDxeMmReadyToLockProtocolGuid ## UNDEFINED # SmiHandlerRegister
--
2.29.2.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110179): https://edk2.groups.io/g/devel/message/110179
Mute This Topic: https://groups.io/mt/102214570/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore
2023-10-27 3:28 [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore Xu, Wei6
2023-10-27 3:28 ` [edk2-devel] [PATCH 1/2] StandaloneMmPkg: Add Standalone Mm Core platform hook lib Xu, Wei6
2023-10-27 3:28 ` [edk2-devel] [PATCH 2/2] StandaloneMmPkg/Core: Consumes Standalone Mm Core Platform Hook Lib Xu, Wei6
@ 2023-10-27 16:08 ` Michael Kubacki
2023-10-28 14:03 ` Laszlo Ersek
2 siblings, 1 reply; 9+ messages in thread
From: Michael Kubacki @ 2023-10-27 16:08 UTC (permalink / raw)
To: devel, wei6.xu; +Cc: Ard Biesheuvel, Sami Mujawar, Ray Ni
This allows ambiguous "platform" code in the critical path of the MM
core. Is this necessary?
Do you need this for one feature that others might too and can be
abstracted? Or, do you plan to perform an unknown and arbitrary number
of changes behind the hook over time?
Thanks,
Michael
On 10/26/2023 11:28 PM, Xu, Wei6 wrote:
> This patch set is to add StandaloneMmCorePlatformHookLib into StandaloneMmCore.
>
> This library class defines a set of platform hooks called by the Standalone Mm Core. With this library, platform can perform specific tasks before and after invoking registered MMI handlers.
> We need this library to implement our feature.
>
> PR: https://github.com/tianocore/edk2/pull/4949
>
>
>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
>
> Cc: Ray Ni <ray.ni@intel.com>
>
>
> Wei6 Xu (2):
> StandaloneMmPkg: Add Standalone Mm Core platform hook lib.
> StandaloneMmPkg/Core: Consumes Standalone Mm Core Platform Hook Lib.
>
> StandaloneMmPkg/Core/StandaloneMmCore.c | 7 ++-
> .../StandaloneMmCorePlatformHookLibNull.c | 45 +++++++++++++++++++
> StandaloneMmPkg/Core/StandaloneMmCore.h | 1 +
> StandaloneMmPkg/Core/StandaloneMmCore.inf | 1 +
> .../Library/StandaloneMmCorePlatformHookLib.h | 44 ++++++++++++++++++
> .../StandaloneMmCorePlatformHookLibNull.inf | 30 +++++++++++++
> StandaloneMmPkg/StandaloneMmPkg.dec | 4 ++
> StandaloneMmPkg/StandaloneMmPkg.dsc | 2 +
> 8 files changed, 133 insertions(+), 1 deletion(-)
> create mode 100644 StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/StandaloneMmCorePlatformHookLibNull.c
> create mode 100644 StandaloneMmPkg/Include/Library/StandaloneMmCorePlatformHookLib.h
> create mode 100644 StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/StandaloneMmCorePlatformHookLibNull.inf
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110220): https://edk2.groups.io/g/devel/message/110220
Mute This Topic: https://groups.io/mt/102214566/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore
2023-10-27 16:08 ` [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore Michael Kubacki
@ 2023-10-28 14:03 ` Laszlo Ersek
2023-10-30 0:52 ` Xu, Wei6
2023-10-30 19:27 ` Michael Kubacki
0 siblings, 2 replies; 9+ messages in thread
From: Laszlo Ersek @ 2023-10-28 14:03 UTC (permalink / raw)
To: devel, mikuback, wei6.xu; +Cc: Ard Biesheuvel, Sami Mujawar, Ray Ni
On 10/27/23 18:08, Michael Kubacki wrote:
> This allows ambiguous "platform" code in the critical path of the MM
> core. Is this necessary?
>
> Do you need this for one feature that others might too and can be
> abstracted? Or, do you plan to perform an unknown and arbitrary number
> of changes behind the hook over time?
Not sure if it's necessary, but it's somewhat "customary". Platform hook
libs are not uncommon; for example PiSmmCpuDxeSmm consumes
SmmCpuPlatformHookLib and SmmCpuFeaturesLib.
My request would be for Wei to file a TianoCore Feature Request
bugzilla, with a bit more information than "We need this library to
implement our feature". Reference the BZ in the commit messages, then
add the BZ to the
<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning>.
Laszlo
>
> Thanks,
> Michael
>
> On 10/26/2023 11:28 PM, Xu, Wei6 wrote:
>> This patch set is to add StandaloneMmCorePlatformHookLib into
>> StandaloneMmCore.
>>
>> This library class defines a set of platform hooks called by the
>> Standalone Mm Core. With this library, platform can perform specific
>> tasks before and after invoking registered MMI handlers.
>> We need this library to implement our feature.
>>
>> PR: https://github.com/tianocore/edk2/pull/4949
>>
>>
>>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>
>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>>
>> Cc: Ray Ni <ray.ni@intel.com>
>>
>>
>> Wei6 Xu (2):
>> StandaloneMmPkg: Add Standalone Mm Core platform hook lib.
>> StandaloneMmPkg/Core: Consumes Standalone Mm Core Platform Hook Lib.
>>
>> StandaloneMmPkg/Core/StandaloneMmCore.c | 7 ++-
>> .../StandaloneMmCorePlatformHookLibNull.c | 45 +++++++++++++++++++
>> StandaloneMmPkg/Core/StandaloneMmCore.h | 1 +
>> StandaloneMmPkg/Core/StandaloneMmCore.inf | 1 +
>> .../Library/StandaloneMmCorePlatformHookLib.h | 44 ++++++++++++++++++
>> .../StandaloneMmCorePlatformHookLibNull.inf | 30 +++++++++++++
>> StandaloneMmPkg/StandaloneMmPkg.dec | 4 ++
>> StandaloneMmPkg/StandaloneMmPkg.dsc | 2 +
>> 8 files changed, 133 insertions(+), 1 deletion(-)
>> create mode 100644
>> StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/StandaloneMmCorePlatformHookLibNull.c
>> create mode 100644
>> StandaloneMmPkg/Include/Library/StandaloneMmCorePlatformHookLib.h
>> create mode 100644
>> StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/StandaloneMmCorePlatformHookLibNull.inf
>>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110246): https://edk2.groups.io/g/devel/message/110246
Mute This Topic: https://groups.io/mt/102214566/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore
2023-10-28 14:03 ` Laszlo Ersek
@ 2023-10-30 0:52 ` Xu, Wei6
2023-10-30 19:27 ` Michael Kubacki
1 sibling, 0 replies; 9+ messages in thread
From: Xu, Wei6 @ 2023-10-30 0:52 UTC (permalink / raw)
To: Laszlo Ersek, devel
[-- Attachment #1: Type: text/plain, Size: 558 bytes --]
Thanks Laszlo and Michael. As I synced with Ray, the platform hook is un necessary, we can implement in another way. So, let's drop this patch.
Thanks again for the kind review.
BR,
Wei
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110280): https://edk2.groups.io/g/devel/message/110280
Mute This Topic: https://groups.io/mt/102214566/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: 1202 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore
2023-10-28 14:03 ` Laszlo Ersek
2023-10-30 0:52 ` Xu, Wei6
@ 2023-10-30 19:27 ` Michael Kubacki
2023-10-31 1:18 ` Xu, Wei6
1 sibling, 1 reply; 9+ messages in thread
From: Michael Kubacki @ 2023-10-30 19:27 UTC (permalink / raw)
To: devel, lersek, wei6.xu; +Cc: Ard Biesheuvel, Sami Mujawar, Ray Ni
On 10/28/2023 10:03 AM, Laszlo Ersek wrote:
> On 10/27/23 18:08, Michael Kubacki wrote:
>> This allows ambiguous "platform" code in the critical path of the MM
>> core. Is this necessary?
>>
>> Do you need this for one feature that others might too and can be
>> abstracted? Or, do you plan to perform an unknown and arbitrary number
>> of changes behind the hook over time?
>
> Not sure if it's necessary, but it's somewhat "customary". Platform hook
> libs are not uncommon; for example PiSmmCpuDxeSmm consumes
> SmmCpuPlatformHookLib and SmmCpuFeaturesLib.
>
> My request would be for Wei to file a TianoCore Feature Request
> bugzilla, with a bit more information than "We need this library to
> implement our feature". Reference the BZ in the commit messages, then
> add the BZ to the
> <https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning>.
>
That would be helpful. Hooks are sometimes necessary of course, but I
generally consider these open-ended hook libraries a design wart. They
introduce an inherently incohesive interface and a convenient place for
new APIs to land instead of more focused interfaces that may be more
appropriate. This extends to the implementation where, over time, it can
become a mess of dependencies and unrelated code coupled together.
I'd like to see if there's a way to avoid that or such a library
interface is the best choice.
> Laszlo
>
>>
>> Thanks,
>> Michael
>>
>> On 10/26/2023 11:28 PM, Xu, Wei6 wrote:
>>> This patch set is to add StandaloneMmCorePlatformHookLib into
>>> StandaloneMmCore.
>>>
>>> This library class defines a set of platform hooks called by the
>>> Standalone Mm Core. With this library, platform can perform specific
>>> tasks before and after invoking registered MMI handlers.
>>> We need this library to implement our feature.
>>>
>>> PR: https://github.com/tianocore/edk2/pull/4949
>>>
>>>
>>>
>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>>
>>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>>>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>>
>>>
>>> Wei6 Xu (2):
>>> StandaloneMmPkg: Add Standalone Mm Core platform hook lib.
>>> StandaloneMmPkg/Core: Consumes Standalone Mm Core Platform Hook Lib.
>>>
>>> StandaloneMmPkg/Core/StandaloneMmCore.c | 7 ++-
>>> .../StandaloneMmCorePlatformHookLibNull.c | 45 +++++++++++++++++++
>>> StandaloneMmPkg/Core/StandaloneMmCore.h | 1 +
>>> StandaloneMmPkg/Core/StandaloneMmCore.inf | 1 +
>>> .../Library/StandaloneMmCorePlatformHookLib.h | 44 ++++++++++++++++++
>>> .../StandaloneMmCorePlatformHookLibNull.inf | 30 +++++++++++++
>>> StandaloneMmPkg/StandaloneMmPkg.dec | 4 ++
>>> StandaloneMmPkg/StandaloneMmPkg.dsc | 2 +
>>> 8 files changed, 133 insertions(+), 1 deletion(-)
>>> create mode 100644
>>> StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/StandaloneMmCorePlatformHookLibNull.c
>>> create mode 100644
>>> StandaloneMmPkg/Include/Library/StandaloneMmCorePlatformHookLib.h
>>> create mode 100644
>>> StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/StandaloneMmCorePlatformHookLibNull.inf
>>>
>>
>>
>>
>>
>>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110336): https://edk2.groups.io/g/devel/message/110336
Mute This Topic: https://groups.io/mt/102214566/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore
2023-10-30 19:27 ` Michael Kubacki
@ 2023-10-31 1:18 ` Xu, Wei6
2023-10-31 1:40 ` Michael Kubacki
0 siblings, 1 reply; 9+ messages in thread
From: Xu, Wei6 @ 2023-10-31 1:18 UTC (permalink / raw)
To: Michael Kubacki, devel@edk2.groups.io, lersek@redhat.com
Cc: Ard Biesheuvel, Sami Mujawar, Ni, Ray
Hi Michael and Laszlo,
Thanks a lot for the review.
We reconsidered the solution and find better way to implement the 'platform hook'. So Let's drop this patch.
Thanks Ray for suggesting a better solution.
BR,
Wei
-----Original Message-----
From: Michael Kubacki <mikuback@linux.microsoft.com>
Sent: Tuesday, October 31, 2023 3:27 AM
To: devel@edk2.groups.io; lersek@redhat.com; Xu, Wei6 <wei6.xu@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore
On 10/28/2023 10:03 AM, Laszlo Ersek wrote:
> On 10/27/23 18:08, Michael Kubacki wrote:
>> This allows ambiguous "platform" code in the critical path of the MM
>> core. Is this necessary?
>>
>> Do you need this for one feature that others might too and can be
>> abstracted? Or, do you plan to perform an unknown and arbitrary
>> number of changes behind the hook over time?
>
> Not sure if it's necessary, but it's somewhat "customary". Platform
> hook libs are not uncommon; for example PiSmmCpuDxeSmm consumes
> SmmCpuPlatformHookLib and SmmCpuFeaturesLib.
>
> My request would be for Wei to file a TianoCore Feature Request
> bugzilla, with a bit more information than "We need this library to
> implement our feature". Reference the BZ in the commit messages, then
> add the BZ to the
> <https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning>.
>
That would be helpful. Hooks are sometimes necessary of course, but I generally consider these open-ended hook libraries a design wart. They introduce an inherently incohesive interface and a convenient place for new APIs to land instead of more focused interfaces that may be more appropriate. This extends to the implementation where, over time, it can become a mess of dependencies and unrelated code coupled together.
I'd like to see if there's a way to avoid that or such a library interface is the best choice.
> Laszlo
>
>>
>> Thanks,
>> Michael
>>
>> On 10/26/2023 11:28 PM, Xu, Wei6 wrote:
>>> This patch set is to add StandaloneMmCorePlatformHookLib into
>>> StandaloneMmCore.
>>>
>>> This library class defines a set of platform hooks called by the
>>> Standalone Mm Core. With this library, platform can perform specific
>>> tasks before and after invoking registered MMI handlers.
>>> We need this library to implement our feature.
>>>
>>> PR: https://github.com/tianocore/edk2/pull/4949
>>>
>>>
>>>
>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>>
>>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>>>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>>
>>>
>>> Wei6 Xu (2):
>>> StandaloneMmPkg: Add Standalone Mm Core platform hook lib.
>>> StandaloneMmPkg/Core: Consumes Standalone Mm Core Platform Hook Lib.
>>>
>>> StandaloneMmPkg/Core/StandaloneMmCore.c | 7 ++-
>>> .../StandaloneMmCorePlatformHookLibNull.c | 45
>>> +++++++++++++++++++
>>> StandaloneMmPkg/Core/StandaloneMmCore.h | 1 +
>>> StandaloneMmPkg/Core/StandaloneMmCore.inf | 1 +
>>> .../Library/StandaloneMmCorePlatformHookLib.h | 44
>>> ++++++++++++++++++
>>> .../StandaloneMmCorePlatformHookLibNull.inf | 30 +++++++++++++
>>> StandaloneMmPkg/StandaloneMmPkg.dec | 4 ++
>>> StandaloneMmPkg/StandaloneMmPkg.dsc | 2 +
>>> 8 files changed, 133 insertions(+), 1 deletion(-)
>>> create mode 100644
>>> StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/Standalo
>>> neMmCorePlatformHookLibNull.c
>>> create mode 100644
>>> StandaloneMmPkg/Include/Library/StandaloneMmCorePlatformHookLib.h
>>> create mode 100644
>>> StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/Standalo
>>> neMmCorePlatformHookLibNull.inf
>>>
>>
>>
>>
>>
>>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110351): https://edk2.groups.io/g/devel/message/110351
Mute This Topic: https://groups.io/mt/102214566/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore
2023-10-31 1:18 ` Xu, Wei6
@ 2023-10-31 1:40 ` Michael Kubacki
0 siblings, 0 replies; 9+ messages in thread
From: Michael Kubacki @ 2023-10-31 1:40 UTC (permalink / raw)
To: devel, wei6.xu, lersek@redhat.com; +Cc: Ard Biesheuvel, Sami Mujawar, Ni, Ray
Thank you for the update.
On 10/30/2023 9:18 PM, Xu, Wei6 wrote:
> Hi Michael and Laszlo,
>
> Thanks a lot for the review.
> We reconsidered the solution and find better way to implement the 'platform hook'. So Let's drop this patch.
> Thanks Ray for suggesting a better solution.
>
> BR,
> Wei
>
> -----Original Message-----
> From: Michael Kubacki <mikuback@linux.microsoft.com>
> Sent: Tuesday, October 31, 2023 3:27 AM
> To: devel@edk2.groups.io; lersek@redhat.com; Xu, Wei6 <wei6.xu@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore
>
> On 10/28/2023 10:03 AM, Laszlo Ersek wrote:
>> On 10/27/23 18:08, Michael Kubacki wrote:
>>> This allows ambiguous "platform" code in the critical path of the MM
>>> core. Is this necessary?
>>>
>>> Do you need this for one feature that others might too and can be
>>> abstracted? Or, do you plan to perform an unknown and arbitrary
>>> number of changes behind the hook over time?
>>
>> Not sure if it's necessary, but it's somewhat "customary". Platform
>> hook libs are not uncommon; for example PiSmmCpuDxeSmm consumes
>> SmmCpuPlatformHookLib and SmmCpuFeaturesLib.
>>
>> My request would be for Wei to file a TianoCore Feature Request
>> bugzilla, with a bit more information than "We need this library to
>> implement our feature". Reference the BZ in the commit messages, then
>> add the BZ to the
>> <https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning>.
>>
> That would be helpful. Hooks are sometimes necessary of course, but I generally consider these open-ended hook libraries a design wart. They introduce an inherently incohesive interface and a convenient place for new APIs to land instead of more focused interfaces that may be more appropriate. This extends to the implementation where, over time, it can become a mess of dependencies and unrelated code coupled together.
>
> I'd like to see if there's a way to avoid that or such a library interface is the best choice.
>
>> Laszlo
>>
>>>
>>> Thanks,
>>> Michael
>>>
>>> On 10/26/2023 11:28 PM, Xu, Wei6 wrote:
>>>> This patch set is to add StandaloneMmCorePlatformHookLib into
>>>> StandaloneMmCore.
>>>>
>>>> This library class defines a set of platform hooks called by the
>>>> Standalone Mm Core. With this library, platform can perform specific
>>>> tasks before and after invoking registered MMI handlers.
>>>> We need this library to implement our feature.
>>>>
>>>> PR: https://github.com/tianocore/edk2/pull/4949
>>>>
>>>>
>>>>
>>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>>>
>>>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>>>>
>>>> Cc: Ray Ni <ray.ni@intel.com>
>>>>
>>>>
>>>> Wei6 Xu (2):
>>>> StandaloneMmPkg: Add Standalone Mm Core platform hook lib.
>>>> StandaloneMmPkg/Core: Consumes Standalone Mm Core Platform Hook Lib.
>>>>
>>>> StandaloneMmPkg/Core/StandaloneMmCore.c | 7 ++-
>>>> .../StandaloneMmCorePlatformHookLibNull.c | 45
>>>> +++++++++++++++++++
>>>> StandaloneMmPkg/Core/StandaloneMmCore.h | 1 +
>>>> StandaloneMmPkg/Core/StandaloneMmCore.inf | 1 +
>>>> .../Library/StandaloneMmCorePlatformHookLib.h | 44
>>>> ++++++++++++++++++
>>>> .../StandaloneMmCorePlatformHookLibNull.inf | 30 +++++++++++++
>>>> StandaloneMmPkg/StandaloneMmPkg.dec | 4 ++
>>>> StandaloneMmPkg/StandaloneMmPkg.dsc | 2 +
>>>> 8 files changed, 133 insertions(+), 1 deletion(-)
>>>> create mode 100644
>>>> StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/Standalo
>>>> neMmCorePlatformHookLibNull.c
>>>> create mode 100644
>>>> StandaloneMmPkg/Include/Library/StandaloneMmCorePlatformHookLib.h
>>>> create mode 100644
>>>> StandaloneMmPkg/Library/StandaloneMmCorePlatformHookLibNull/Standalo
>>>> neMmCorePlatformHookLibNull.inf
>>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>>
>>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110353): https://edk2.groups.io/g/devel/message/110353
Mute This Topic: https://groups.io/mt/102214566/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread