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.37378.1613421208452595099 for ; Mon, 15 Feb 2021 12:33:28 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=SbY4xa6l; 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=1613421207; 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=sRFyt3bg5IdIDZVVNkQfBCKxwkXRNcBnRtVF3vU9SqI=; b=SbY4xa6lA/x1E8U9w2qJhpMzQwT1VBaH65nMbcBXl3/gIhcUs8/qn972sRMIq4o0o44hZ8 zOBAoetVkiJKwujDh4VwnTc64jiFB+A51+iO+IE7jCe5E+XZjxQX9oDxYUzeWIsu5CGINc 92sNOTMXDIEvVMtIDFY5piZWe887zEk= 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-176-hEEJJwIyO-WsFsBiHY3sow-1; Mon, 15 Feb 2021 15:33:24 -0500 X-MC-Unique: hEEJJwIyO-WsFsBiHY3sow-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id BFBE6107ACC7; Mon, 15 Feb 2021 20:33:22 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-65.ams2.redhat.com [10.36.112.65]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5B23E2BFEC; Mon, 15 Feb 2021 20:33:21 +0000 (UTC) Subject: Re: [PATCH v2 4/4] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support To: mikuback@linux.microsoft.com, devel@edk2.groups.io Cc: Eric Dong , Ray Ni , Rahul Kumar References: <20210213005806.2927-1-mikuback@linux.microsoft.com> <20210213005806.2927-5-mikuback@linux.microsoft.com> From: "Laszlo Ersek" Message-ID: <96e8e185-ce9e-91a4-0ce4-f98b697b48c1@redhat.com> Date: Mon, 15 Feb 2021 21:33:20 +0100 MIME-Version: 1.0 In-Reply-To: <20210213005806.2927-5-mikuback@linux.microsoft.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 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 On 02/13/21 01:58, 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 pre-existing 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 | 2 +- > UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c | 2 +- > UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c | 2 +- > UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.c | 51 ++++++++++++++++++++ > UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf | 38 +++++++++++++++ > UefiCpuPkg/UefiCpuPkg.dsc | 1 + > 6 files changed, 93 insertions(+), 3 deletions(-) > > diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > index a494988898b8..990fdb098865 100644 > --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > @@ -7,7 +7,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > > -#include > +#include > #include > #include > #include > diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c > index eebbbfd00a83..5946081afbb7 100644 > --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c > +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibNoStm.c > @@ -8,7 +8,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > > -#include > +#include > #include > #include "CpuFeaturesLib.h" > > diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c > index 4b6bf958cedf..cda1ab8e78de 100644 > --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c > +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c > @@ -6,7 +6,7 @@ > > **/ > > -#include > +#include > #include > #include > #include (1) Do you really need to update this header file? It's never built for the standalone MM lib instance. (If you do it for consistency, I guess that's OK, but then it should be mentioned in the commit message.) > diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.c > new file mode 100644 > index 000000000000..114b177e5e57 > --- /dev/null > +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.c > @@ -0,0 +1,51 @@ > +/** @file > +Standalone MM CPU specific programming. > + > +Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.
> +Copyright (c) Microsoft Corporation.
> +SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > +#include > +#include "CpuFeaturesLib.h" > + > +/** > + Gets the maximum number of logical processors from the PCD PcdCpuMaxLogicalProcessorNumber. > + > + This access is abstracted from the PCD services to enforce that the PCD be > + FixedAtBuild in the Standalone MM build of this driver. > + > + @retval The value of PcdCpuMaxLogicalProcessorNumber. (2) Sorry, I'm just noticing -- given that we don't provide a constant return value here, we should use "@return", not "@retval". (Applies to all occurrences of the GetCpuMaxLogicalProcessorNumber() documentation.) > + > + > +**/ > +UINT32 > +GetCpuMaxLogicalProcessorNumber ( > + VOID > + ) > +{ > + return FixedPcdGet32 (PcdCpuMaxLogicalProcessorNumber); > +} > + > +/** > + The Standalone MM library constructor. > + > + @param[in] ImageHandle Image handle of this driver. > + @param[in] SystemTable A Pointer to the EFI System Table. > + > + @retval EFI_SUCCESS This constructor always returns success. > + > +**/ > +EFI_STATUS > +EFIAPI > +StandaloneMmCpuFeaturesLibConstructor ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_MM_SYSTEM_TABLE *SystemTable > + ) > +{ > + CpuFeaturesLibInitialization (); > + > + return EFI_SUCCESS; > +} > diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf > new file mode 100644 > index 000000000000..64f0a0853337 > --- /dev/null > +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf > @@ -0,0 +1,38 @@ > +## @file > +# Standalone MM CPU specific programming. > +# > +# Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.
> +# Copyright (c) Microsoft Corporation.
> +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +## > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = StandaloneMmCpuFeaturesLib > + MODULE_UNI_FILE = SmmCpuFeaturesLib.uni > + FILE_GUID = BB554A2D-F5DF-41D3-8C62-46476A2B2B18 > + MODULE_TYPE = MM_STANDALONE > + VERSION_STRING = 1.0 > + PI_SPECIFICATION_VERSION = 0x00010032 > + LIBRARY_CLASS = SmmCpuFeaturesLib > + CONSTRUCTOR = StandaloneMmCpuFeaturesLibConstructor > + > +[Sources] > + CpuFeaturesLib.h > + StandaloneMmCpuFeaturesLib.c > + SmmCpuFeaturesLib.c > + SmmCpuFeaturesLibNoStm.c (3) Darn. I'm displeased with my previous feedback. Unfortunately, I couldn't foresee the end result of my v1 comments, at such a distance. With v2 applied, we build the SmmCpuFeaturesLibConstructor() function from "SmmCpuFeaturesLibNoStm.c" for the StandaloneMmCpuFeaturesLib instance as well -- we just never call it. It's not really clean. The "feature map" (?) is something like this: traditional, traditional, standalone, no STM STM no STM entry point type DXE DXE MM lib inst. init. basic STM basic processor init. basic STM basic PCD access any any fixed The first two properties, i.e. "entry point type" and "library instance initialization", dictate the constructor implementation *together*. And that suggests we need three separate files (DXE-basic, DXE-STM, MM-basic). In other words, the SmmCpuFeaturesLibConstructor() function should be re-added to yet another new file, in patch#2. On the other hand, creating yet another C file with just the DXE-basic constructor seems awkward. :( (This C file would be listed in "SmmCpuFeaturesLib.inf", but not "StandaloneMmCpuFeaturesLib.inf".) Any suggestions / preferences? (I apologize again for missing this in the v1 review.) Thanks Laszlo > + > +[Packages] > + MdePkg/MdePkg.dec > + UefiCpuPkg/UefiCpuPkg.dec > + > +[LibraryClasses] > + BaseLib > + DebugLib > + MemoryAllocationLib > + PcdLib > + > +[FixedPcd] > + gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## SOMETIMES_CONSUMES > diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc > index 9128cef076dd..7db419471deb 100644 > --- a/UefiCpuPkg/UefiCpuPkg.dsc > +++ b/UefiCpuPkg/UefiCpuPkg.dsc > @@ -154,6 +154,7 @@ [Components.IA32, Components.X64] > UefiCpuPkg/Library/SmmCpuPlatformHookLibNull/SmmCpuPlatformHookLibNull.inf > UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf > UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf > + UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf > UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.inf > UefiCpuPkg/PiSmmCommunication/PiSmmCommunicationPei.inf > UefiCpuPkg/PiSmmCommunication/PiSmmCommunicationSmm.inf >