public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Nate DeSimone" <nathaniel.l.desimone@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
	"mikuback@linux.microsoft.com" <mikuback@linux.microsoft.com>,
	"Chiu, Chasel" <chasel.chiu@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
Date: Thu, 1 Apr 2021 19:47:08 +0000	[thread overview]
Message-ID: <MWHPR1101MB2160FECA85ACDC6FD1E27B58CD7B9@MWHPR1101MB2160.namprd11.prod.outlook.com> (raw)
In-Reply-To: <8f967a59-c8de-506e-bf32-a1e0a4813c0a@linux.microsoft.com>

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.

> > +  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
> >
> 
> 
> 
> 


  parent reply	other threads:[~2021-04-01 19:47 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <166A6D94DDC3B65B.3944@groups.io>
2021-03-27  1:45 ` [edk2-devel] [edk2-platforms][PATCH v2 1/1] MinPlatformPkg/Acpi/AcpiSmm: Add Standalone MM support Michael Kubacki
2021-03-29  5:15   ` 回复: " gaoliming
2021-04-01 19:47   ` Nate DeSimone [this message]
2021-04-02  0:49     ` Michael Kubacki
2021-04-02  1:26       ` Nate DeSimone

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=MWHPR1101MB2160FECA85ACDC6FD1E27B58CD7B9@MWHPR1101MB2160.namprd11.prod.outlook.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox