From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::143; helo=mail-it1-x143.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x143.google.com (mail-it1-x143.google.com [IPv6:2607:f8b0:4864:20::143]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 5529C208AE98F for ; Tue, 19 Feb 2019 08:44:13 -0800 (PST) Received: by mail-it1-x143.google.com with SMTP id f10so7553954ita.4 for ; Tue, 19 Feb 2019 08:44:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=k0on1QvGCQYoCCkOU1FwfABZePt8MpNbspb09xszrfk=; b=dL4zpbs1gRPtI6LDcNgVnEXWzVbjjMOJRS1vM82fNWbQqTtCWfSD4mgOWF7eRXtB9y ztPRaFIZlbGyCZM/HkEMy+E00dIQ38bIJSg3eFvdfHhVkEzsTDKRZUjwkKzfQ80UcVRX X0Y9F6ZlKssQ9fbHweV8RDfG68UUUB8TBcvcPUUCF8frpowE8g4rQyJT7zGLlYnAgB5l 4q3ldN9UXNgOSOb7pka30N/OhjaWcQgRtLzc94wiLaZ6oyhJhn/QuoysmnntZyRV4uoT Fq0/CTCRxoXdLQaJT8ns49JE1sX5S++C22kXQB8XQ4arsHtzuc1k+wjj8o/ZsQtmMuyY h8vg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=k0on1QvGCQYoCCkOU1FwfABZePt8MpNbspb09xszrfk=; b=r69yeEc34waan918JWpGhNIljj91x58B+Zv7POzsk+PLXdVgVbzUvNv6kVPhS7YxOZ JMphME17grnwHFdIwY10CEjem3OKUMnMTLC3WXLuGiAjr/nmlFPX7iZUdyJ/CfFLyihF m2OlYuHZxWmMhDibLHEFTiq2pw+0MtITPsGKXwGZogBVCvxDebzzv6Ib0QMP2h2BC/FE l0VSOmY5mlYSkSkNMN9TMo5Zc2fGLs9EF4qSJ9NmcxrME71LANy65+pYgqPDKjtvLrZB oamQ5cQ0F/G7K5JxrKwIT44bMKJ9TgxBPGPjn9kxNX1Uwy+NMOIxIxwqBk84xK7yZegJ etiQ== X-Gm-Message-State: AHQUAuazkjU40yOLuYpVGg0CPyPAdIDvS16X+PBvdoPapQhdxoXy831u 2Hw/ehz44OiujwFq82VvglbbhzWftBtZXcZ520lBoA== X-Google-Smtp-Source: AHgI3IZwyO8Km9e9F6UIyRx5RUhk2apsghwkeYRp8aLILkgarqtW2yfzjV9XBGOnx/SCg5JGjx3DQ6vIW+/qHiOFxb4= X-Received: by 2002:a24:45dd:: with SMTP id c90mr2947248itd.71.1550594652409; Tue, 19 Feb 2019 08:44:12 -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: <1550571334-29663-3-git-send-email-jagadeesh.ujja@arm.com> From: Ard Biesheuvel Date: Tue, 19 Feb 2019 17:43:59 +0100 Message-ID: To: Jagadeesh Ujja Cc: "edk2-devel@lists.01.org" , "Gao, Liming" , "Zhang, Chao B" , Leif Lindholm 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: Tue, 19 Feb 2019 16:44:13 -0000 Content-Type: text/plain; charset="UTF-8" Hello 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 >