[edk2-devel] [PATCH v1 1/1] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support
Michael Kubacki
mikuback at linux.microsoft.com
Sat Feb 13 01:03:23 UTC 2021
Hi Laszlo,
Thank you for the quick and detailed feedback. This is definitely the
right direction, I suppose someone had to clean it up :).
All of your suggestions are present in v2:
https://edk2.groups.io/g/devel/message/71656
Regards,
Michael
On 2/12/2021 1:10 AM, Laszlo Ersek wrote:
> Hi Michael,
>
> On 02/12/21 05:11, mikuback at linux.microsoft.com wrote:
>> From: Michael Kubacki <michael.kubacki at microsoft.com>
>>
>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3218
>>
>> Adds an INF for StandaloneMmCpuFeaturesLib, which supports building
>> the SmmCpuFeaturesLib code for Standalone MM. Minimal code changes
>> are made to allow reuse of existing code for Standalone MM.
>>
>> The original INF file names are left intact (continue to use SMM
>> terminology) to retain backward compatibility with platforms that
>> use those INFs. Similarly, the C file names are unchanged to be
>> consistent with the INF file names.
>>
>> Cc: Eric Dong <eric.dong at intel.com>
>> Cc: Ray Ni <ray.ni at intel.com>
>> Cc: Laszlo Ersek <lersek at redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar at intel.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki at microsoft.com>
>> ---
>> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 18 +++----
>> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c | 2 +-
>> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c | 10 ++--
>> UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.c | 50 +++++++++++++++++++
>> UefiCpuPkg/Library/SmmCpuFeaturesLib/TraditionalMmCpuFeaturesLib.c | 51 ++++++++++++++++++++
>> UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h | 39 +++++++++++++++
>> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 +-
>> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf | 2 +
>> UefiCpuPkg/Library/SmmCpuFeaturesLib/{SmmCpuFeaturesLib.inf => StandaloneMmCpuFeaturesLib.inf} | 16 +++---
>> UefiCpuPkg/UefiCpuPkg.dsc | 1 +
>> 10 files changed, 169 insertions(+), 24 deletions(-)
>
> I've found this patch surprisingly difficult to review.
>
> The reason for that is two-fold, as much as I can determine.
>
> (1) This patch should be split in two parts, namely:
>
> (1a) interface extraction
>
> (1b) adding the new library instance
>
>
> (2) *However*, the real problem is that the pre-patch status is a mess
> already. Consider the "SmmCpuFeaturesLibStm.inf" instance:
>
> - this instance has a constructor function called
> SmmCpuFeaturesLibStmConstructor()
>
> - the "SmmStm.c" file declares the *other* instance's constructor
> function, and does so without a header file
>
> - the STM instance's constructor calls the other instance's constructor
> like a normal function.
>
>
> I would say that the original commit that introduced the STM instance
> *like this* created technical debt.
>
> Regardless of whether someone agrees with me on that or not, *after*
> your patch, we'll have a totally awkward situation where *some*
> polymorphism is implemented correctly -- namely, the mechanism
> introduced by your patch, with two constructors calling a common helper
> function --, while at the *same time*, some *other* polymorphism is
> implemented incorrectly, or at least differently -- with a constructor
> calling another instance's constructor.
>
> It gets worse. There is *already* (i.e., pre-patch) a multi-instance
> internal function, namely FinishSmmCpuFeaturesInitializeProcessor(). But
> that function is not declared in any header file; the declaration is
> embedded in "SmmCpuFeaturesLib.c"
>
> All this is completely mind-boggling, and the main reason why this patch
> is so difficult to review. For me anyway.
>
> So here's what I suggest / request. In total, we're going to need *four*
> patches.
>
>
> (3) In the first patch, please start untangling the current (pre-patch)
> mess. Namely:
>
> (3a) Introduce the new header file ("CpuFeaturesLib.h"), but with *only*
> the FinishSmmCpuFeaturesInitializeProcessor() declaration.
>
> (3b) Remove the (embedded) declaration from its current spot.
>
> (3c) Consume the new header file in *three* C files that define, or
> call, FinishSmmCpuFeaturesInitializeProcessor().
>
> (3d) Extend the INF files with the new header file.
>
>
> (4) In the second patch, continue untangling the current mess:
>
> (4a) declare the CpuFeaturesLibInitialization() function in the header
> file added in (3a)
>
> (4b) Rename the SmmCpuFeaturesLibConstructor() function in
> "SmmCpuFeaturesLib.c" to CpuFeaturesLibInitialization()
>
> (4c) In "SmmStm.c", remove the declaration of SmmCpuFeaturesLibConstructor()
>
> (4d) In "SmmStm.c", call CpuFeaturesLibInitialization() rather than
> SmmCpuFeaturesLibConstructor()
>
> (4e) Re-introduce the SmmCpuFeaturesLibConstructor() function, but to
> the file "SmmCpuFeaturesLibNoStm.c". The implementation should be just a
> call to CpuFeaturesLibInitialization().
>
> This step establishes the *good* pattern, from your current patch, for
> the *existent* status.
>
>
> (5) In the third patch:
>
> (5a) declare the GetCpuMaxLogicalProcessorNumber() function in
> "CpuFeaturesLib.h"
>
> (5b) call GetCpuMaxLogicalProcessorNumber() rather than PcdGet32() in
> CpuFeaturesLibInitialization(), in "SmmCpuFeaturesLib.c"
>
> (5c) Introduce the "TraditionalMmCpuFeaturesLib.c" file, containing the
> GetCpuMaxLogicalProcessorNumber() implementation only -- calling PcdGet32()
>
> (5d) add the new C file to both INF files
>
> (5e) This patch should not rename any files, should not rename any
> functions, and should not change any <PiSmm.h> include directives.
>
>
> (6) In the fourth patch:
>
> (6a) introduce the files
>
> StandaloneMmCpuFeaturesLib.c
> StandaloneMmCpuFeaturesLib.inf
>
> like they are in the present patch.
>
> (6b) extend the DSC file with the new library instance
>
> (6c) modify the <PiSmm.h> #include directives wherever necessary, but
> *only* in those files where the update is really necessary.
>
> Thanks
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71661): https://edk2.groups.io/g/devel/message/71661
Mute This Topic: https://groups.io/mt/80576529/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list