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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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?


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