[edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore

[edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore

[Xu, Wei6]

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

-- 
2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110177): https://edk2.groups.io/g/devel/message/110177
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]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/2] StandaloneMmPkg: Add Standalone Mm Core platform hook lib.

[Xu, Wei6]

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]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 2/2] StandaloneMmPkg/Core: Consumes Standalone Mm Core Platform Hook Lib.

[Xu, Wei6]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore

[Michael Kubacki]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore

[Xu, Wei6]

[-- 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 --]

Re: [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore

[Michael Kubacki]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore

[Xu, Wei6]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore

[Michael Kubacki]

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]
-=-=-=-=-=-=-=-=-=-=-=-