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

[Laszlo Ersek <lersek@redhat.com>]

On 01/10/19 08:59, Zeng, Star wrote:
> 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?

It should be possible to build all library instances in a central
Package (well, all Packages really), using the Package's DSC file. To my
understanding, libraries built like this are not expected to be used in
actual (shipped) drivers / applications, nor is their indiscriminate
distribution (as LIBs) expected. For example, shipping a BaseXxxLibNull
library instance in binary form seems quite useless.

With that in mind, I think a Null instance for the entry point in
question makes sense, under MdeModulePkg.

Thanks
Laszlo


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