From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web10.1014.1613178203383338028 for ; Fri, 12 Feb 2021 17:03:23 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=T29SsiJR; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [10.124.238.202] (unknown [131.107.174.202]) by linux.microsoft.com (Postfix) with ESMTPSA id EF3B920B6C40; Fri, 12 Feb 2021 17:03:22 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com EF3B920B6C40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1613178203; bh=/H8R6kSwdjxdf0yRsyYx5/aYs9dwICRZNLf3c1LRBrM=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=T29SsiJR2H+14YmchmpcNW7O8tnQYkrFAhr5kTiO/7Hi/lWP3DUPJyiZHa0uTAd9l kTumTFyXqsmVcvqKiKXl5Oey7TrriLFD7hvjpGqfr2n9auIzexT3FMfagGvaddQIFo 7noRnOF4ZdTqrki6N0SFR6JKyd3stG1gHbAllv2g= Subject: Re: [PATCH v1 1/1] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support To: Laszlo Ersek , devel@edk2.groups.io Cc: Eric Dong , Ray Ni , Rahul Kumar References: <20210212041124.2029-1-mikuback@linux.microsoft.com> From: "Michael Kubacki" Message-ID: <8f2602d0-7d17-54d1-5cb7-7336d5b1e1c7@linux.microsoft.com> Date: Fri, 12 Feb 2021 17:03:23 -0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> >> 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 >> Cc: Ray Ni >> Cc: Laszlo Ersek >> Cc: Rahul Kumar >> Signed-off-by: Michael Kubacki >> --- >> 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 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 #include directives wherever necessary, but > *only* in those files where the update is really necessary. > > Thanks > Laszlo >