Re: [PATCH 2/2] ArmPlatformPkg/NorFlash: Allow reusability as a MM driver

[Jagadeesh Ujja <jagadeesh.ujja@arm.com>]

On Tue, Feb 19, 2019 at 10:14 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> Hello Jagadeesh,
>
Hi Ard,

Thank you for your valuable comments. Will do the appropriate changes
based on your comments and publish in the next patchset

Regards,
Jagadeesh
> On Tue, 19 Feb 2019 at 11:32, Jagadeesh Ujja <jagadeesh.ujja@arm.com> wrote:
> >
> > Adapt the NorFlash driver to be used as a MM_STANDALONE driver to
> > allow access to NOR flash for code executing in MM_STANDALONE mode.
> > This allows storing of EFI variables on NOR flash which is accessible
> > only via the MM STANDALONE mode software.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jagadeesh Ujja <jagadeesh.ujja@arm.com>
> > ---
> >  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c   | 267 ++++++++++++++++++++
> >  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf |  77 ++++++
> >  2 files changed, 344 insertions(+)
> >
> > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
> > new file mode 100644
> > index 0000000..1e3603c
> > --- /dev/null
> > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
> > @@ -0,0 +1,267 @@
> > +/*++ @file  NorFlashStandaloneMm.c
> > +
> > + Copyright (c) 2019, ARM 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 <PiMm.h>
> > +
> > +#include <Library/BaseMemoryLib.h>
> > +#include <Library/MemoryAllocationLib.h>
> > +#include <Library/PcdLib.h>
> > +#include <Library/PcdLib.h>
> > +#include <Library/BaseLib.h>
> > +#include <Library/HobLib.h>
> > +#include <Library/UefiLib.h>
> > +#include <Library/BaseMemoryLib.h>
> > +#include <Library/MemoryAllocationLib.h>
>
> Please sort alphabetically - I know this originates in existing code,
> but it is a brand new file so let's make it clean from the start.
>
> > +
> > +#include <Guid/VariableFormat.h>
> > +#include <Guid/SystemNvDataGuid.h>
> > +#include <Guid/NvVarStoreFormatted.h>
>
> I don't think we need this - please see below.
>
> > +#include "NorFlash.h"
> > +
> > +//
> > +// Global variable declarations
> > +//
> > +NOR_FLASH_INSTANCE **mNorFlashInstances;
> > +UINT32               mNorFlashDeviceCount;
> > +
>
> These are definitions, not declarations. Could they be moved to a
> shared .c file instead?
>
> > +extern NOR_FLASH_INSTANCE  mNorFlashInstanceTemplate;
>
> Move this to a header?
>
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +NorFlashFvbInitialize (
> > +  IN NOR_FLASH_INSTANCE* Instance
> > +  )
> > +{
> > +  EFI_STATUS  Status;
> > +  UINT32      FvbNumLba;
> > +  EFI_BOOT_MODE BootMode;
> > +
> > +  DEBUG((DEBUG_BLKIO,"NorFlashFvbInitialize\n"));
> > +  ASSERT((Instance != NULL));
> > +
> > +  mFlashNvStorageVariableBase = FixedPcdGet32 (PcdFlashNvStorageVariableBase);
> > +
> > +  // Set the index of the first LBA for the FVB
> > +  Instance->StartLba = (PcdGet32 (PcdFlashNvStorageVariableBase) - Instance->RegionBaseAddress) / Instance->Media.BlockSize;
> > +
>
> Please wrap to 80 columns
>
> > +  BootMode = GetBootModeHob ();
>
> Where does the boot mode HOB come from in standalone MM?
>
> > +  if (BootMode == BOOT_WITH_DEFAULT_SETTINGS) {
> > +    Status = EFI_INVALID_PARAMETER;
> > +  } else {
> > +    // Determine if there is a valid header at the beginning of the NorFlash
> > +    Status = ValidateFvHeader (Instance);
> > +  }
> > +
>
>
> > +  // Install the Default FVB header if required
> > +  if (EFI_ERROR(Status)) {
> > +    // There is no valid header, so time to install one.
> > +    DEBUG ((EFI_D_INFO, "%a: The FVB Header is not valid.\n", __FUNCTION__));
> > +    DEBUG ((EFI_D_INFO, "%a: Installing a correct one for this volume.\n",
> > +      __FUNCTION__));
> > +
>
> Please use DEBUG_INFO not EFI_D_INFO (the latter form is deprecated)
>
> > +    // Erase all the NorFlash that is reserved for variable storage
> > +    FvbNumLba = (PcdGet32(PcdFlashNvStorageVariableSize) + PcdGet32(PcdFlashNvStorageFtwWorkingSize) + PcdGet32(PcdFlashNvStorageFtwSpareSize)) / Instance->Media.BlockSize;
> > +
>
> Please wrap to 80 columns
>
> > +    Status = FvbEraseBlocks (&Instance->FvbProtocol, (EFI_LBA)0, FvbNumLba, EFI_LBA_LIST_TERMINATOR);
> > +    if (EFI_ERROR(Status)) {
> > +      return Status;
> > +    }
> > +
> > +    // Install all appropriate headers
> > +    Status = InitializeFvAndVariableStoreHeaders (Instance);
> > +    if (EFI_ERROR(Status)) {
> > +      return Status;
> > +    }
>
> This if {} is redundant since we will be returning Status anyway.
>
> > +  }
> > +
> > +  return Status;
> > +}
> > +
> > +VOID
> > +EFIAPI
> > +NorFlashLock (
> > +  NOR_FLASH_LOCK_CONTEXT    *Context
> > +  )
> > +{
> > +}
> > +
> > +VOID
> > +EFIAPI
> > +NorFlashUnlock (
> > +  NOR_FLASH_LOCK_CONTEXT    *Context
> > +  )
> > +{
> > +}
> > +
> > +EFI_STATUS
> > +NorFlashCreateInstance (
> > +  IN UINTN                  NorFlashDeviceBase,
> > +  IN UINTN                  NorFlashRegionBase,
> > +  IN UINTN                  NorFlashSize,
> > +  IN UINT32                 Index,
> > +  IN UINT32                 BlockSize,
> > +  IN BOOLEAN                SupportFvb,
> > +  OUT NOR_FLASH_INSTANCE**  NorFlashInstance
> > +  )
> > +{
> > +  EFI_STATUS Status;
> > +  NOR_FLASH_INSTANCE* Instance;
> > +
> > +  ASSERT(NorFlashInstance != NULL);
> > +
> > +  Instance = AllocateRuntimeCopyPool (sizeof(NOR_FLASH_INSTANCE),&mNorFlashInstanceTemplate);
>
> Why runtime?
>
> > +  if (Instance == NULL) {
> > +    return EFI_OUT_OF_RESOURCES;
> > +  }
> > +
> > +  Instance->DeviceBaseAddress = NorFlashDeviceBase;
> > +  Instance->RegionBaseAddress = NorFlashRegionBase;
> > +  Instance->Size = NorFlashSize;
> > +
> > +  Instance->BlockIoProtocol.Media = &Instance->Media;
> > +  Instance->Media.MediaId = Index;
> > +  Instance->Media.BlockSize = BlockSize;
> > +  Instance->Media.LastBlock = (NorFlashSize / BlockSize)-1;
> > +
>
> We should get rid of the block I/O and disk I/O protocols here - we
> don't need them in standalone MM
>
> > +  CopyGuid (&Instance->DevicePath.Vendor.Guid, &gEfiCallerIdGuid);
> > +  Instance->DevicePath.Index = (UINT8)Index;
> > +
> > +  Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);;
>
> Use AllocatePool ()
> Drop redundant ;
>
> > +  if (Instance->ShadowBuffer == NULL) {
> > +    return EFI_OUT_OF_RESOURCES;
> > +  }
> > +
> > +  if (SupportFvb) {
> > +    NorFlashFvbInitialize (Instance);
> > +
> > +    //Install DevicePath Protocol
> > +    Status = gMmst->MmInstallProtocolInterface (
> > +                      &Instance->Handle,
> > +                      &gEfiDevicePathProtocolGuid,
>
> Do we need this in standalone MM ?
>
> > +                      EFI_NATIVE_INTERFACE,
> > +                      &Instance->DevicePath
> > +                      );
> > +    if (EFI_ERROR(Status)) {
> > +      FreePool (Instance);
> > +      return Status;
> > +    }
> > +    //Install BlockIo Protocol
> > +    Status = gMmst->MmInstallProtocolInterface (
> > +                      &Instance->Handle,
> > +                      &gEfiBlockIoProtocolGuid,
>
> or this
>
>
> > +                      EFI_NATIVE_INTERFACE,
> > +                      &Instance->BlockIoProtocol
> > +                      );
> > +    if (EFI_ERROR(Status)) {
> > +      FreePool (Instance);
> > +      return Status;
> > +    }
> > +    //Install FirmwareVolumeBlock Protocol
> > +    Status = gMmst->MmInstallProtocolInterface (
> > +                      &Instance->Handle,
> > +                      &gEfiSmmFirmwareVolumeBlockProtocolGuid,
> > +                      EFI_NATIVE_INTERFACE,
> > +                      &Instance->FvbProtocol
> > +                      );
> > +    if (EFI_ERROR(Status)) {
> > +      FreePool (Instance);
> > +      return Status;
> > +    }
> > +  } else {
> > +    //Install DevicePath Protocol
> > +    Status = gMmst->MmInstallProtocolInterface (
> > +                      &Instance->Handle,
> > +                      &gEfiDevicePathProtocolGuid,
>
> or this
>
> > +                      EFI_NATIVE_INTERFACE,
> > +                      &Instance->DevicePath
> > +                      );
> > +    if (EFI_ERROR(Status)) {
> > +      FreePool (Instance);
> > +      return Status;
> > +    }
> > +    //Install BlockIo Protocol
> > +    Status = gMmst->MmInstallProtocolInterface (
> > +                      &Instance->Handle,
> > +                      &gEfiBlockIoProtocolGuid,
>
> or this
>
>
> > +                      EFI_NATIVE_INTERFACE,
> > +                      &Instance->BlockIoProtocol
> > +                      );
> > +    if (EFI_ERROR(Status)) {
> > +      FreePool (Instance);
> > +      return Status;
> > +    }
> > +    //Install DiskIO Protocol
> > +    Status = gMmst->MmInstallProtocolInterface (
> > +                      &Instance->Handle,
> > +                      &gEfiDiskIoProtocolGuid,
>
> or this
>
> > +                      EFI_NATIVE_INTERFACE,
> > +                      &Instance->DiskIoProtocol
> > +                      );
> > +    if (EFI_ERROR(Status)) {
> > +      FreePool (Instance);
> > +      return Status;
> > +    }
> > +  }
> > +
> > +  *NorFlashInstance = Instance;
> > +  return Status;
> > +}
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +NorFlashInitialise (
> > +  IN EFI_HANDLE         ImageHandle,
> > +  IN EFI_MM_SYSTEM_TABLE   *SystemTable
> > +  )
> > +{
> > +  EFI_STATUS              Status;
> > +  UINT32                  Index;
> > +  NOR_FLASH_DESCRIPTION*  NorFlashDevices;
> > +  BOOLEAN                 ContainVariableStorage;
> > +
> > +  Status = NorFlashPlatformInitialization ();
> > +  if (EFI_ERROR(Status)) {
> > +    DEBUG((EFI_D_ERROR,"NorFlashInitialise: Fail to initialize Nor Flash devices\n"));
>
> Please use DEBUG_ERROR, and use %a and __FUNCTION__ for the function name
>
> > +    return Status;
> > +  }
> > +
> > +  Status = NorFlashPlatformGetDevices (&NorFlashDevices, &mNorFlashDeviceCount);
> > +  if (EFI_ERROR(Status)) {
> > +    DEBUG((EFI_D_ERROR,"NorFlashInitialise: Fail to get Nor Flash devices\n"));
>
> same here
>
> > +    return Status;
> > +  }
> > +
> > +  mNorFlashInstances = AllocateRuntimePool (sizeof(NOR_FLASH_INSTANCE*) * mNorFlashDeviceCount);
>
> no runtime
>
> > +
> > +  for (Index = 0; Index < mNorFlashDeviceCount; Index++) {
> > +    // Check if this NOR Flash device contain the variable storage region
> > +    ContainVariableStorage =
> > +        (NorFlashDevices[Index].RegionBaseAddress <= PcdGet32 (PcdFlashNvStorageVariableBase)) &&
> > +        (PcdGet32 (PcdFlashNvStorageVariableBase) + PcdGet32 (PcdFlashNvStorageVariableSize) <= NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size);
>
> Wrap to 80 columns
>
> > +
> > +    Status = NorFlashCreateInstance (
> > +      NorFlashDevices[Index].DeviceBaseAddress,
> > +      NorFlashDevices[Index].RegionBaseAddress,
> > +      NorFlashDevices[Index].Size,
> > +      Index,
> > +      NorFlashDevices[Index].BlockSize,
> > +      ContainVariableStorage,
> > +      &mNorFlashInstances[Index]
> > +    );
> > +    if (EFI_ERROR(Status)) {
> > +      DEBUG((EFI_D_ERROR,"NorFlashInitialise: Fail to create instance for NorFlash[%d]\n",Index));
>
> Please use DEBUG_ERROR, and use %a and __FUNCTION__ for the function name
>
> > +    }
> > +  }
> > +  return Status;
> > +}
> > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf
> > new file mode 100644
> > index 0000000..b189220
> > --- /dev/null
> > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf
> > @@ -0,0 +1,77 @@
> > +#/** @file
> > +#
> > +#  Component description file for NorFlashStandaloneMm module
> > +#
> > +#  Copyright (c) 2019, ARM 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.
> > +#
> > +#**/
> > +
> > +[Defines]
> > +  INF_VERSION                    = 0x00010005
>
> Please use the latest version (0x0001001B IIRC)
>
> > +  BASE_NAME                      = ArmVeNorFlashDxe
>
> Please choose a different name
>
> > +  FILE_GUID                      = 166F677B-DAC9-4AE4-AD34-2FF2504B0637
> > +  MODULE_TYPE                    = MM_STANDALONE
> > +  VERSION_STRING                 = 1.0
> > +  PI_SPECIFICATION_VERSION       = 0x00010032
> > +  ENTRY_POINT                    = NorFlashInitialise
> > +
> > +[Sources.common]
> > +  NorFlash.c
> > +  NorFlashFvb.c
> > +  NorFlashBlockIo.c
> > +  NorFlashStandaloneMm.c
>
> Please order alphabetically
>
> > +
> > +[Packages]
> > +  MdePkg/MdePkg.dec
> > +  MdeModulePkg/MdeModulePkg.dec
> > +  ArmPlatformPkg/ArmPlatformPkg.dec
> > +  EmbeddedPkg/EmbeddedPkg.dec
> > +  ArmPkg/ArmPkg.dec
> > +  StandaloneMmPkg/StandaloneMmPkg.dec
> > +
>
> Same here
>
> > +[LibraryClasses]
> > +  StandaloneMmDriverEntryPoint
> > +  BaseMemoryLib
> > +  ArmSvcLib
> > +  ArmLib
> > +  IoLib
> > +  BaseLib
> > +  DebugLib
> > +  HobLib
> > +  MemoryAllocationLib
> > +  NorFlashPlatformLib
> > +  MmServicesTableLib
> > +
>
> Do we really need all of these?
>
> > +[Guids]
> > +  gEfiSystemNvDataFvGuid
> > +  gEfiVariableGuid
> > +  gEfiAuthenticatedVariableGuid
> > +  gEfiEventVirtualAddressChangeGuid
> > +  gEdkiiNvVarStoreFormattedGuid     ## PRODUCES ## PROTOCOL
> > +
>
> Same here
>
> > +[Protocols]
> > +  gEfiBlockIoProtocolGuid
> > +  gEfiDevicePathProtocolGuid
> > +  gEfiSmmFirmwareVolumeBlockProtocolGuid
> > +  gEfiDiskIoProtocolGuid
> > +
>
> and here
>
> > +[Pcd.common]
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
> > +
> > +  gArmPlatformTokenSpaceGuid.PcdNorFlashCheckBlockLocked
> > +
> > +[Depex]
> > +  TRUE
> > --
> > 2.7.4
> >
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel