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 <mikub...@linux.microsoft.com> Sent: Tuesday, October 31, 2023 3:27 AM To: devel@edk2.groups.io; ler...@redhat.com; Xu, Wei6 <wei6...@intel.com> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>; Sami Mujawar <sami.muja...@arm.com>; Ni, Ray <ray...@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+tianoc...@kernel.org> >>> >>> Cc: Sami Mujawar <sami.muja...@arm.com> >>> >>> Cc: Ray Ni <ray...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
