From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web08.8085.1613571555790888744 for ; Wed, 17 Feb 2021 06:19:15 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=YLt6nWV7; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1613571554; 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=iiKh/40aarE39S5Ku98eWkVZME9jajKjxGblDekNxHM=; b=YLt6nWV7M9VoiQdyPKhbqiFnmDLmRad3oetUMMUJjXpOo/38YDH9M681oSftSMluea/ukj Uxl9DSBouC+QvWnYjYh6E8JZVfzMEFk+X1k5EpCs5YS7seJSIrGGjoVBMwqY4p9NihYIka Mk7OC9bJqgv14d+bSy0Y0DQVkvy9uCw= 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-527-so0kPVfQP2-mRlgBeLxnuA-1; Wed, 17 Feb 2021 09:19:10 -0500 X-MC-Unique: so0kPVfQP2-mRlgBeLxnuA-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id E6645801982; Wed, 17 Feb 2021 14:19:08 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-236.ams2.redhat.com [10.36.115.236]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4A34160C6B; Wed, 17 Feb 2021 14:19:06 +0000 (UTC) Subject: Re: [PATCH v2 4/4] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support To: Michael Kubacki , 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> <96e8e185-ce9e-91a4-0ce4-f98b697b48c1@redhat.com> <5f3b6f0b-3974-ed1f-4ed2-195d0e782772@linux.microsoft.com> From: "Laszlo Ersek" Message-ID: <9cff9359-30b8-c7fd-60fa-764781ecca53@redhat.com> Date: Wed, 17 Feb 2021 15:19:05 +0100 MIME-Version: 1.0 In-Reply-To: <5f3b6f0b-3974-ed1f-4ed2-195d0e782772@linux.microsoft.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 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: 8bit On 02/16/21 21:11, Michael Kubacki wrote: > On 2/15/2021 12:33 PM, Laszlo Ersek wrote: >> 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.) >> > I did update it for consistency. I will note this in the commit message > in v3. >> >>> 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.) >> >> > I will update to "@return" in v3. >>> + >>> + >>> +**/ >>> +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.) >> > I'm leaning toward adding it in a new file in patch #2. > > If that's done, is there a preference for the file name? > > After reviewing the current set of files, the usage of > "SmmCpuFeaturesLib.c" in slightly inconsistent with the other .c files > that handle their instance specific logic. > > Given that SmmCpuFeaturesLib.inf will now have an instance-specific > file, would it be more intuitive to have that file be named > "SmmCpuFeaturesLib.c" and rename the current file shared across all > instances to "SmmCpuFeaturesLibCommon.c"? > > If the rename occurred, the final file set would look like as below > across the instances. > > SmmCpuFeaturesLib.inf: >   [Sources] >     CpuFeaturesLib.h >     SmmCpuFeaturesLib.c --> Will contain SmmCpuFeaturesLibConstructor() >     SmmCpuFeaturesLibCommon.c >     SmmCpuFeaturesLibNoStm.c >     TraditionalMmCpuFeaturesLib.c > > SmmCpuFeaturesLibStm.inf: >   [Sources] >     CpuFeaturesLib.h >     SmmCpuFeaturesLibCommon.c >     SmmStm.c >     SmmStm.h >     TraditionalMmCpuFeaturesLib.c > > StandaloneMmCpuFeaturesLib.inf: >   [Sources] >     CpuFeaturesLib.h >     StandaloneMmCpuFeaturesLib.c >     SmmCpuFeaturesLibCommon.c >     SmmCpuFeaturesLibNoStm.c Looks good! 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 >>> >