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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki Sent: Friday, March 26, 2021 6:45 PM To: devel@edk2.groups.io; Chiu, Chasel <chasel.c...@intel.com>; Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; Liming Gao <gaolim...@byosoft.com.cn>; Dong, Eric <eric.d...@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.kuba...@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 filepathremain 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.c...@intel.com> Cc: Nate DeSimone <nathaniel.l.desim...@intel.com> Cc: Liming Gao <gaolim...@byosoft.com.cn> Cc: Eric Dong <eric.d...@intel.com> Signed-off-by: Michael Kubacki <michael.kuba...@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 thisdriver- @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 thisdriver+ @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 thisdriver+ @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 eitherFixedAtBuild+# or PatchableInModule [Pcd] gMinPlatformPkgTokenSpaceGuid.PcdAcpiEnableSwSmi ##CONSUMESgMinPlatformPkgTokenSpaceGuid.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.infMemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmMemoryAll ocationLib/StandaloneMmMemoryAllocationLib.infMmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/Stan daloneMmServicesTableLib.inf + PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.infSpiFlashCommonLib|MinPlatformPkg/Flash/Library/SpiFlashCommonLibNull/ SpiFlashCommonLibNull.infStandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntry Point/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.infMinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiEnableLibNull.infMinPlatformPkg/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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
