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

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

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.chiu@intel.com>; Desimone,
>> Nathaniel L <nathaniel.l.desimone@intel.com>; Liming Gao
>> <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@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@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 (
> 
> 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
>>>
>>
>>
>> 
>>
>