From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web10.9747.1617324572563048595 for ; Thu, 01 Apr 2021 17:49:32 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=A7oRBbPV; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [10.124.238.202] (unknown [167.220.2.74]) by linux.microsoft.com (Postfix) with ESMTPSA id 8F4AB20B5680; Thu, 1 Apr 2021 17:49:31 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 8F4AB20B5680 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1617324571; bh=CKJg6uFDjw64f3/Jyv2vMhxXPFzPeaNAHG1boPXVKUw=; h=Subject:To:References:From:Date:In-Reply-To:From; b=A7oRBbPVVCgY74KofxuIOTcX76Gm14X4txHvk8UaaMaQqc2wA5HZEBtgnqKc/1y71 a/jVut8h50ewaqrVNFs0SeTO+f/WD4fJ1cdbqOKlKHB6AFZoIkl1F7oFH4t3oOcJUh 1cgIS9d+X3vHHhfP8RaH8SS/4UB2l5rlaSs7ele4= Subject: Re: [edk2-devel] [edk2-platforms][PATCH v2 1/1] MinPlatformPkg/Acpi/AcpiSmm: Add Standalone MM support To: "Desimone, Nathaniel L" , "devel@edk2.groups.io" , "Chiu, Chasel" , Liming Gao , "Dong, Eric" References: <166A6D94DDC3B65B.3944@groups.io> <8f967a59-c8de-506e-bf32-a1e0a4813c0a@linux.microsoft.com> From: "Michael Kubacki" Message-ID: <90d1e321-d585-021a-1687-bbf3d7a862ec@linux.microsoft.com> Date: Thu, 1 Apr 2021 17:49:31 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 On Behalf Of Michael >> Kubacki >> Sent: Friday, March 26, 2021 6:45 PM >> To: devel@edk2.groups.io; Chiu, Chasel ; Desimone, >> Nathaniel L ; Liming Gao >> ; Dong, Eric >> 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 >>> >>> 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 >>> Cc: Nate DeSimone >>> Cc: Liming Gao >>> Cc: Eric Dong >>> Signed-off-by: Michael Kubacki >>> --- >>> >>> 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.
>>> +Copyright (c) Microsoft Corporation.
>>> SPDX-License-Identifier: BSD-2-Clause-Patent >>> >>> **/ >>> >>> -#include "AcpiSmm.h" >>> +#include >>> +#include #include >>> +#include #include >>> +#include >>> + >>> +#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.
>>> +Copyright (c) Microsoft Corporation.
>>> +SPDX-License-Identifier: BSD-2-Clause-Patent >>> + >>> +**/ >>> + >>> +#include >>> + >>> +#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.
>>> +Copyright (c) Microsoft Corporation.
>>> +SPDX-License-Identifier: BSD-2-Clause-Patent >>> + >>> +**/ >>> + >>> +#include >>> + >>> +#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.
>>> +Copyright (c) Microsoft Corporation.
>>> +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.
>>> -SPDX-License-Identifier: BSD-2-Clause-Patent >>> - >>> -**/ >>> - >>> -#ifndef _ACPI_SMM_H_ >>> -#define _ACPI_SMM_H_ >>> - >>> -#include >>> -#include >>> -#include >>> -#include >>> -#include >>> -#include >>> -#include >>> -#include -#include >>> -#include >>> - >>> -#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.
>>> +# Copyright (c) Microsoft Corporation.
>>> # >>> # 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.
>>> +# Copyright (c) Microsoft Corporation.
>>> # >>> # 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 >>> >> >> >> >> >