From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web09.1501.1613121018453841058 for ; Fri, 12 Feb 2021 01:10:18 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=O+0xCCkO; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1613121017; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=GCof+9UfCtTaP0ADD4vQytavZc7yaHLZV6E4hHS/aIo=; b=O+0xCCkOS77kTXusH1nkMuA1qoMsXXn19KBERXtR7tb5+TCbeDBMhSPzAGRtPniwO9VHZ8 vLYQzJPLkNsFBYyqIFVxl3UNfSDmF12Z2EqXA4VkjFRGU8RIdfMAkKXtRkT+O7Togvp0S6 /lY9sZJVoKjc2Xadpm8lueWKLEWC55w= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-534-I_Peve8UNvO4JouVamYLRA-1; Fri, 12 Feb 2021 04:10:12 -0500 X-MC-Unique: I_Peve8UNvO4JouVamYLRA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D1196801965; Fri, 12 Feb 2021 09:10:10 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-18.ams2.redhat.com [10.36.113.18]) by smtp.corp.redhat.com (Postfix) with ESMTP id 614B570946; Fri, 12 Feb 2021 09:10:09 +0000 (UTC) Subject: Re: [PATCH v1 1/1] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support To: mikuback@linux.microsoft.com, devel@edk2.groups.io Cc: Eric Dong , Ray Ni , Rahul Kumar References: <20210212041124.2029-1-mikuback@linux.microsoft.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 12 Feb 2021 10:10:08 +0100 MIME-Version: 1.0 In-Reply-To: <20210212041124.2029-1-mikuback@linux.microsoft.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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