Re: [edk2-devel] [edk2-platforms][PATCH v2 1/1] MinPlatformPkg/Acpi/AcpiSmm: Add Standalone MM support

["Michael Kubacki" <mikuback@linux.microsoft.com>]

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@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@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Eric Dong <eric.dong@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@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 (
> +  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/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.inf
>     MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf
> +  PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>     SpiFlashCommonLib|MinPlatformPkg/Flash/Library/SpiFlashCommonLibNull/SpiFlashCommonLibNull.inf
>     StandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntryPoint/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
>