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

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

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