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

["Wang, Jian J" <jian.j.wang@intel.com>]

Star,

I think moving it to MdePkg would be better, just like UefiDriverEntryPoint. A dummy
one may be not necessary.

Regards,
Jian


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Zeng,
> Star
> Sent: Thursday, January 10, 2019 4:00 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Gao, Liming
> <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Laszlo Ersek <lersek@redhat.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH 4/6] MdeModulePkg/FaultTolerantWriteDxe:
> implement standalone MM version
> 
> 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/FaultTolerantWriteStandalon
> eMm.c
> >>>> | 70 +++++++++++++++
> >>>>
> >>>>
> MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandalon
> eMm.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/FaultTolerantWriteStandal
> oneMm.c
> >>>>
> b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandal
> oneMm.c
> >>>>
> >>>> new file mode 100644
> >>>> index 000000000000..b6fbf6c64f8a
> >>>> --- /dev/null
> >>>> +++
> >>>>
> b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandal
> oneMm.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/FaultTolerantWriteStandal
> oneMm.inf
> >>>>
> b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandal
> oneMm.inf
> >>>>
> >>>> new file mode 100644
> >>>> index 000000000000..99bd62ad5ceb
> >>>> --- /dev/null
> >>>> +++
> >>>>
> b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandal
> oneMm.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
> >>>> +
> >>>>
> >>
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel