From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 69881211B63F1 for ; Thu, 10 Jan 2019 05:03:12 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CBF38420B3; Thu, 10 Jan 2019 13:03:11 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-17.rdu2.redhat.com [10.10.121.17]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0E1D9600C8; Thu, 10 Jan 2019 13:03:09 +0000 (UTC) To: "Zeng, Star" , Ard Biesheuvel Cc: Hao Wu , Michael D Kinney , "edk2-devel@lists.01.org" , Liming Gao References: <20190103182825.32231-1-ard.biesheuvel@linaro.org> <20190103182825.32231-6-ard.biesheuvel@linaro.org> <4a353237-1191-a831-1b6e-e981b7c59a7d@intel.com> <539a4ec0-eb09-588b-2a24-4b1f450f587b@intel.com> From: Laszlo Ersek Message-ID: <91581880-e3ef-31d6-40eb-1d52f5f0cdc1@redhat.com> Date: Thu, 10 Jan 2019 14:03:07 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Thu, 10 Jan 2019 13:03:11 +0000 (UTC) Subject: Re: [PATCH 4/6] MdeModulePkg/FaultTolerantWriteDxe: implement standalone MM version X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 Jan 2019 13:03:12 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 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 >>>>> --- >>>>> >>>>> 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.
>>>>> +Copyright (c) 2018, Linaro, Ltd. All rights reserved.
>>>>> +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 >>>>> +#include >>>>> +#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.
>>>>> +# >>>>> +#  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 >>>>> + >>>>> >>> >