Re: [PATCH 2/6] MdePkg: implement MmServicesTableLib based on traditional SMM

["Zeng, Star" <star.zeng@intel.com>]

Hi Ard,

Another minor feedback.

On 2019/1/10 14:14, Zeng, Star wrote:
> Hi Ard,
> 
> Some minor feedback added inline.
> 
> On 2019/1/4 2:28, Ard Biesheuvel wrote:
>> The definitions of the rebranded MM protocol stack were chosen such
>> that the existing SMM based core drivers can be reused. So let's
>> implement MmServicesTableLib based on gEfiMmBaseProtocolGuid, which
>> is simply gEfiSmmBase2ProtocolGuid under the hood.
> 
> Good commit log.
> 
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>   MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c   | 63 
>> ++++++++++++++++++++
>>   MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf | 45 
>> ++++++++++++++
>>   MdePkg/Library/MmServicesTableLib/MmServicesTableLib.uni | 22 +++++++
>>   MdePkg/MdePkg.dsc                                        |  1 +
>>   4 files changed, 131 insertions(+)
>>
>> diff --git a/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c 
>> b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
>> new file mode 100644
>> index 000000000000..f41d25e59805
>> --- /dev/null
>> +++ b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
>> @@ -0,0 +1,63 @@
>> +/** @file
>> +  MM Services Table Library.
>> +
>> +  Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
>> +  Copyright (c) 2018, Linaro, Ltd. All rights reserved.<BR>
>> +  This program and the accompanying materials
>> +  are licensed and made available under the terms and conditions of 
>> the BSD License
>> +  which accompanies this distribution.  The full text of the license 
>> may be found at
>> +  http://opensource.org/licenses/bsd-license.php.
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS 
>> OR IMPLIED.
>> +
>> +**/
>> +
>> +#include <PiMm.h>
>> +#include <Protocol/MmBase.h>
>> +#include <Library/MmServicesTableLib.h>
>> +#include <Library/DebugLib.h>
>> +
>> +EFI_MM_SYSTEM_TABLE   *gMmst             = NULL;
>> +
>> +/**
>> +  The constructor function caches the pointer of SMM Services Table.

Use "MM" instead of "SMM" here?

Thanks,
Star

>> +
>> +  @param  ImageHandle   The firmware allocated handle for the EFI image.
>> +  @param  SystemTable   A pointer to the EFI System Table.
>> +
>> +  @retval EFI_SUCCESS   The constructor always returns EFI_SUCCESS.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +MmServicesTableLibConstructor (
>> +  IN EFI_HANDLE        ImageHandle,
>> +  IN EFI_SYSTEM_TABLE  *SystemTable
>> +  )
>> +{
>> +  EFI_STATUS              Status;
>> +  EFI_MM_BASE_PROTOCOL    *InternalMmBase;
>> +
>> +  InternalMmBase = NULL;
>> +  //
>> +  // Retrieve MM Base Protocol,  Do not use gBS from 
>> UefiBootServicesTableLib on purpose
>> +  // to prevent inclusion of gBS, gST, and gImageHandle from SMM 
>> Drivers unless the
>> +  // MM driver explicity declares that dependency.
>> +  //
>> +  Status = SystemTable->BootServices->LocateProtocol (
>> +                                        &gEfiMmBaseProtocolGuid,
>> +                                        NULL,
>> +                                        (VOID **)&InternalMmBase
>> +                                        );
>> +  ASSERT_EFI_ERROR (Status);
>> +  ASSERT (InternalMmBase != NULL);
>> +
>> +  //
>> +  // We are in MM, retrieve the pointer to MM System Table
>> +  //
>> +  InternalMmBase->GetMmstLocation (InternalMmBase, &gMmst);
>> +  ASSERT (gMmst != NULL);
>> +
>> +  return EFI_SUCCESS;
>> +}
>> diff --git a/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf 
>> b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf
>> new file mode 100644
>> index 000000000000..4418cc2f1464
>> --- /dev/null
>> +++ b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf
>> @@ -0,0 +1,45 @@
>> +## @file
>> +# MM Services Table Library.
>> +#
>> +# Copyright (c) 2009 - 2014, Intel Corporation. All rights reserved.<BR>
>> +# Copyright (c) 2018, Linaro, Ltd. All rights reserved.<BR>
>> +#
>> +#  This program and the accompanying materials
>> +#  are licensed and made available under the terms and conditions of 
>> the BSD License
>> +#  which accompanies this distribution. The full text of the license 
>> may be found at
>> +#  http://opensource.org/licenses/bsd-license.php.
>> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS 
>> OR IMPLIED.
>> +#
>> +#
>> +##
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x0001001B
>> +  BASE_NAME                      = MmServicesTableLib
>> +  MODULE_UNI_FILE                = MmServicesTableLib.uni
>> +  FILE_GUID                      = 9508ECFD-66D1-4B4C-9415-F25F0FFF9E93
>> +  MODULE_TYPE                    = DXE_SMM_DRIVER
>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = MmServicesTableLib|DXE_SMM_DRIVER
>> +  PI_SPECIFICATION_VERSION       = 0x00010032
>> +  CONSTRUCTOR                    = MmServicesTableLibConstructor
>> +
>> +#
>> +#  VALID_ARCHITECTURES           = IA32 X64 ARM AARCH64
>> +#
>> +
>> +[Sources]
>> +  MmServicesTableLib.c
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +
>> +[LibraryClasses]
>> +  DebugLib
>> +
>> +[Protocols]
>> +  gEfiMmBaseProtocolGuid                      ## CONSUMES
>> +
>> +[Depex]
>> +  gEfiMmBaseProtocolGuid
>> diff --git a/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.uni 
>> b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.uni
>> new file mode 100644
>> index 000000000000..2b2c7e14024c
>> --- /dev/null
>> +++ b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.uni
>> @@ -0,0 +1,22 @@
>> +// /** @file
>> +// MM Services Table Library.
>> +//
>> +// MM Services Table Library.
> 
> How about adding "It implements MmServicesTableLib based on 
> gEfiMmBaseProtocolGuid, which is simply gEfiSmmBase2ProtocolGuid under 
> the hood." here to make the implementation more clear?
> 
> With Jian's feedback and this handled, Reviewed-by: Star Zeng 
> <star.zeng@intel.com>.
> 
> Thanks,
> Star
> 
>> +//
>> +// Copyright (c) 2009 - 2014, Intel Corporation. All rights 
>> reserved.<BR>
>> +// Copyright (c) 2018, Linaro, Ltd. All rights reserved.<BR>
>> +//
>> +// This program and the accompanying materials
>> +// are licensed and made available under the terms and conditions of 
>> the BSD License
>> +// which accompanies this distribution. The full text of the license 
>> may be found at
>> +// http://opensource.org/licenses/bsd-license.php.
>> +// THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +// WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS 
>> OR IMPLIED.
>> +//
>> +// **/
>> +
>> +
>> +#string STR_MODULE_ABSTRACT             #language en-US "MM Services 
>> Table Library"
>> +
>> +#string STR_MODULE_DESCRIPTION          #language en-US "MM Services 
>> Table Library."
>> +
>> diff --git a/MdePkg/MdePkg.dsc b/MdePkg/MdePkg.dsc
>> index d43ffe4deb49..428b14e6a4e0 100644
>> --- a/MdePkg/MdePkg.dsc
>> +++ b/MdePkg/MdePkg.dsc
>> @@ -141,6 +141,7 @@ [Components.IA32, Components.X64]
>>     MdePkg/Library/BaseRngLib/BaseRngLib.inf
>>     MdePkg/Library/SmmPciExpressLib/SmmPciExpressLib.inf
>>     MdePkg/Library/SmiHandlerProfileLibNull/SmiHandlerProfileLibNull.inf
>> +  MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf
>>   [Components.EBC]
>>     MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
>>