Re: [PATCH v1 1/1] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support

["Michael Kubacki" <mikuback@linux.microsoft.com>]

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@linux.microsoft.com wrote:
>> From: Michael Kubacki <michael.kubacki@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@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@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
>