[edk2-devel] [PATCH v2 4/4] UefiCpuPkg/SmmCpuFeaturesLib: Add Standalone MM support
Laszlo Ersek
lersek at redhat.com
Wed Feb 17 14:19:05 UTC 2021
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 at linux.microsoft.com wrote:
>>> From: Michael Kubacki <michael.kubacki at 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 at intel.com>
>>> Cc: Ray Ni <ray.ni at intel.com>
>>> Cc: Laszlo Ersek <lersek at redhat.com>
>>> Cc: Rahul Kumar <rahul1.kumar at intel.com>
>>> Signed-off-by: Michael Kubacki <michael.kubacki at 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
>>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71745): https://edk2.groups.io/g/devel/message/71745
Mute This Topic: https://groups.io/mt/80599748/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list