From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web12.8470.1593085148678510985 for ; Thu, 25 Jun 2020 04:39:08 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 5D88C1FB; Thu, 25 Jun 2020 04:39:08 -0700 (PDT) Received: from [192.168.43.142] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 945A63F73C; Thu, 25 Jun 2020 04:39:06 -0700 (PDT) Subject: Re: [PATCH v3 06/15] ArmVirtPkg: Add Kvmtool NOR flash lib To: Sami Mujawar , devel@edk2.groups.io Cc: leif@nuviainc.com, philmd@redhat.com, lersek@redhat.com, Alexandru.Elisei@arm.com, Andre.Przywara@arm.com, Matteo.Carlini@arm.com, Laura.Moretta@arm.com, nd@arm.com References: <20200624133458.61920-1-sami.mujawar@arm.com> <20200624133458.61920-7-sami.mujawar@arm.com> From: "Ard Biesheuvel" Message-ID: Date: Thu, 25 Jun 2020 13:38:57 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200624133458.61920-7-sami.mujawar@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 6/24/20 3:34 PM, Sami Mujawar wrote: > Kvmtool places the base address of the CFI flash in > the device tree it passes to UEFI. This library > parses the kvmtool device tree to read the CFI base > address and initialise the PCDs use by the NOR flash > driver and the variable storage. > > UEFI takes ownership of the CFI flash hardware, and > exposes its functionality through the UEFI Runtime > Variable Service. Therefore, disable the device tree > node for the CFI flash used for storing the UEFI > variables, to prevent the OS from attaching its device > driver as well. > > Signed-off-by: Sami Mujawar > Acked-by: Laszlo Ersek > --- > > Notes: > v3: > - ASSERT is sufficient to test Locating [Ard] > gFdtClientProtocolGuid as DEPEX ensures that this is > guaranteed to succeed. > - Removed additional error handling based on review [Sami] > feedback. > - Fix confusion caused by use of macro MAX_FLASH_BANKS. [Philippe] > - Renamed MAX_FLASH_BANKS to MAX_FLASH_DEVICES. [Sami] > - Use macro to define block size for flash. [Philippe] > - Defined macro KVMTOOL_NOR_BLOCK_SIZE and also configured [Sami] > to reflect the correct block size 64KB. > - Disable the DT flash node used for UEFI variable storage [Sami] > as UEFI takes ownership of the flash device. > Ref: https://edk2.groups.io/g/devel/topic/74200914#60341 > > v2: > - Library to read CFI flash base address from DT and initialise [Sami] > PCDs used for NOR flash variables. > > ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c | 330 ++++++++++++++++++++ > ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf | 49 +++ > 2 files changed, 379 insertions(+) > > diff --git a/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c > new file mode 100644 > index 0000000000000000000000000000000000000000..8e9dcf31691b4b12b9c7bac1ad4ba8d3a534a1d8 > --- /dev/null > +++ b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c > @@ -0,0 +1,330 @@ > +/** @file > + An instance of the NorFlashPlatformLib for Kvmtool platform. > + > + Copyright (c) 2020, ARM Ltd. All rights reserved.
> + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > + **/ > + > +#include > +#include > +#include > +#include > +#include > + > +/** Macro defining the NOR block size configured in Kvmtool. > +*/ > +#define KVMTOOL_NOR_BLOCK_SIZE SIZE_64KB > + > +/** Macro defining the maximum number of Flash devices. > +*/ > +#define MAX_FLASH_DEVICES 4 > + > +/** Macro defining the cfi-flash label describing the UEFI variable store. > +*/ > +#define LABEL_UEFI_VAR_STORE "System-firmware" > + > +STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[MAX_FLASH_DEVICES]; > +STATIC UINTN mNorFlashDeviceCount = 0; > +STATIC INT32 mUefiVarStoreNode = MAX_INT32; > +STATIC FDT_CLIENT_PROTOCOL *mFdtClient; > + > +/** This function performs platform specific actions to initialise > + the NOR flash, if required. > + > + @retval EFI_SUCCESS Success. > +**/ > +EFI_STATUS > +NorFlashPlatformInitialization ( > + VOID > + ) > +{ > + EFI_STATUS Status; > + > + DEBUG ((DEBUG_INFO, "NorFlashPlatformInitialization\n")); > + > + if ((mNorFlashDeviceCount > 0) && (mUefiVarStoreNode != MAX_INT32)) { > + // > + // UEFI takes ownership of the cfi-flash hardware, and exposes its > + // functionality through the UEFI Runtime Variable Service. This means we > + // need to disable it in the device tree to prevent the OS from attaching > + // its device driver as well. > + // Note: This library is loaded by the FaultTolerantWriteDxe to setup the > + // Ftw PCDs and later by the NorFlashDxe to provide the NorFlashPlatformLib > + // interfaces. Therefore the FDT node used for UEFI storage variable is > + // disabled here. > + // > + Status = mFdtClient->SetNodeProperty ( > + mFdtClient, > + mUefiVarStoreNode, > + "status", > + "disabled", > + sizeof ("disabled") > + ); Can you explain why this action is deferred to NorFlashPlatformInitialization()? > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_WARN, "Failed to set cfi-flash status to 'disabled'\n")); > + } > + } else { > + Status = EFI_NOT_FOUND; > + DEBUG ((DEBUG_ERROR, "Flash device for UEFI variable storage not found\n")); > + } > + > + return Status; > +} > + > +/** Initialise Non volatile Flash storage variables. > + > + @param [in] FlashDevice Pointer to the NOR Flash device. > + > + @retval EFI_SUCCESS Success. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_OUT_OF_RESOURCES Insufficient flash storage space. > +**/ STATIC > +EFI_STATUS > +SetupVariableStore ( > + IN NOR_FLASH_DESCRIPTION * FlashDevice > + ) > +{ > + UINTN FlashRegion; > + UINTN FlashNvStorageVariableBase; > + UINTN FlashNvStorageFtwWorkingBase; > + UINTN FlashNvStorageFtwSpareBase; > + UINTN FlashNvStorageVariableSize; > + UINTN FlashNvStorageFtwWorkingSize; > + UINTN FlashNvStorageFtwSpareSize; > + > + FlashNvStorageVariableSize = PcdGet32 (PcdFlashNvStorageVariableSize); > + FlashNvStorageFtwWorkingSize = PcdGet32 (PcdFlashNvStorageFtwWorkingSize); > + FlashNvStorageFtwSpareSize = PcdGet32 (PcdFlashNvStorageFtwSpareSize); > + > + if ((FlashNvStorageVariableSize == 0) || > + (FlashNvStorageFtwWorkingSize == 0) || > + (FlashNvStorageFtwSpareSize == 0)) { > + DEBUG ((DEBUG_ERROR, "FlashNvStorage size not defined\n")); > + return EFI_INVALID_PARAMETER; > + } > + > + // Setup the variable store > + FlashRegion = FlashDevice->DeviceBaseAddress; > + > + FlashNvStorageVariableBase = FlashRegion; > + FlashRegion += PcdGet32 (PcdFlashNvStorageVariableSize); > + > + FlashNvStorageFtwWorkingBase = FlashRegion; > + FlashRegion += PcdGet32 (PcdFlashNvStorageFtwWorkingSize); > + > + FlashNvStorageFtwSpareBase = FlashRegion; > + FlashRegion += PcdGet32 (PcdFlashNvStorageFtwSpareSize); > + > + if (FlashRegion > (FlashDevice->DeviceBaseAddress + FlashDevice->Size)) { > + DEBUG ((DEBUG_ERROR, "Insufficient flash storage size\n")); > + return EFI_OUT_OF_RESOURCES; > + } > + > + PcdSet32S ( > + PcdFlashNvStorageVariableBase, > + FlashNvStorageVariableBase > + ); > + > + PcdSet32S ( > + PcdFlashNvStorageFtwWorkingBase, > + FlashNvStorageFtwWorkingBase > + ); > + > + PcdSet32S ( > + PcdFlashNvStorageFtwSpareBase, > + FlashNvStorageFtwSpareBase > + ); > + > + DEBUG (( > + DEBUG_INFO, > + "PcdFlashNvStorageVariableBase = 0x%x\n", > + FlashNvStorageVariableBase > + )); > + DEBUG (( > + DEBUG_INFO, > + "PcdFlashNvStorageVariableSize = 0x%x\n", > + FlashNvStorageVariableSize > + )); > + DEBUG (( > + DEBUG_INFO, > + "PcdFlashNvStorageFtwWorkingBase = 0x%x\n", > + FlashNvStorageFtwWorkingBase > + )); > + DEBUG (( > + DEBUG_INFO, > + "PcdFlashNvStorageFtwWorkingSize = 0x%x\n", > + FlashNvStorageFtwWorkingSize > + )); > + DEBUG (( > + DEBUG_INFO, > + "PcdFlashNvStorageFtwSpareBase = 0x%x\n", > + FlashNvStorageFtwSpareBase > + )); > + DEBUG (( > + DEBUG_INFO, > + "PcdFlashNvStorageFtwSpareSize = 0x%x\n", > + FlashNvStorageFtwSpareSize > + )); > + > + return EFI_SUCCESS; > +} > + > +/** Return the Flash devices on the platform. > + > + @param [out] NorFlashDescriptions Pointer to the Flash device description. > + @param [out] Count Number of Flash devices. > + > + @retval EFI_SUCCESS Success. > + @retval EFI_NOT_FOUND Flash device not found. > +**/ > +EFI_STATUS > +NorFlashPlatformGetDevices ( > + OUT NOR_FLASH_DESCRIPTION **NorFlashDescriptions, > + OUT UINT32 *Count > + ) > +{ > + if (mNorFlashDeviceCount > 0) { > + *NorFlashDescriptions = mNorFlashDevices; > + *Count = mNorFlashDeviceCount; > + return EFI_SUCCESS; > + } > + return EFI_NOT_FOUND; > +} > + > +/** Entrypoint for NorFlashPlatformLib. > + > + @param [in] ImageHandle The handle to the image. > + @param [in] SystemTable Pointer to the System Table. > + > + @retval EFI_SUCCESS Success. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_NOT_FOUND Flash device not found. > +**/ > +EFI_STATUS > +EFIAPI > +NorFlashPlatformLibConstructor ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE * SystemTable > + ) > +{ > + INT32 Node; > + EFI_STATUS Status; > + EFI_STATUS FindNodeStatus; > + CONST UINT32 *Reg; > + UINT32 PropSize; > + UINT64 Base; > + UINT64 Size; > + UINTN UefiVarStoreIndex; > + CONST CHAR8 *Label; > + UINT32 LabelLen; > + > + if (mNorFlashDeviceCount != 0) { > + return EFI_SUCCESS; > + } > + > + Status = gBS->LocateProtocol ( > + &gFdtClientProtocolGuid, > + NULL, > + (VOID **)&mFdtClient > + ); > + ASSERT_EFI_ERROR (Status); > + > + UefiVarStoreIndex = MAX_UINTN; > + for (FindNodeStatus = mFdtClient->FindCompatibleNode ( > + mFdtClient, > + "cfi-flash", > + &Node > + ); > + !EFI_ERROR (FindNodeStatus) && > + (mNorFlashDeviceCount < MAX_FLASH_DEVICES); > + FindNodeStatus = mFdtClient->FindNextCompatibleNode ( > + mFdtClient, > + "cfi-flash", > + Node, > + &Node > + )) { > + Status = mFdtClient->GetNodeProperty ( > + mFdtClient, > + Node, > + "label", > + (CONST VOID **)&Label, > + &LabelLen > + ); > + if (EFI_ERROR (Status)) { > + DEBUG (( > + DEBUG_ERROR, > + "%a: GetNodeProperty ('label') failed (Status == %r)\n", > + __FUNCTION__, > + Status > + )); > + } else if (AsciiStrCmp (Label, LABEL_UEFI_VAR_STORE) == 0) { > + UefiVarStoreIndex = mNorFlashDeviceCount; > + mUefiVarStoreNode = Node; > + } > + > + Status = mFdtClient->GetNodeProperty ( > + mFdtClient, > + Node, > + "reg", > + (CONST VOID **)&Reg, > + &PropSize > + ); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: GetNodeProperty () failed (Status == %r)\n", > + __FUNCTION__, Status)); > + continue; > + } > + > + ASSERT ((PropSize % (4 * sizeof (UINT32))) == 0); > + > + while ((PropSize >= (4 * sizeof (UINT32))) && > + (mNorFlashDeviceCount < MAX_FLASH_DEVICES)) { > + Base = SwapBytes64 (ReadUnaligned64 ((VOID *)&Reg[0])); > + Size = SwapBytes64 (ReadUnaligned64 ((VOID *)&Reg[2])); > + Reg += 4; > + > + PropSize -= 4 * sizeof (UINT32); > + > + // > + // Disregard any flash devices that overlap with the primary FV. > + // The firmware is not updatable from inside the guest anyway. > + // > + if ((PcdGet64 (PcdFvBaseAddress) + PcdGet32 (PcdFvSize) > Base) && > + (Base + Size) > PcdGet64 (PcdFvBaseAddress)) { > + continue; > + } > + > + DEBUG (( > + DEBUG_INFO, > + "NOR%d : Base = 0x%lx, Size = 0x%lx\n", > + mNorFlashDeviceCount, > + Base, > + Size > + )); > + > + mNorFlashDevices[mNorFlashDeviceCount].DeviceBaseAddress = (UINTN)Base; > + mNorFlashDevices[mNorFlashDeviceCount].RegionBaseAddress = (UINTN)Base; > + mNorFlashDevices[mNorFlashDeviceCount].Size = (UINTN)Size; > + mNorFlashDevices[mNorFlashDeviceCount].BlockSize = KVMTOOL_NOR_BLOCK_SIZE; > + mNorFlashDeviceCount++; > + } > + } // for > + > + // Setup the variable store in the last device > + if (mNorFlashDeviceCount > 0) { > + if (UefiVarStoreIndex == MAX_UINTN) { > + // We did not find a label matching the UEFI Variable store. Default to > + // using the last cfi-flash device as the variable store. > + UefiVarStoreIndex = mNorFlashDeviceCount - 1; > + mUefiVarStoreNode = Node; > + } > + if (mNorFlashDevices[UefiVarStoreIndex].DeviceBaseAddress != 0) { > + return SetupVariableStore (&mNorFlashDevices[UefiVarStoreIndex]); > + } > + } > + > + return EFI_NOT_FOUND; > +} > + > diff --git a/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf > new file mode 100644 > index 0000000000000000000000000000000000000000..a3230e4b2be668904322103825b93e867503984e > --- /dev/null > +++ b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf > @@ -0,0 +1,49 @@ > +#/** @file > +# > +# Copyright (c) 2020, ARM Ltd. All rights reserved.
> +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +#**/ > + > +[Defines] > + INF_VERSION = 0x0001001B > + BASE_NAME = NorFlashKvmtoolLib > + FILE_GUID = E75F07A1-B160-4893-BDD4-09E32FF847DC > + MODULE_TYPE = DXE_DRIVER > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = NorFlashPlatformLib > + CONSTRUCTOR = NorFlashPlatformLibConstructor > + > +[Sources.common] > + NorFlashKvmtool.c > + > +[Packages] > + ArmPkg/ArmPkg.dec > + ArmPlatformPkg/ArmPlatformPkg.dec > + ArmVirtPkg/ArmVirtPkg.dec > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + > +[LibraryClasses] > + BaseLib > + DebugLib > + PcdLib > + UefiBootServicesTableLib > + > +[Protocols] > + gFdtClientProtocolGuid ## CONSUMES > + > +[Pcd] > + gArmTokenSpaceGuid.PcdFvBaseAddress > + gArmTokenSpaceGuid.PcdFvSize > + > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize > + > +[Depex] > + gFdtClientProtocolGuid > + >