[edk2-devel] [edk2-platforms][PATCH v2 1/1] MinPlatformPkg/Acpi/AcpiSmm: Add Standalone MM support
Michael Kubacki
mikuback at linux.microsoft.com
Fri Apr 2 00:49:31 UTC 2021
Hi Nate,
Reply is inline.
Thanks,
Michael
On 4/1/2021 12:47 PM, Desimone, Nathaniel L wrote:
> Hi Michael,
>
> Review comments inline.
>
> Thanks,
> Nate
>
>> -----Original Message-----
>> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Michael
>> Kubacki
>> Sent: Friday, March 26, 2021 6:45 PM
>> To: devel at edk2.groups.io; Chiu, Chasel <chasel.chiu at intel.com>; Desimone,
>> Nathaniel L <nathaniel.l.desimone at intel.com>; Liming Gao
>> <gaoliming at byosoft.com.cn>; Dong, Eric <eric.dong at intel.com>
>> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v2 1/1]
>> MinPlatformPkg/Acpi/AcpiSmm: Add Standalone MM support
>>
>> Sending a review reminder since it's been a few weeks.
>>
>> Thanks,
>> Michael
>>
>> On 3/8/2021 9:17 AM, Michael Kubacki wrote:
>>> From: Michael Kubacki <michael.kubacki at microsoft.com>
>>>
>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3248
>>>
>>> Adds a new module called AcpiStandaloneMm that serves the same role as
>>> AcpiSmm but in a Standalone MM environment.
>>>
>>> This change follows a similar pattern to other changes that have added
>>> Standalone MM support to a SMM module. The SMM INF name and file
>> path
>>> remain unaltered to allow backward compatibility and much of the code
>>> is shared between the driver instances with unique entry points for
>>> each respective module type.
>>>
>>> Cc: Chasel Chiu <chasel.chiu at intel.com>
>>> Cc: Nate DeSimone <nathaniel.l.desimone at intel.com>
>>> Cc: Liming Gao <gaoliming at byosoft.com.cn>
>>> Cc: Eric Dong <eric.dong at intel.com>
>>> Signed-off-by: Michael Kubacki <michael.kubacki at microsoft.com>
>>> ---
>>>
>>> Notes:
>>> V2 change: Add BZ link
>>>
>>> Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/{AcpiSmm.c => AcpiMm.c}
>> | 33 +++++++++----------
>>> Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiStandaloneMm.c
>> | 34 ++++++++++++++++++++
>>> Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiTraditionalMm.c
>> | 34 ++++++++++++++++++++
>>> Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiMm.h |
>> 23 +++++++++++++
>>> Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.h
>> | 24 --------------
>>> Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.inf
>> | 21 ++++++------
>>> Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/{AcpiSmm.inf =>
>> AcpiStandaloneMm.inf} | 32 +++++++++---------
>>> Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc | 2
>> ++
>>> 8 files changed, 133 insertions(+), 70 deletions(-)
>>>
>>> diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.c
>>> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiMm.c
>>> similarity index 81%
>>> rename from Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.c
>>> rename to Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiMm.c
>>> index 809f75d3c588..2cf559f3fe09 100644
>>> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.c
>>> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiMm.c
>>> @@ -1,12 +1,20 @@
>>> /** @file
>>> - Acpi Smm driver.
>>> + Functions shared between driver instances.
>>>
>>> Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>>> +Copyright (c) Microsoft Corporation.<BR>
>>> SPDX-License-Identifier: BSD-2-Clause-Patent
>>>
>>> **/
>>>
>>> -#include "AcpiSmm.h"
>>> +#include <PiMm.h>
>>> +#include <Library/BoardAcpiEnableLib.h> #include <Library/DebugLib.h>
>>> +#include <Library/MmServicesTableLib.h> #include <Library/PcdLib.h>
>>> +#include <Protocol/SmmSwDispatch2.h>
>>> +
>>> +#include "AcpiMm.h"
>>>
>>> /**
>>> Enable SCI
>>> @@ -53,20 +61,13 @@ DisableAcpiCallback (
>>> }
>>>
>>> /**
>>> - Initializes the Acpi Smm Driver
>>> -
>>> - @param[in] ImageHandle - Pointer to the loaded image protocol for this
>> driver
>>> - @param[in] SystemTable - Pointer to the EFI System Table
>>> -
>>> - @retval Status - EFI_SUCCESS
>>> - @retval Assert, otherwise.
>>> + ACPI initialization logic shared between the Traditional MM and
>>> + Standalone MM driver instances.
>>>
>>> **/
>>> -EFI_STATUS
>>> -EFIAPI
>>> -InitializeAcpiSmm (
>>> - IN EFI_HANDLE ImageHandle,
>>> - IN EFI_SYSTEM_TABLE *SystemTable
>>> +VOID
>>> +InitializeAcpiMm (
>>> + VOID
>>> )
>>> {
>>> EFI_STATUS Status;
>>> @@ -77,7 +78,7 @@ InitializeAcpiSmm (
>>> //
>>> // Locate the ICH SMM SW dispatch protocol
>>> //
>>> - Status = gSmst->SmmLocateProtocol
>> (&gEfiSmmSwDispatch2ProtocolGuid,
>>> NULL, (VOID**)&SwDispatch);
>>> + Status = gMmst->MmLocateProtocol
>> (&gEfiSmmSwDispatch2ProtocolGuid,
>>> + NULL, (VOID**) &SwDispatch);
>>> ASSERT_EFI_ERROR (Status);
>>>
>>> //
>>> @@ -103,6 +104,4 @@ InitializeAcpiSmm (
>>> &SwHandle
>>> );
>>> ASSERT_EFI_ERROR (Status);
>>> -
>>> - return EFI_SUCCESS;
>>> }
>>> diff --git
>>> a/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiStandaloneMm.c
>>> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiStandaloneMm.c
>>> new file mode 100644
>>> index 000000000000..f378942fdc07
>>> --- /dev/null
>>> +++
>> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiStandaloneMm.c
>>> @@ -0,0 +1,34 @@
>>> +/** @file
>>> + Standalone MM driver for ACPI initialization.
>>> +
>>> +Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>>> +Copyright (c) Microsoft Corporation.<BR>
>>> +SPDX-License-Identifier: BSD-2-Clause-Patent
>>> +
>>> +**/
>>> +
>>> +#include <PiMm.h>
>>> +
>>> +#include "AcpiMm.h"
>>> +
>>> +/**
>>> + The Standalone MM driver entry point.
>>> +
>>> + @param[in] ImageHandle - Pointer to the loaded image protocol for this
>> driver
>>> + @param[in] SystemTable - Pointer to the EFI MM System Table
>>> +
>>> + @retval Status - EFI_SUCCESS
>>> + @retval Assert, otherwise.
>>> +
>>> +**/
>>> +EFI_STATUS
>>> +EFIAPI
>>> +AcpiStandaloneMmEntryPoint (
>>> + IN EFI_HANDLE ImageHandle,
>>> + IN EFI_MM_SYSTEM_TABLE *MmSystemTable
>>> + )
>>> +{
>>> + InitializeAcpiMm ();
>>> +
>>> + return EFI_SUCCESS;
>>> +}
>>> diff --git
>>> a/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiTraditionalMm.c
>>> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiTraditionalMm.c
>>> new file mode 100644
>>> index 000000000000..9512926b9e2e
>>> --- /dev/null
>>> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiTraditionalMm.c
>>> @@ -0,0 +1,34 @@
>>> +/** @file
>>> + Traditional MM driver for ACPI initialization.
>>> +
>>> +Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>>> +Copyright (c) Microsoft Corporation.<BR>
>>> +SPDX-License-Identifier: BSD-2-Clause-Patent
>>> +
>>> +**/
>>> +
>>> +#include <PiMm.h>
>>> +
>>> +#include "AcpiMm.h"
>>> +
>>> +/**
>>> + The Traditional MM driver entry point.
>>> +
>>> + @param[in] ImageHandle - Pointer to the loaded image protocol for this
>> driver
>>> + @param[in] SystemTable - Pointer to the EFI System Table
>>> +
>>> + @retval Status - EFI_SUCCESS
>>> + @retval Assert, otherwise.
>>> +
>>> +**/
>>> +EFI_STATUS
>>> +EFIAPI
>>> +AcpiTraditionalMmEntryPoint (
>
> Please rename this to AcpiSmmEntryPoint(). To my knowledge the verbiage "Traditional MM" is not used anywhere else and "SMM" does a pretty good job of denoting this is the older path.
>
Where exactly do you mean by anywhere else?
It has a fair amount of usage in converted modules in edk2:
-
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/StatusCodeHandler/Smm/StatusCodeHandlerSmm.inf
-
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf
-
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxSmmLib.inf
-
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
-
https://github.com/tianocore/edk2/blob/master/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.inf
I also used this pattern when making the
MinPlatformPkg/Flash/SpiFvbService change:
-
https://github.com/tianocore/edk2-platforms/commit/f62887cbb37e846e6768ed05d6ac55f7288388dd
I don't have a strong preference but this pattern is similar to those
changes.
>>> + IN EFI_HANDLE ImageHandle,
>>> + IN EFI_SYSTEM_TABLE *SystemTable
>>> + )
>>> +{
>>> + InitializeAcpiMm ();
>>> +
>>> + return EFI_SUCCESS;
>>> +}
>>> diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiMm.h
>>> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiMm.h
>>> new file mode 100644
>>> index 000000000000..051474b0e833
>>> --- /dev/null
>>> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiMm.h
>>> @@ -0,0 +1,23 @@
>>> +/** @file
>>> + Internal header file for the ACPI MM driver.
>>> +
>>> +Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>>> +Copyright (c) Microsoft Corporation.<BR>
>>> +SPDX-License-Identifier: BSD-2-Clause-Patent
>>> +
>>> +**/
>>> +
>>> +#ifndef _ACPI_MM_H_
>>> +#define _ACPI_MM_H_
>>> +
>>> +/**
>>> + ACPI initialization logic shared between the Traditional MM and
>>> + Standalone MM driver instances.
>>> +
>>> +**/
>>> +VOID
>>> +InitializeAcpiMm (
>>> + VOID
>>> + );
>>> +
>>> +#endif
>>> diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.h
>>> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.h
>>> deleted file mode 100644
>>> index e34ffb1b755b..000000000000
>>> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.h
>>> +++ /dev/null
>>> @@ -1,24 +0,0 @@
>>> -/** @file
>>> - Header file for the Smm platform driver.
>>> -
>>> -Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>>> -SPDX-License-Identifier: BSD-2-Clause-Patent
>>> -
>>> -**/
>>> -
>>> -#ifndef _ACPI_SMM_H_
>>> -#define _ACPI_SMM_H_
>>> -
>>> -#include <PiSmm.h>
>>> -#include <Library/DebugLib.h>
>>> -#include <Library/HobLib.h>
>>> -#include <Library/IoLib.h>
>>> -#include <Library/PcdLib.h>
>>> -#include <Library/UefiBootServicesTableLib.h>
>>> -#include <Library/UefiLib.h>
>>> -#include <Library/SmmServicesTableLib.h> -#include
>>> <Library/BoardAcpiEnableLib.h> -#include <Protocol/SmmSwDispatch2.h>
>>> -
>>> -#endif
>>> -
>>> diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.inf
>>> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.inf
>>> index fbaf46752563..651d4a293e9b 100644
>>> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.inf
>>> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.inf
>>> @@ -1,7 +1,8 @@
>>> ### @file
>>> -# Component information file for ACPI SMM module.
>>> +# Component information file for ACPI Traditional MM module.
>>> #
>>> # Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>>> +# Copyright (c) Microsoft Corporation.<BR>
>>> #
>>> # SPDX-License-Identifier: BSD-2-Clause-Patent
>>> #
>>> @@ -14,18 +15,15 @@ [Defines]
>>> VERSION_STRING = 1.0
>>> MODULE_TYPE = DXE_SMM_DRIVER
>>> PI_SPECIFICATION_VERSION = 1.20
>>> - ENTRY_POINT = InitializeAcpiSmm
>>> + ENTRY_POINT = AcpiTraditionalMmEntryPoint
>>>
>>> [LibraryClasses]
>>> - UefiDriverEntryPoint
>>> - UefiBootServicesTableLib
>>> + BoardAcpiEnableLib
>>> DebugLib
>>> - HobLib
>>> - IoLib
>>> + MmServicesTableLib
>>> PcdLib
>>> + UefiDriverEntryPoint
>>> UefiLib
>>> - SmmServicesTableLib
>>> - BoardAcpiEnableLib
>>>
>>> [Packages]
>>> MdePkg/MdePkg.dec
>>> @@ -36,13 +34,12 @@ [Pcd]
>>> gMinPlatformPkgTokenSpaceGuid.PcdAcpiDisableSwSmi ##
>> CONSUMES
>>>
>>> [Sources]
>>> - AcpiSmm.h
>>> - AcpiSmm.c
>>> + AcpiMm.h
>>> + AcpiMm.c
>>> + AcpiTraditionalMm.c
>>>
>>> [Protocols]
>>> gEfiSmmSwDispatch2ProtocolGuid ## CONSUMES
>>>
>>> -[Guids]
>>> -
>>> [Depex]
>>> gEfiSmmSwDispatch2ProtocolGuid
>>> diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.inf
>>> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiStandaloneMm.inf
>>> similarity index 50%
>>> copy from Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.inf
>>> copy to
>>> Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiStandaloneMm.inf
>>> index fbaf46752563..f7d0861b512d 100644
>>> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.inf
>>> +++
>> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiSmm/AcpiStandaloneMm.inf
>>> @@ -1,7 +1,8 @@
>>> ### @file
>>> -# Component information file for ACPI SMM module.
>>> +# Component information file for ACPI Standalone MM module.
>>> #
>>> # Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>>> +# Copyright (c) Microsoft Corporation.<BR>
>>> #
>>> # SPDX-License-Identifier: BSD-2-Clause-Patent
>>> #
>>> @@ -9,40 +10,37 @@
>>>
>>> [Defines]
>>> INF_VERSION = 0x00010017
>>> - BASE_NAME = AcpiSmm
>>> - FILE_GUID = DF9A9FFC-A075-4867-A0B2-5E7540BB023E
>>> + BASE_NAME = AcpiStandaloneMm
>>> + FILE_GUID = F113611F-DEE7-4137-8623-0168675E9F6D
>>> VERSION_STRING = 1.0
>>> - MODULE_TYPE = DXE_SMM_DRIVER
>>> - PI_SPECIFICATION_VERSION = 1.20
>>> - ENTRY_POINT = InitializeAcpiSmm
>>> + MODULE_TYPE = MM_STANDALONE
>>> + PI_SPECIFICATION_VERSION = 0x00010032
>>> + ENTRY_POINT = AcpiStandaloneMmEntryPoint
>>>
>>> [LibraryClasses]
>>> - UefiDriverEntryPoint
>>> - UefiBootServicesTableLib
>>> + BoardAcpiEnableLib
>>> DebugLib
>>> - HobLib
>>> - IoLib
>>> + MmServicesTableLib
>>> PcdLib
>>> - UefiLib
>>> - SmmServicesTableLib
>>> - BoardAcpiEnableLib
>>> + StandaloneMmDriverEntryPoint
>>>
>>> [Packages]
>>> MdePkg/MdePkg.dec
>>> MinPlatformPkg/MinPlatformPkg.dec
>>>
>>> +# Note: All PCDs consumed in the Standalone MM instance must be either
>> FixedAtBuild
>>> +# or PatchableInModule
>>> [Pcd]
>>> gMinPlatformPkgTokenSpaceGuid.PcdAcpiEnableSwSmi ##
>> CONSUMES
>>> gMinPlatformPkgTokenSpaceGuid.PcdAcpiDisableSwSmi ##
>> CONSUMES
>>>
>>> [Sources]
>>> - AcpiSmm.h
>>> - AcpiSmm.c
>>> + AcpiMm.h
>>> + AcpiMm.c
>>> + AcpiStandaloneMm.c
>>>
>>> [Protocols]
>>> gEfiSmmSwDispatch2ProtocolGuid ## CONSUMES
>>>
>>> -[Guids]
>>> -
>>> [Depex]
>>> gEfiSmmSwDispatch2ProtocolGuid
>>> diff --git a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
>>> b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
>>> index 0460fd5a3206..998ee7909568 100644
>>> --- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
>>> +++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
>>> @@ -117,6 +117,7 @@ [LibraryClasses.common.MM_STANDALONE]
>>> DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
>>>
>> MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmMemoryAll
>> ocationLib/StandaloneMmMemoryAllocationLib.inf
>>>
>>>
>> MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/Stan
>> dal
>>> oneMmServicesTableLib.inf
>>> + PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>>>
>> SpiFlashCommonLib|MinPlatformPkg/Flash/Library/SpiFlashCommonLibNull/
>> SpiFlashCommonLibNull.inf
>>>
>>>
>> StandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntry
>> Poi
>>> nt/StandaloneMmDriverEntryPoint.inf
>>>
>>> @@ -147,6 +148,7 @@ [Components]
>>>
>>> MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf
>>> MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.inf
>>> + MinPlatformPkg/Acpi/AcpiSmm/AcpiStandaloneMm.inf
>>> MinPlatformPkg/Acpi/Library/DxeAslUpdateLib/DxeAslUpdateLib.inf
>>>
>> MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiEnableLibNull.inf
>>>
>>> MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiTableLibNull.inf
>>>
>>
>>
>>
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73630): https://edk2.groups.io/g/devel/message/73630
Mute This Topic: https://groups.io/mt/81179549/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