Re: [PATCH v2 4/4] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support

["Laszlo Ersek" <lersek@redhat.com>]

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 <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 pre-existing 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           
>>> |  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 <PiSmm.h>
>>> +#include <PiMm.h>
>>>   #include <Library/SmmCpuFeaturesLib.h>
>>>   #include <Library/BaseLib.h>
>>>   #include <Library/MtrrLib.h>
>>> 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 <PiSmm.h>
>>> +#include <PiMm.h>
>>>   #include <Library/SmmCpuFeaturesLib.h>
>>>   #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 <PiSmm.h>
>>> +#include <PiMm.h>
>>>   #include <Library/BaseLib.h>
>>>   #include <Library/BaseMemoryLib.h>
>>>   #include <Library/MemoryAllocationLib.h>
>>
>> (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.<BR>
>>> +Copyright (c) Microsoft Corporation.<BR>
>>> +SPDX-License-Identifier: BSD-2-Clause-Patent
>>> +
>>> +**/
>>> +
>>> +#include <PiMm.h>
>>> +#include <Library/PcdLib.h>
>>> +#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.<BR>
>>> +#  Copyright (c) Microsoft Corporation.<BR>
>>> +#  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
>>>
>