From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=217.140.101.70; helo=foss.arm.com; envelope-from=jagadeesh.ujja@arm.com; receiver=edk2-devel@lists.01.org Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id 4585C208F6129 for ; Tue, 19 Feb 2019 21:24:07 -0800 (PST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 8B18A1650 for ; Tue, 19 Feb 2019 21:24:07 -0800 (PST) Received: from mail-it1-f177.google.com (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 65D893F8C6 for ; Tue, 19 Feb 2019 21:24:07 -0800 (PST) Received: by mail-it1-f177.google.com with SMTP id f10so12446004ita.4 for ; Tue, 19 Feb 2019 21:24:07 -0800 (PST) X-Gm-Message-State: AHQUAuZvoDwldR5dZRVPYenwNu4d8c+Xw1LuvudmLdlbeFZXF71QqeOD Su/EVom4R6vPNRGb2ev/gb37yLZxBxvY5Fx/yHU= X-Google-Smtp-Source: AHgI3IaPTC/27gSY/zpkDn6ll+q5GZ5dnBr1HkH/dGs4s6TLsE5p7fAdfwOFpj/rA2px/ezBPGDwWRgFoWRe9m2ta/Q= X-Received: by 2002:a02:4084:: with SMTP id n126mr12708365jaa.78.1550640246508; Tue, 19 Feb 2019 21:24:06 -0800 (PST) MIME-Version: 1.0 References: <1550571334-29663-1-git-send-email-jagadeesh.ujja@arm.com> <1550571334-29663-3-git-send-email-jagadeesh.ujja@arm.com> In-Reply-To: From: Jagadeesh Ujja Date: Wed, 20 Feb 2019 10:53:55 +0530 X-Gmail-Original-Message-ID: Message-ID: To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , "Zhang, Chao B" , "Gao, Liming" Subject: Re: [PATCH 2/2] ArmPlatformPkg/NorFlash: Allow reusability as a MM driver 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: Wed, 20 Feb 2019 05:24:08 -0000 Content-Type: text/plain; charset="UTF-8" On Tue, Feb 19, 2019 at 10:14 PM Ard Biesheuvel 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 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 > > --- > > 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.
> > + > > + 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > 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 > > +#include > > +#include > > 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