Re: [PATCH 4/6] MdeModulePkg/FaultTolerantWriteDxe: implement standalone MM version

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

On 2019/1/10 15:33, Ard Biesheuvel wrote:
> On Thu, 10 Jan 2019 at 08:30, Zeng, Star <star.zeng@intel.com> wrote:
>>
>> Hi Ard,
>>
>> Another minor feedback.
>>
>> On 2019/1/10 14:47, Zeng, Star wrote:
>>> Hi Ard,
>>>
>>> Some minor feedback added inline.
>>>
>>> On 2019/1/4 2:28, Ard Biesheuvel wrote:
>>>> Implement a new version of the fault tolerant write driver that can
>>>> be used in the context of a standalone MM implementation.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> ---
>>>>
>>>> MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.c
>>>> | 70 +++++++++++++++
>>>>
>>>> MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf
>>>> | 90 ++++++++++++++++++++
>>>>    2 files changed, 160 insertions(+)
>>
>> Please add it into MdeModulePkg.dsc for package build verification.
>>
> 
> Hello Star,
> 
> Thanks for all the feedback. I will respond in more detail later.
> 
> However, to the point raised here: it is not possible to add these
> drivers to MdeModulePkg.dsc unless we add a dummy implementation of
> StandaloneMmDriverEntryPoint to MdeModulePkg. Do you think we should
> do that?

Oh, good information.
To have full code building coverage for the package, personally I think 
we can move StandaloneMmDriverEntryPoint library class and instance into 
MdePkg, and even the MmServicesTableLib for MM_STANDALONE, they should 
be generic enough.

I do not want to block this patch set because of this. So let's discuss 
this in parallel as separated topic.

Mike, Liming, Laszlo, Jian and Hao,\
What's your opinion?


Thanks,
Star

> 
> 
>>>>
>>>> diff --git
>>>> a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.c
>>>> b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.c
>>>>
>>>> new file mode 100644
>>>> index 000000000000..b6fbf6c64f8a
>>>> --- /dev/null
>>>> +++
>>>> b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.c
>>>>
>>>> @@ -0,0 +1,70 @@
>>>> +/** @file
>>>> +
>>>> +  Parts of the SMM/MM implementation that are specific to standalone MM
>>>> +
>>>> +Copyright (c) 2010 - 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 <Library/SmmMemLib.h>
>>>> +#include <Library/UefiBootServicesTableLib.h>
>>>> +#include "FaultTolerantWrite.h"
>>>> +#include "FaultTolerantWriteSmmCommon.h"
>>>> +
>>>> +BOOLEAN
>>>> +FtwSmmIsBufferOutsideSmmValid (
>>>> +  IN EFI_PHYSICAL_ADDRESS  Buffer,
>>>> +  IN UINT64                Length
>>>> +  )
>>>> +{
>>>> +  return TRUE;
>>>> +}
>>>
>>> Please add function comment header for it, otherwise some coding style
>>> tool may report error.
>>>
>>>> +
>>>> +/**
>>>> +  Internal implementation of CRC32. Depending on the execution context
>>>> +  (standalone SMM or DXE vs standalone MM), this function is implemented
>>>> +  via a call to the CalculateCrc32 () boot service, or via a library
>>>> +  call.
>>>> +
>>>> +  If Buffer is NULL, then ASSERT().
>>>> +  If Length is greater than (MAX_ADDRESS - Buffer + 1), then ASSERT().
>>>> +
>>>> +  @param[in]  Buffer       A pointer to the buffer on which the
>>>> 32-bit CRC is to be computed.
>>>> +  @param[in]  Length       The number of bytes in the buffer Data.
>>>> +
>>>> +  @retval Crc32            The 32-bit CRC was computed for the data
>>>> buffer.
>>>> +
>>>> +**/
>>>> +UINT32
>>>> +FtwCalculateCrc32 (
>>>> +  IN  VOID                         *Buffer,
>>>> +  IN  UINTN                        Length
>>>> +  )
>>>> +{
>>>> +  return CalculateCrc32 (Buffer, Length);
>>>> +}
>>>
>>> Please add function comment header for it, otherwise some coding style
>>> tool may report error.
>>>
>>>> +
>>>> +VOID
>>>> +FtwNotifySmmReady (
>>>> +  VOID
>>>> +  )
>>>> +{
>>>> +}
>>>
>>> Please add function comment header for it, otherwise some coding style
>>> tool may report error.
>>>
>>> Thanks,
>>> Star
>>>
>>>> +
>>>> +EFI_STATUS
>>>> +EFIAPI
>>>> +StandaloneMmFaultTolerantWriteInitialize (
>>>> +  IN EFI_HANDLE            ImageHandle,
>>>> +  IN EFI_MM_SYSTEM_TABLE   *MmSystemTable
>>>> +  )
>>>> +{
>>>> +  return MmFaultTolerantWriteInitialize ();
>>>> +}
>>>> diff --git
>>>> a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf
>>>> b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf
>>>>
>>>> new file mode 100644
>>>> index 000000000000..99bd62ad5ceb
>>>> --- /dev/null
>>>> +++
>>>> b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf
>>>>
>>>> @@ -0,0 +1,90 @@
>>>> + ## @file
>>>> +#   Fault Tolerant Write Smm Driver.
>>>> +#
>>>> +#   This driver installs SMM Fault Tolerant Write (FTW) protocol,
>>>> which provides fault
>>>> +#   tolerant write capability in SMM environment for block devices.
>>>> Its implementation
>>>> +#   depends on the full functionality SMM FVB protocol that support
>>>> read, write/erase
>>>> +#   flash access.
>>>> +#
>>>> +# Copyright (c) 2010 - 2018, Intel Corporation. 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                    = 0x0001001A
>>>> +  BASE_NAME                      = FaultTolerantWriteStandaloneMm
>>>> +  FILE_GUID                      = 3aade4ec-63cc-4a48-a928-5a374dd463eb
>>>> +  MODULE_TYPE                    = MM_STANDALONE
>>>> +  VERSION_STRING                 = 1.0
>>>> +  PI_SPECIFICATION_VERSION       = 0x00010032
>>>> +  ENTRY_POINT                    =
>>>> StandaloneMmFaultTolerantWriteInitialize
>>>> +
>>>> +#
>>>> +# The following information is for reference only and not required by
>>>> the build tools.
>>>> +#
>>>> +#  VALID_ARCHITECTURES           = AARCH64
>>>> +#
>>>> +
>>>> +[Sources]
>>>> +  FtwMisc.c
>>>> +  UpdateWorkingBlock.c
>>>> +  FaultTolerantWrite.c
>>>> +  FaultTolerantWriteStandaloneMm.c
>>>> +  FaultTolerantWriteSmm.c
>>>> +  FaultTolerantWrite.h
>>>> +  FaultTolerantWriteSmmCommon.h
>>>> +
>>>> +[Packages]
>>>> +  MdePkg/MdePkg.dec
>>>> +  MdeModulePkg/MdeModulePkg.dec
>>>> +  StandaloneMmPkg/StandaloneMmPkg.dec
>>>> +
>>>> +[LibraryClasses]
>>>> +  BaseLib
>>>> +  BaseMemoryLib
>>>> +  DebugLib
>>>> +  MemoryAllocationLib
>>>> +  MmServicesTableLib
>>>> +  PcdLib
>>>> +  ReportStatusCodeLib
>>>> +  StandaloneMmDriverEntryPoint
>>>> +
>>>> +[Guids]
>>>> +  #
>>>> +  # Signature in EFI_FAULT_TOLERANT_WORKING_BLOCK_HEADER
>>>> +  #
>>>> +  ## CONSUMES           ## GUID
>>>> +  ## PRODUCES           ## GUID
>>>> +  gEdkiiWorkingBlockSignatureGuid
>>>> +
>>>> +[Protocols]
>>>> +  gEfiSmmSwapAddressRangeProtocolGuid |
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdFullFtwServiceEnable  ##
>>>> SOMETIMES_CONSUMES
>>>> +  ## NOTIFY
>>>> +  ## CONSUMES
>>>> +  gEfiSmmFirmwareVolumeBlockProtocolGuid
>>>> +  ## PRODUCES
>>>> +  ## UNDEFINED # SmiHandlerRegister
>>>> +  gEfiSmmFaultTolerantWriteProtocolGuid
>>>> +  gEfiMmEndOfDxeProtocolGuid                       ## CONSUMES
>>>> +
>>>> +[FeaturePcd]
>>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFullFtwServiceEnable    ## CONSUMES
>>>> +
>>>> +[Pcd]
>>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase
>>>> ## SOMETIMES_CONSUMES
>>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64
>>>> ## CONSUMES
>>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
>>>> ## CONSUMES
>>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase
>>>> ## SOMETIMES_CONSUMES
>>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64
>>>> ## CONSUMES
>>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
>>>> ## CONSUMES
>>>> +
>>>> +[Depex]
>>>> +  TRUE
>>>> +
>>>>
>>