On Tue, Feb 19, 2019 at 10:14 PM Ard Biesheuvel <ard.biesheu...@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.u...@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.u...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel