* [PATCH edk2-platforms v3 0/2] Add support for running StandaloneMm as OP-TEE TA @ 2020-12-16 11:09 Sughosh Ganu 2020-12-16 11:09 ` [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver Sughosh Ganu 2020-12-16 11:09 ` [PATCH edk2-platforms v3 2/2] StMMRpmb: Add support for building StandaloneMm image for OP-TEE Sughosh Ganu 0 siblings, 2 replies; 14+ messages in thread From: Sughosh Ganu @ 2020-12-16 11:09 UTC (permalink / raw) To: devel Cc: Sami Mujawar, Ard Biesheuvel, Leif Lindholm, Sahil Malhotra, Ilias Apalodimas This patch series is adding a platform definition for compiling StMM as a flash image, which we can run from OP-TEE. SPM (responsible for dispatching StMM) and SPD (for OP-TEE) are mutually exclusive and there's no Trusted Application in OP-TEE for managing EFI variables (only a Microsoft one, for Authenticated variables). This means that one can have a secure OS or secure variable storage. With some recent changes merged in OP-TEE [1] and U-Boot [2] we can launch StMM from an OP-TEE secure partition which is mimicking SPM. By re-using StMM we have EDK2s approved application controlling variable storage and the ability to run a secure world OS. This also allows various firmware implementations to adopt EDK2 way of storing variables (including the FTW implementation), as long as OP-TEE is available on that given platform (or any other secure OS that can launch StMM and has a supplicant for handling the RPMB partition). Another advantage is that OP-TEE has the ability to access an eMMC RPMB partition to store those variables, so any platform with OP-TEE and an eMMC can store variables securely. This requires a normal world supplicant, which is implemented in U-Boot currently. Similar functionality can be added in EDK2 by porting the supplicant and adapt it to using the native eMMC drivers. Although this approach might seem counter-intuitive at first glance, considering the FFA [3] in Arm architecture, using a Secure Partition that includes everything seems like a better choice at the moment and is preferred over a rewritten from scratch TA. There's is one drawback in using OP-TEE. The current SPM calls need to run to completion. This contradicts the current OP-TEE RPC call requirements, used to access the RPMB storage. Thats leads to two different SMC calls for entering secure world to access StMM (one for SPM and one for SPD). Since this is quite tricky to compile and test you can use this [4]. Just clone the repo and run ./build.sh. The script will pick up edk2, edk2-platforms, op-tee, TF-A and U-boot and compile all the necessary binaries for QEMU. A patch (hack) has been added to U-boot to allow RPMB emulation through it's supplicant, since QEMU RPMB emulation is not yet available. After compiling and launching QEMU the usual U-boot commands for EFI variable management will store the variables on the emulated RPMB device. [1] https://github.com/OP-TEE/optee_os/pull/3973 [2] http://u-boot.10912.n7.nabble.com/PATCH-0-7-v4-EFI-variable-support-via-OP-TEE-td412499.html [3] https://developer.arm.com/documentation/den0077/a [4] https://git.linaro.org/people/ilias.apalodimas/efi_optee_variables.git/ Changes since V2: - Allocate a dynamic number of pages based on the Pcd values instead of a static number - Clean up unused structs in header file - Added checks in OpTeeRpmbFvbGetBlockSize and handle NumLba=0 Changes since V1: Some enhancements made by Ilias to the Optee Rpmb driver This series is to be reviewed along with V2 of the patch series for enablement of Firmware Framework(FF-A)[1] [1] - https://edk2.groups.io/g/devel/message/68766 Ilias Apalodimas (2): Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver StMMRpmb: Add support for building StandaloneMm image for OP-TEE Platform/StMMRpmb/PlatformStandaloneMm.dsc | 168 ++++ Platform/StMMRpmb/PlatformStandaloneMm.fdf | 111 +++ Drivers/OpTeeRpmb/FixupPcd.inf | 44 ++ Drivers/OpTeeRpmb/OpTeeRpmbFv.inf | 58 ++ Drivers/OpTeeRpmb/OpTeeRpmbFvb.h | 35 + Drivers/OpTeeRpmb/FixupPcd.c | 74 ++ Drivers/OpTeeRpmb/OpTeeRpmbFvb.c | 803 ++++++++++++++++++++ 7 files changed, 1293 insertions(+) create mode 100644 Platform/StMMRpmb/PlatformStandaloneMm.dsc create mode 100644 Platform/StMMRpmb/PlatformStandaloneMm.fdf create mode 100644 Drivers/OpTeeRpmb/FixupPcd.inf create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFv.inf create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFvb.h create mode 100644 Drivers/OpTeeRpmb/FixupPcd.c create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFvb.c -- 2.17.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver 2020-12-16 11:09 [PATCH edk2-platforms v3 0/2] Add support for running StandaloneMm as OP-TEE TA Sughosh Ganu @ 2020-12-16 11:09 ` Sughosh Ganu 2021-01-27 17:10 ` Sami Mujawar 2020-12-16 11:09 ` [PATCH edk2-platforms v3 2/2] StMMRpmb: Add support for building StandaloneMm image for OP-TEE Sughosh Ganu 1 sibling, 1 reply; 14+ messages in thread From: Sughosh Ganu @ 2020-12-16 11:09 UTC (permalink / raw) To: devel Cc: Sami Mujawar, Ard Biesheuvel, Leif Lindholm, Sahil Malhotra, Ilias Apalodimas From: Ilias Apalodimas <ilias.apalodimas@linaro.org> A following patch is adding support for building StMM in order to run it from OP-TEE. OP-TEE in combination with a NS-world supplicant can use the RPMB partition of an eMMC to store EFI variables. The supplicant functionality is currently available in U-Boot only but can be ported into EDK2. Assuming similar functionality is added in EDK2, this will allow any hardware with an RPMB partition to store EFI variables securely. So let's add a driver that enables access of the RPMB partition through OP-TEE. Since the upper layers expect a byte addressable interface, the driver allocates memory and patches the PCDs, while syncing the memory/hardware on read/write callbacks. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- Changes since V2: - Allocate a dynamic number of pages based on the Pcd values instead of a static number - Clean up unused structs in header file - Added checks in OpTeeRpmbFvbGetBlockSize and handle NumLba=0 Drivers/OpTeeRpmb/FixupPcd.inf | 44 ++ Drivers/OpTeeRpmb/OpTeeRpmbFv.inf | 58 ++ Drivers/OpTeeRpmb/OpTeeRpmbFvb.h | 35 + Drivers/OpTeeRpmb/FixupPcd.c | 74 ++ Drivers/OpTeeRpmb/OpTeeRpmbFvb.c | 803 ++++++++++++++++++++ 5 files changed, 1014 insertions(+) diff --git a/Drivers/OpTeeRpmb/FixupPcd.inf b/Drivers/OpTeeRpmb/FixupPcd.inf new file mode 100644 index 0000000000..f0cfdf7a4c --- /dev/null +++ b/Drivers/OpTeeRpmb/FixupPcd.inf @@ -0,0 +1,44 @@ +## @file +# Instance of Base Memory Library without assembly. +# +# Copyright (c) 2020, Linaro Ltd. All rights reserved.<BR> +# +# SPDX-License-Identifier: BSD-2-Clause-Patent +# +# +## + +[Defines] + INF_VERSION = 0x0001001A + BASE_NAME = FixupPcd + FILE_GUID = a827c337-a9c6-301b-aeb7-acbc95d8da22 + MODULE_TYPE = BASE + VERSION_STRING = 0.1 + LIBRARY_CLASS = RpmbPcdFixup|MM_STANDALONE + CONSTRUCTOR = FixPcdMemory + +[Sources] + FixupPcd.c + OpTeeRpmbFvb.h + +[Packages] + MdeModulePkg/MdeModulePkg.dec + MdePkg/MdePkg.dec + +[LibraryClasses] + BaseLib + DebugLib + MmServicesTableLib + PcdLib + +[Pcd] + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize + + +[Protocols] + gEfiSmmFirmwareVolumeBlockProtocolGuid ## CONSUMES diff --git a/Drivers/OpTeeRpmb/OpTeeRpmbFv.inf b/Drivers/OpTeeRpmb/OpTeeRpmbFv.inf new file mode 100644 index 0000000000..b21f7397e5 --- /dev/null +++ b/Drivers/OpTeeRpmb/OpTeeRpmbFv.inf @@ -0,0 +1,58 @@ +## @file +# +# Component description file for OpTeeRpmbFv module +# +# Copyright (c) 2020, Linaro Ltd. All rights reserved.<BR> +# +# SPDX-License-Identifier: BSD-2-Clause-Patent +# +## + +[Defines] + INF_VERSION = 0x0001001A + BASE_NAME = OpTeeRpmbFv + FILE_GUID = 4803FC20-E583-3BCD-8C60-141E85C9A2CF + MODULE_TYPE = MM_STANDALONE + VERSION_STRING = 0.1 + PI_SPECIFICATION_VERSION = 0x00010032 + ENTRY_POINT = OpTeeRpmbFvbInit + +[Sources] + OpTeeRpmbFvb.c + OpTeeRpmbFvb.h + +[Packages] + ArmPkg/ArmPkg.dec + ArmPlatformPkg/ArmPlatformPkg.dec + MdeModulePkg/MdeModulePkg.dec + MdePkg/MdePkg.dec + StandaloneMmPkg/StandaloneMmPkg.dec + +[LibraryClasses] + ArmSvcLib + BaseLib + BaseMemoryLib + DebugLib + MemoryAllocationLib + MmServicesTableLib + PcdLib + StandaloneMmDriverEntryPoint + +[Guids] + gEfiAuthenticatedVariableGuid + gEfiSystemNvDataFvGuid + gEfiVariableGuid + +[Pcd] + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize + +[Protocols] + gEfiSmmFirmwareVolumeBlockProtocolGuid ## PRODUCES + +[Depex] + TRUE diff --git a/Drivers/OpTeeRpmb/OpTeeRpmbFvb.h b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.h new file mode 100644 index 0000000000..717fc4a78b --- /dev/null +++ b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.h @@ -0,0 +1,35 @@ +/** @file + + Copyright (c) 2020, Linaro Ltd. All rights reserved.<BR> + SPDX-License-Identifier: BSD-2-Clause-Patent + +**/ + +#ifndef __OPTEE_RPMB_FV_ +#define __OPTEE_RPMB_FV_ + +/* SVC Args */ +#define SP_SVC_RPMB_READ 0xC4000066 +#define SP_SVC_RPMB_WRITE 0xC4000067 + +#define FILENAME "EFI_VARS" + +#define FLASH_SIGNATURE SIGNATURE_32('r', 'p', 'm', 'b') +#define INSTANCE_FROM_FVB_THIS(a) CR(a, MEM_INSTANCE, FvbProtocol, \ + FLASH_SIGNATURE) + +typedef struct _MEM_INSTANCE MEM_INSTANCE; +typedef EFI_STATUS (*MEM_INITIALIZE) (MEM_INSTANCE* Instance); +struct _MEM_INSTANCE +{ + UINT32 Signature; + MEM_INITIALIZE Initialize; + BOOLEAN Initialized; + EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL FvbProtocol; + EFI_HANDLE Handle; + EFI_PHYSICAL_ADDRESS MemBaseAddress; + UINT16 BlockSize; + UINT16 NBlocks; +}; + +#endif diff --git a/Drivers/OpTeeRpmb/FixupPcd.c b/Drivers/OpTeeRpmb/FixupPcd.c new file mode 100644 index 0000000000..3cc882fa94 --- /dev/null +++ b/Drivers/OpTeeRpmb/FixupPcd.c @@ -0,0 +1,74 @@ +/** @file + + Update the patched PCDs to their correct value + + Copyright (c) 2020, Linaro Ltd. All rights reserved.<BR> + + SPDX-License-Identifier: BSD-2-Clause-Patent + +**/ + +/** + * Patch the relevant PCDs of the RPMB driver with the correct address of the + * allocated memory + * +**/ +#include <Library/BaseLib.h> +#include <Library/DebugLib.h> +#include <Library/MmServicesTableLib.h> +#include <Library/PcdLib.h> + +#include <Protocol/FirmwareVolumeBlock.h> +#include <Protocol/SmmFirmwareVolumeBlock.h> + +#include "OpTeeRpmbFvb.h" + +/** + Fixup the Pcd values for variable storage + + Since the upper layers of EDK2 expect a memory mapped interface and we can't + offer that from an RPMB, the driver allocates memory on init and passes that + on the upper layers. Since the memory is dynamically allocated and we can't set the + PCD is StMM context, we need to patch it correctly on each access + + @retval EFI_SUCCESS Protocol was found and PCDs patched up + + **/ +EFI_STATUS +EFIAPI +FixPcdMemory ( + VOID + ) +{ + EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *FvbProtocol; + MEM_INSTANCE *Instance; + EFI_STATUS Status; + + // + // Locate SmmFirmwareVolumeBlockProtocol + // + Status = gMmst->MmLocateProtocol ( + &gEfiSmmFirmwareVolumeBlockProtocolGuid, + NULL, + (VOID **) &FvbProtocol + ); + ASSERT_EFI_ERROR (Status); + + Instance = INSTANCE_FROM_FVB_THIS(FvbProtocol); + // Patch PCDs with the the correct values + PatchPcdSet32 (PcdFlashNvStorageVariableBase, Instance->MemBaseAddress); + PatchPcdSet32 (PcdFlashNvStorageFtwWorkingBase, Instance->MemBaseAddress + + PcdGet32 (PcdFlashNvStorageVariableSize)); + PatchPcdSet32 (PcdFlashNvStorageFtwSpareBase, Instance->MemBaseAddress + + PcdGet32 (PcdFlashNvStorageVariableSize) + + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)); + + DEBUG ((DEBUG_INFO, "%a: Fixup PcdFlashNvStorageVariableBase: 0x%lx\n", + __FUNCTION__, PcdGet32 (PcdFlashNvStorageVariableBase))); + DEBUG ((DEBUG_INFO, "%a: Fixup PcdFlashNvStorageFtwWorkingBase: 0x%lx\n", + __FUNCTION__, PcdGet32 (PcdFlashNvStorageFtwWorkingBase))); + DEBUG ((DEBUG_INFO, "%a: Fixup PcdFlashNvStorageFtwSpareBase: 0x%lx\n", + __FUNCTION__, PcdGet32 (PcdFlashNvStorageFtwSpareBase))); + + return Status; +} diff --git a/Drivers/OpTeeRpmb/OpTeeRpmbFvb.c b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.c new file mode 100644 index 0000000000..71bb1f33b6 --- /dev/null +++ b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.c @@ -0,0 +1,803 @@ +/** @file + + FV block I/O protocol driver for RPMB eMMC accessed via OP-TEE + + Copyright (c) 2020, Linaro Ltd. All rights reserved.<BR> + + SPDX-License-Identifier: BSD-2-Clause-Patent + +**/ + +#include <Library/ArmSvcLib.h> +#include <Library/BaseLib.h> +#include <Library/BaseMemoryLib.h> +#include <Library/DebugLib.h> +#include <Library/MemoryAllocationLib.h> +#include <Library/MmServicesTableLib.h> +#include <Library/PcdLib.h> + +#include <IndustryStandard/ArmFfaSvc.h> +#include <IndustryStandard/ArmMmSvc.h> +#include <Protocol/FirmwareVolumeBlock.h> +#include <Protocol/SmmFirmwareVolumeBlock.h> +#include <Guid/VariableFormat.h> + +#include "OpTeeRpmbFvb.h" + +static const UINT16 mem_mgr_id = 3U; +static const UINT16 storage_id = 4U; + +STATIC MEM_INSTANCE mInstance; + +/** + Sends an SVC call to OP-TEE for reading/writing an RPMB partition + + @param SvcAct SVC ID for read/write + @param Addr Base address of the buffer. When reading contents will be + copied to that buffer after reading them from the device. + When writing, the buffer holds the contents we want to + write cwtoin the device + @param NumBytes Number of bytes to read/write + @param Offset Offset into the RPMB file + + @retval OP-TEE return code +**/ + +STATIC +EFI_STATUS +ReadWriteRpmb ( + UINTN SvcAct, + UINTN Addr, + UINTN NumBytes, + UINTN Offset + ) +{ + ARM_SVC_ARGS SvcArgs; + EFI_STATUS Status; + + ZeroMem (&SvcArgs, sizeof (SvcArgs)); + + SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64; + SvcArgs.Arg1 = storage_id; + SvcArgs.Arg2 = 0; + SvcArgs.Arg3 = SvcAct; + SvcArgs.Arg4 = Addr; + SvcArgs.Arg5 = NumBytes; + SvcArgs.Arg6 = Offset; + + ArmCallSvc (&SvcArgs); + if (SvcArgs.Arg3) { + DEBUG ((DEBUG_ERROR, "%a: Svc Call 0x%08x Addr: 0x%08x len: 0x%x Offset: 0x%x failed with 0x%x\n", + __func__, SvcAct, Addr, NumBytes, Offset, SvcArgs.Arg3)); + } + + switch (SvcArgs.Arg3) { + case ARM_SVC_SPM_RET_SUCCESS: + Status = EFI_SUCCESS; + break; + + case ARM_SVC_SPM_RET_NOT_SUPPORTED: + Status = EFI_UNSUPPORTED; + break; + + case ARM_SVC_SPM_RET_INVALID_PARAMS: + Status = EFI_INVALID_PARAMETER; + break; + + case ARM_SVC_SPM_RET_DENIED: + Status = EFI_ACCESS_DENIED; + break; + + case ARM_SVC_SPM_RET_NO_MEMORY: + Status = EFI_BAD_BUFFER_SIZE; + break; + + default: + Status = EFI_ACCESS_DENIED; + } + + return Status; +} + +/** + The GetAttributes() function retrieves the attributes and + current settings of the block. + + @param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL instance. + + @param Attributes Pointer to EFI_FVB_ATTRIBUTES_2 in which the + attributes and current settings are + returned. Type EFI_FVB_ATTRIBUTES_2 is defined + in EFI_FIRMWARE_VOLUME_HEADER. + + @retval EFI_SUCCESS The firmware volume attributes were + returned. + +**/ +STATIC +EFI_STATUS +OpTeeRpmbFvbGetAttributes ( + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, + OUT EFI_FVB_ATTRIBUTES_2 *Attributes + ) +{ + *Attributes = EFI_FVB2_READ_ENABLED_CAP | // Reads may be enabled + EFI_FVB2_READ_STATUS | // Reads are currently enabled + EFI_FVB2_WRITE_STATUS | // Writes are currently enabled + EFI_FVB2_WRITE_ENABLED_CAP | // Writes may be enabled + EFI_FVB2_STICKY_WRITE | // A block erase is required to flip bits into EFI_FVB2_ERASE_POLARITY + EFI_FVB2_MEMORY_MAPPED | // It is memory mapped + EFI_FVB2_ERASE_POLARITY; // After erasure all bits take this value (i.e. '1') + + return EFI_SUCCESS; +} + +/** + The SetAttributes() function sets configurable firmware volume + attributes and returns the new settings of the firmware volume. + + @param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL instance. + + @param Attributes On input, Attributes is a pointer to + EFI_FVB_ATTRIBUTES_2 that contains the + desired firmware volume settings. On + successful return, it contains the new + settings of the firmware volume. Type + EFI_FVB_ATTRIBUTES_2 is defined in + EFI_FIRMWARE_VOLUME_HEADER. + + @retval EFI_SUCCESS The firmware volume attributes were returned. + + @retval EFI_INVALID_PARAMETER The attributes requested are in + conflict with the capabilities + as declared in the firmware + volume header. + +**/ +STATIC +EFI_STATUS +OpTeeRpmbFvbSetAttributes ( + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, + IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes + ) +{ + return EFI_SUCCESS; // ignore for now +} + +/** + The GetPhysicalAddress() function retrieves the base address of + a memory-mapped firmware volume. This function should be called + only for memory-mapped firmware volumes. + + @param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL instance. + + @param Address Pointer to a caller-allocated + EFI_PHYSICAL_ADDRESS that, on successful + return from GetPhysicalAddress(), contains the + base address of the firmware volume. + + @retval EFI_SUCCESS The firmware volume base address was returned. + + @retval EFI_UNSUPPORTED The firmware volume is not memory mapped. + +**/ +STATIC +EFI_STATUS +OpTeeRpmbFvbGetPhysicalAddress ( + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, + OUT EFI_PHYSICAL_ADDRESS *Address + ) +{ + MEM_INSTANCE *Instance; + + Instance = INSTANCE_FROM_FVB_THIS(This); + *Address = Instance->MemBaseAddress; + + return EFI_SUCCESS; +} + +/** + The GetBlockSize() function retrieves the size of the requested + block. It also returns the number of additional blocks with + the identical size. The GetBlockSize() function is used to + retrieve the block map (see EFI_FIRMWARE_VOLUME_HEADER). + + + @param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL instance. + + @param Lba Indicates the block for which to return the size. + + @param BlockSize Pointer to a caller-allocated UINTN in which + the size of the block is returned. + + @param NumberOfBlocks Pointer to a caller-allocated UINTN in + which the number of consecutive blocks, + starting with Lba, is returned. All + blocks in this range have a size of + BlockSize. + + + @retval EFI_SUCCESS The firmware volume base address was returned. + + @retval EFI_INVALID_PARAMETER The requested LBA is out of range. + +**/ +STATIC +EFI_STATUS +OpTeeRpmbFvbGetBlockSize ( + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, + IN EFI_LBA Lba, + OUT UINTN *BlockSize, + OUT UINTN *NumberOfBlocks + ) +{ + MEM_INSTANCE *Instance; + + Instance = INSTANCE_FROM_FVB_THIS(This); + if (Lba > Instance->NBlocks) { + return EFI_INVALID_PARAMETER; + } + + *NumberOfBlocks = Instance->NBlocks - (UINTN) Lba; + *BlockSize = Instance->BlockSize; + + return EFI_SUCCESS; +} + +/** + Reads the specified number of bytes into a buffer from the specified block. + + The Read() function reads the requested number of bytes from the + requested block and stores them in the provided buffer. + Implementations should be mindful that the firmware volume + might be in the ReadDisabled state. If it is in this state, + the Read() function must return the status code + EFI_ACCESS_DENIED without modifying the contents of the + buffer. The Read() function must also prevent spanning block + boundaries. If a read is requested that would span a block + boundary, the read must read up to the boundary but not + beyond. The output parameter NumBytes must be set to correctly + indicate the number of bytes actually read. The caller must be + aware that a read may be partially completed. + + @param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL instance. + + @param Lba The starting logical block index + from which to read. + + @param Offset Offset into the block at which to begin reading. + + @param NumBytes Pointer to a UINTN. At entry, *NumBytes + contains the total size of the buffer. At + exit, *NumBytes contains the total number of + bytes read. + + @param Buffer Pointer to a caller-allocated buffer that will + be used to hold the data that is read. + + @retval EFI_SUCCESS The firmware volume was read successfully, + and contents are in Buffer. + + @retval EFI_BAD_BUFFER_SIZE Read attempted across an LBA + boundary. On output, NumBytes + contains the total number of bytes + returned in Buffer. + + @retval EFI_ACCESS_DENIED The firmware volume is in the + ReadDisabled state. + + @retval EFI_DEVICE_ERROR The block device is not + functioning correctly and could + not be read. + +**/ + +STATIC +EFI_STATUS +OpTeeRpmbFvbRead ( + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, + IN EFI_LBA Lba, + IN UINTN Offset, + IN OUT UINTN *NumBytes, + IN OUT UINT8 *Buffer + ) +{ + EFI_STATUS Status = EFI_SUCCESS; + MEM_INSTANCE *Instance; + VOID *Base; + + Instance = INSTANCE_FROM_FVB_THIS(This); + if (Instance->Initialized == FALSE) { + Instance->Initialize (Instance); + } + + Base = (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize + Offset; + // We could read the data from the RPMB instead of memory + // The 2 copies should already be identical + // Copy from memory image + CopyMem (Buffer, Base, *NumBytes); + + return Status; +} + +/** + Writes the specified number of bytes from the input buffer to the block. + + The Write() function writes the specified number of bytes from + the provided buffer to the specified block and offset. If the + firmware volume is sticky write, the caller must ensure that + all the bits of the specified range to write are in the + EFI_FVB_ERASE_POLARITY state before calling the Write() + function, or else the result will be unpredictable. This + unpredictability arises because, for a sticky-write firmware + volume, a write may negate a bit in the EFI_FVB_ERASE_POLARITY + state but cannot flip it back again. Before calling the + Write() function, it is recommended for the caller to first call + the EraseBlocks() function to erase the specified block to + write. A block erase cycle will transition bits from the + (NOT)EFI_FVB_ERASE_POLARITY state back to the + EFI_FVB_ERASE_POLARITY state. Implementations should be + mindful that the firmware volume might be in the WriteDisabled + state. If it is in this state, the Write() function must + return the status code EFI_ACCESS_DENIED without modifying the + contents of the firmware volume. The Write() function must + also prevent spanning block boundaries. If a write is + requested that spans a block boundary, the write must store up + to the boundary but not beyond. The output parameter NumBytes + must be set to correctly indicate the number of bytes actually + written. The caller must be aware that a write may be + partially completed. All writes, partial or otherwise, must be + fully flushed to the hardware before the Write() service + returns. + + @param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL instance. + + @param Lba The starting logical block index to write to. + + @param Offset Offset into the block at which to begin writing. + + @param NumBytes The pointer to a UINTN. At entry, *NumBytes + contains the total size of the buffer. At + exit, *NumBytes contains the total number of + bytes actually written. + + @param Buffer The pointer to a caller-allocated buffer that + contains the source for the write. + + @retval EFI_SUCCESS The firmware volume was written successfully. + + @retval EFI_BAD_BUFFER_SIZE The write was attempted across an + LBA boundary. On output, NumBytes + contains the total number of bytes + actually written. + + @retval EFI_ACCESS_DENIED The firmware volume is in the + WriteDisabled state. + + @retval EFI_DEVICE_ERROR The block device is malfunctioning + and could not be written. + + +**/ +STATIC +EFI_STATUS +OpTeeRpmbFvbWrite ( + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, + IN EFI_LBA Lba, + IN UINTN Offset, + IN OUT UINTN *NumBytes, + IN UINT8 *Buffer + ) +{ + MEM_INSTANCE *Instance; + EFI_STATUS Status = EFI_SUCCESS; + VOID *Base; + + Instance = INSTANCE_FROM_FVB_THIS(This); + if (Instance->Initialized == FALSE) { + Instance->Initialize (Instance); + } + Base = (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize + Offset; + Status = ReadWriteRpmb (SP_SVC_RPMB_WRITE, (UINTN) Buffer, *NumBytes, + Lba * Instance->BlockSize + Offset); + if (Status != EFI_SUCCESS) { + return Status; + } + + // Update the memory copy + CopyMem (Base, Buffer, *NumBytes); + + return EFI_SUCCESS; +} + +/** + Erases and initializes a firmware volume block. + + The EraseBlocks() function erases one or more blocks as denoted + by the variable argument list. The entire parameter list of + blocks must be verified before erasing any blocks. If a block is + requested that does not exist within the associated firmware + volume (it has a larger index than the last block of the + firmware volume), the EraseBlocks() function must return the + status code EFI_INVALID_PARAMETER without modifying the contents + of the firmware volume. Implementations should be mindful that + the firmware volume might be in the WriteDisabled state. If it + is in this state, the EraseBlocks() function must return the + status code EFI_ACCESS_DENIED without modifying the contents of + the firmware volume. All calls to EraseBlocks() must be fully + flushed to the hardware before the EraseBlocks() service + returns. + + @param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL + instance. + + @param ... The variable argument list is a list of tuples. + Each tuple describes a range of LBAs to erase + and consists of the following: + - An EFI_LBA that indicates the starting LBA + - A UINTN that indicates the number of blocks to + erase. + + The list is terminated with an + EFI_LBA_LIST_TERMINATOR. For example, the + following indicates that two ranges of blocks + (5-7 and 10-11) are to be erased: EraseBlocks + (This, 5, 3, 10, 2, EFI_LBA_LIST_TERMINATOR); + + @retval EFI_SUCCESS The erase request successfully + completed. + + @retval EFI_ACCESS_DENIED The firmware volume is in the + WriteDisabled state. + @retval EFI_DEVICE_ERROR The block device is not functioning + correctly and could not be written. + The firmware device may have been + partially erased. + @retval EFI_INVALID_PARAMETER One or more of the LBAs listed + in the variable argument list do + not exist in the firmware volume. + +**/ +STATIC +EFI_STATUS +OpTeeRpmbFvbErase ( + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, + ... + ) +{ + MEM_INSTANCE *Instance; + UINTN NumBytes; + UINTN NumLba; + EFI_LBA Start; + VOID *Base; + VOID *Buf; + VA_LIST Args; + EFI_STATUS Status; + + Instance = INSTANCE_FROM_FVB_THIS(This); + + VA_START (Args, This); + for (Start = VA_ARG (Args, EFI_LBA); + Start != EFI_LBA_LIST_TERMINATOR; + Start = VA_ARG (Args, EFI_LBA)) { + NumLba = VA_ARG (Args, UINTN); + if (NumLba == 0 || Start + NumLba > Instance->NBlocks) { + return EFI_INVALID_PARAMETER; + } + NumBytes = NumLba * Instance->BlockSize; + Base = (VOID *)Instance->MemBaseAddress + Start * Instance->BlockSize; + Buf = AllocatePool(NumLba * Instance->BlockSize); + if (!Buf) { + return EFI_DEVICE_ERROR; + } + SetMem64 (Buf, NumLba * Instance->BlockSize, ~0UL); + // Write the device + Status = ReadWriteRpmb (SP_SVC_RPMB_WRITE, (UINTN) Buf, NumBytes, + Start * Instance->BlockSize); + if (Status != EFI_SUCCESS) { + return Status; + } + // Update the in memory copy + SetMem64 (Base, NumLba * Instance->BlockSize, ~0UL); + FreePool (Buf); + } + + VA_END (Args); + + return EFI_SUCCESS; +} + +/** + Since we use a memory backed storage we need to restore the RPMB contents + into memory before we register the Fvb protocol. + + @param Instace Address to copy flash contents to + + @retval 0 on success, OP-TEE error on failure +**/ +STATIC +VOID +ReadEntireFlash ( + MEM_INSTANCE *Instance + ) +{ + UINTN ReadAddr; + + UINTN StorageFtwWorkingSize = PcdGet32(PcdFlashNvStorageFtwWorkingSize); + UINTN StorageVariableSize = PcdGet32(PcdFlashNvStorageVariableSize); + UINTN StorageFtwSpareSize = PcdGet32(PcdFlashNvStorageFtwSpareSize); + + ReadAddr = Instance->MemBaseAddress; + // There's no need to check if the read failed here. The upper EDK2 layers + // will initialize the flash correctly if the in-memory copy is wrong + ReadWriteRpmb(SP_SVC_RPMB_READ, ReadAddr, StorageVariableSize + + StorageFtwWorkingSize + StorageFtwSpareSize , 0); +} + + +STATIC +EFI_STATUS +EFIAPI +ValidateFvHeader ( + IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader + ) +{ + UINT16 Checksum; + VARIABLE_STORE_HEADER *VariableStoreHeader; + UINTN VariableStoreLength; + UINTN FvLength; + + FvLength = PcdGet32(PcdFlashNvStorageVariableSize) + + PcdGet32(PcdFlashNvStorageFtwWorkingSize) + + PcdGet32(PcdFlashNvStorageFtwSpareSize); + + // Verify the header revision, header signature, length + // Length of FvBlock cannot be 2**64-1 + // HeaderLength cannot be an odd number + // + if ( (FwVolHeader->Revision != EFI_FVH_REVISION) + || (FwVolHeader->Signature != EFI_FVH_SIGNATURE) + || (FwVolHeader->FvLength != FvLength) + ) + { + DEBUG ((DEBUG_INFO, "%a: No Firmware Volume header present\n", + __FUNCTION__)); + return EFI_NOT_FOUND; + } + + // Check the Firmware Volume Guid + if (!CompareGuid (&FwVolHeader->FileSystemGuid, &gEfiSystemNvDataFvGuid)) { + DEBUG ((DEBUG_INFO, "%a: Firmware Volume Guid non-compatible\n", + __FUNCTION__)); + return EFI_NOT_FOUND; + } + + // Verify the header checksum + Checksum = CalculateSum16((UINT16*)FwVolHeader, FwVolHeader->HeaderLength); + if (Checksum != 0) { + DEBUG ((DEBUG_INFO, "%a: FV checksum is invalid (Checksum:0x%X)\n", + __FUNCTION__, Checksum)); + return EFI_NOT_FOUND; + } + + VariableStoreHeader = (VOID *)((UINTN)FwVolHeader + + FwVolHeader->HeaderLength); + + // Check the Variable Store Guid + if (!CompareGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid) && + !CompareGuid (&VariableStoreHeader->Signature, + &gEfiAuthenticatedVariableGuid)) { + DEBUG ((DEBUG_INFO, "%a: Variable Store Guid non-compatible\n", + __FUNCTION__)); + return EFI_NOT_FOUND; + } + + VariableStoreLength = PcdGet32 (PcdFlashNvStorageVariableSize) - + FwVolHeader->HeaderLength; + if (VariableStoreHeader->Size != VariableStoreLength) { + DEBUG ((DEBUG_INFO, "%a: Variable Store Length does not match\n", + __FUNCTION__)); + return EFI_NOT_FOUND; + } + + return EFI_SUCCESS; + +} + +STATIC +EFI_STATUS +InitializeFvAndVariableStoreHeaders ( + MEM_INSTANCE *Instance + ) +{ + EFI_FIRMWARE_VOLUME_HEADER *FirmwareVolumeHeader; + VARIABLE_STORE_HEADER *VariableStoreHeader; + EFI_STATUS Status = EFI_SUCCESS; + UINTN HeadersLength; + VOID* Headers; + + HeadersLength = sizeof(EFI_FIRMWARE_VOLUME_HEADER) + + sizeof(EFI_FV_BLOCK_MAP_ENTRY) + + sizeof(VARIABLE_STORE_HEADER); + Headers = AllocateZeroPool(HeadersLength); + + // + // EFI_FIRMWARE_VOLUME_HEADER + // + FirmwareVolumeHeader = (EFI_FIRMWARE_VOLUME_HEADER*)Headers; + CopyGuid (&FirmwareVolumeHeader->FileSystemGuid, &gEfiSystemNvDataFvGuid); + FirmwareVolumeHeader->FvLength = + PcdGet32(PcdFlashNvStorageVariableSize) + + PcdGet32(PcdFlashNvStorageFtwWorkingSize) + + PcdGet32(PcdFlashNvStorageFtwSpareSize); + FirmwareVolumeHeader->Signature = EFI_FVH_SIGNATURE; + FirmwareVolumeHeader->Attributes = EFI_FVB2_READ_ENABLED_CAP | + EFI_FVB2_READ_STATUS | + EFI_FVB2_STICKY_WRITE | + EFI_FVB2_MEMORY_MAPPED | + EFI_FVB2_ERASE_POLARITY | + EFI_FVB2_WRITE_STATUS | + EFI_FVB2_WRITE_ENABLED_CAP; + + FirmwareVolumeHeader->HeaderLength = sizeof(EFI_FIRMWARE_VOLUME_HEADER) + + sizeof(EFI_FV_BLOCK_MAP_ENTRY); + FirmwareVolumeHeader->Revision = EFI_FVH_REVISION; + FirmwareVolumeHeader->BlockMap[0].NumBlocks = Instance->NBlocks; + FirmwareVolumeHeader->BlockMap[0].Length = Instance->BlockSize; + FirmwareVolumeHeader->BlockMap[1].NumBlocks = 0; + FirmwareVolumeHeader->BlockMap[1].Length = 0; + FirmwareVolumeHeader->Checksum = CalculateCheckSum16 ( + (UINT16*)FirmwareVolumeHeader, + FirmwareVolumeHeader->HeaderLength); + + // + // VARIABLE_STORE_HEADER + // + VariableStoreHeader = (VOID *)((UINTN)Headers + + FirmwareVolumeHeader->HeaderLength); + CopyGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid); + VariableStoreHeader->Size = PcdGet32(PcdFlashNvStorageVariableSize) - + FirmwareVolumeHeader->HeaderLength; + VariableStoreHeader->Format = VARIABLE_STORE_FORMATTED; + VariableStoreHeader->State = VARIABLE_STORE_HEALTHY; + + Status = ReadWriteRpmb(SP_SVC_RPMB_WRITE, (UINTN) Headers, HeadersLength, 0); + if (Status != EFI_SUCCESS) { + goto Exit; + } + // Install the combined header in memory + CopyMem ((VOID*) Instance->MemBaseAddress, Headers, HeadersLength); + +Exit: + FreePool (Headers); + return Status; +} + +STATIC +EFI_STATUS +EFIAPI +FvbInitialize ( + MEM_INSTANCE *Instance + ) +{ + EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader; + EFI_STATUS Status; + + if (Instance->Initialized == TRUE) { + return EFI_SUCCESS; + } + + // FirmwareVolumeHeader->FvLength is declared to have the Variable area + // AND the FTW working area AND the FTW Spare contiguous. + ASSERT (PcdGet32 (PcdFlashNvStorageVariableBase) + + PcdGet32 (PcdFlashNvStorageVariableSize) == + PcdGet32 (PcdFlashNvStorageFtwWorkingBase)); + ASSERT (PcdGet32 (PcdFlashNvStorageFtwWorkingBase) + + PcdGet32 (PcdFlashNvStorageFtwWorkingSize) == + PcdGet32 (PcdFlashNvStorageFtwSpareBase)); + + // Check if the size of the area is at least one block size + ASSERT ((PcdGet32 (PcdFlashNvStorageVariableSize) > 0) && + (PcdGet32 (PcdFlashNvStorageVariableSize) / Instance->BlockSize > 0)); + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > 0) && + (PcdGet32 (PcdFlashNvStorageFtwWorkingSize) / Instance->BlockSize > 0)); + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwSpareSize) > 0) && + (PcdGet32 (PcdFlashNvStorageFtwSpareSize) / Instance->BlockSize > 0)); + + // Ensure the Variable areas are aligned on block size boundaries + ASSERT ((PcdGet32 (PcdFlashNvStorageVariableBase) % Instance->BlockSize) == 0); + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwWorkingBase) % Instance->BlockSize) == 0); + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwSpareBase) % Instance->BlockSize) == 0); + + // Read the file from disk and copy it to memory + ReadEntireFlash (Instance); + + FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *) Instance->MemBaseAddress; + Status = ValidateFvHeader(FwVolHeader); + if (EFI_ERROR (Status)) { + // There is no valid header, so time to install one. + DEBUG ((DEBUG_INFO, "%a: The FVB Header is not valid.\n", __FUNCTION__)); + + // Reset memory + SetMem64 ((VOID *)Instance->MemBaseAddress, Instance->NBlocks * Instance->BlockSize, ~0UL); + DEBUG ((DEBUG_INFO, "%a: Erasing Flash.\n", __FUNCTION__)); + Status = ReadWriteRpmb(SP_SVC_RPMB_WRITE, Instance->MemBaseAddress, + PcdGet32(PcdFlashNvStorageVariableSize) + + PcdGet32(PcdFlashNvStorageFtwWorkingSize) + + PcdGet32(PcdFlashNvStorageFtwSpareSize), 0); + if (Status != EFI_SUCCESS) { + return Status; + } + // Install all appropriate headers + DEBUG ((DEBUG_INFO, "%a: Installing a correct one for this volume.\n", + __FUNCTION__)); + Status = InitializeFvAndVariableStoreHeaders (Instance); + if (EFI_ERROR (Status)) { + return Status; + } + } else { + DEBUG ((DEBUG_INFO, "%a: Found valid FVB Header.\n", __FUNCTION__)); + } + Instance->Initialized = TRUE; + + return EFI_SUCCESS; +} + +EFI_STATUS +EFIAPI +OpTeeRpmbFvbInit ( + IN EFI_HANDLE ImageHandle, + IN EFI_MM_SYSTEM_TABLE *SystemTable + ) +{ + EFI_STATUS Status; + VOID *Addr; + UINTN FvLength; + UINTN NBlocks; + + FvLength = PcdGet32(PcdFlashNvStorageVariableSize) + + PcdGet32(PcdFlashNvStorageFtwWorkingSize) + + PcdGet32(PcdFlashNvStorageFtwSpareSize); + + NBlocks = EFI_SIZE_TO_PAGES(ALIGN_VARIABLE(FvLength)); + Addr = AllocatePages(NBlocks); + ASSERT (Addr != NULL); + + SetMem (&mInstance, sizeof (mInstance), 0); + + mInstance.FvbProtocol.GetPhysicalAddress = OpTeeRpmbFvbGetPhysicalAddress; + mInstance.FvbProtocol.GetAttributes = OpTeeRpmbFvbGetAttributes; + mInstance.FvbProtocol.SetAttributes = OpTeeRpmbFvbSetAttributes; + mInstance.FvbProtocol.GetBlockSize = OpTeeRpmbFvbGetBlockSize; + mInstance.FvbProtocol.EraseBlocks = OpTeeRpmbFvbErase; + mInstance.FvbProtocol.Write = OpTeeRpmbFvbWrite; + mInstance.FvbProtocol.Read = OpTeeRpmbFvbRead; + + mInstance.MemBaseAddress = (EFI_PHYSICAL_ADDRESS) Addr; + mInstance.Signature = FLASH_SIGNATURE; + mInstance.Initialize = FvbInitialize; + mInstance.BlockSize = EFI_PAGE_SIZE; + mInstance.NBlocks = NBlocks; + + // Update the defined PCDs related to Variable Storage + PatchPcdSet32 (PcdFlashNvStorageVariableBase, mInstance.MemBaseAddress); + PatchPcdSet32 (PcdFlashNvStorageFtwWorkingBase, mInstance.MemBaseAddress + + PcdGet32 (PcdFlashNvStorageVariableSize)); + PatchPcdSet32 (PcdFlashNvStorageFtwSpareBase, mInstance.MemBaseAddress + + PcdGet32 (PcdFlashNvStorageVariableSize) + + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)); + + Status = gMmst->MmInstallProtocolInterface ( + &mInstance.Handle, + &gEfiSmmFirmwareVolumeBlockProtocolGuid, + EFI_NATIVE_INTERFACE, + &mInstance.FvbProtocol + ); + ASSERT_EFI_ERROR (Status); + + DEBUG ((DEBUG_INFO, "%a: Register OP-TEE RPMB Fvb\n", __FUNCTION__)); + DEBUG ((DEBUG_INFO, "%a: Using NV store FV in-memory copy at 0x%lx\n", + __FUNCTION__, PatchPcdGet32 (PcdFlashNvStorageVariableBase))); + + return Status; +} -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver 2020-12-16 11:09 ` [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver Sughosh Ganu @ 2021-01-27 17:10 ` Sami Mujawar 2021-01-29 8:02 ` Ilias Apalodimas 0 siblings, 1 reply; 14+ messages in thread From: Sami Mujawar @ 2021-01-27 17:10 UTC (permalink / raw) To: Sughosh Ganu, devel@edk2.groups.io Cc: Ard Biesheuvel, Leif Lindholm, Sahil Malhotra, Ilias Apalodimas Hi Sughosh, There are some edk2 coding standard incompatibilities (like space between function name and opening bracket, function parameter alignment etc.) and documentation for some functions is missing in the patch. Can you fix these, please? Please also run the ECC tool in Drivers/OpTeeRpmb folder and fix any reported issues. The ECC errors 10002, 10006 and 10022 can be ignored for now. Other than that, please find my response inline marked [SAMI]. Regards, Sami Mujawar -----Original Message----- From: Sughosh Ganu <sughosh.ganu@linaro.org> Sent: 16 December 2020 11:09 AM To: devel@edk2.groups.io Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Leif Lindholm <leif@nuviainc.com>; Sahil Malhotra <sahil.malhotra@linaro.org>; Ilias Apalodimas <ilias.apalodimas@linaro.org> Subject: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver From: Ilias Apalodimas <ilias.apalodimas@linaro.org> A following patch is adding support for building StMM in order to run it from OP-TEE. OP-TEE in combination with a NS-world supplicant can use the RPMB partition of an eMMC to store EFI variables. The supplicant functionality is currently available in U-Boot only but can be ported into EDK2. Assuming similar functionality is added in EDK2, this will allow any hardware with an RPMB partition to store EFI variables securely. So let's add a driver that enables access of the RPMB partition through OP-TEE. Since the upper layers expect a byte addressable interface, the driver allocates memory and patches the PCDs, while syncing the memory/hardware on read/write callbacks. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- Changes since V2: - Allocate a dynamic number of pages based on the Pcd values instead of a static number - Clean up unused structs in header file - Added checks in OpTeeRpmbFvbGetBlockSize and handle NumLba=0 Drivers/OpTeeRpmb/FixupPcd.inf | 44 ++ Drivers/OpTeeRpmb/OpTeeRpmbFv.inf | 58 ++ Drivers/OpTeeRpmb/OpTeeRpmbFvb.h | 35 + Drivers/OpTeeRpmb/FixupPcd.c | 74 ++ Drivers/OpTeeRpmb/OpTeeRpmbFvb.c | 803 ++++++++++++++++++++ 5 files changed, 1014 insertions(+) diff --git a/Drivers/OpTeeRpmb/FixupPcd.inf b/Drivers/OpTeeRpmb/FixupPcd.inf new file mode 100644 index 0000000000..f0cfdf7a4c --- /dev/null +++ b/Drivers/OpTeeRpmb/FixupPcd.inf @@ -0,0 +1,44 @@ +## @file +# Instance of Base Memory Library without assembly. +# +# Copyright (c) 2020, Linaro Ltd. All rights reserved.<BR> +# +# SPDX-License-Identifier: BSD-2-Clause-Patent +# +# +## + +[Defines] + INF_VERSION = 0x0001001A + BASE_NAME = FixupPcd + FILE_GUID = a827c337-a9c6-301b-aeb7-acbc95d8da22 + MODULE_TYPE = BASE + VERSION_STRING = 0.1 + LIBRARY_CLASS = RpmbPcdFixup|MM_STANDALONE + CONSTRUCTOR = FixPcdMemory + +[Sources] + FixupPcd.c + OpTeeRpmbFvb.h + +[Packages] + MdeModulePkg/MdeModulePkg.dec + MdePkg/MdePkg.dec + +[LibraryClasses] + BaseLib + DebugLib + MmServicesTableLib + PcdLib + +[Pcd] + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize + + +[Protocols] + gEfiSmmFirmwareVolumeBlockProtocolGuid ## CONSUMES diff --git a/Drivers/OpTeeRpmb/OpTeeRpmbFv.inf b/Drivers/OpTeeRpmb/OpTeeRpmbFv.inf new file mode 100644 index 0000000000..b21f7397e5 --- /dev/null +++ b/Drivers/OpTeeRpmb/OpTeeRpmbFv.inf @@ -0,0 +1,58 @@ +## @file +# +# Component description file for OpTeeRpmbFv module +# +# Copyright (c) 2020, Linaro Ltd. All rights reserved.<BR> +# +# SPDX-License-Identifier: BSD-2-Clause-Patent +# +## + +[Defines] + INF_VERSION = 0x0001001A + BASE_NAME = OpTeeRpmbFv + FILE_GUID = 4803FC20-E583-3BCD-8C60-141E85C9A2CF + MODULE_TYPE = MM_STANDALONE + VERSION_STRING = 0.1 + PI_SPECIFICATION_VERSION = 0x00010032 + ENTRY_POINT = OpTeeRpmbFvbInit + +[Sources] + OpTeeRpmbFvb.c + OpTeeRpmbFvb.h + +[Packages] + ArmPkg/ArmPkg.dec + ArmPlatformPkg/ArmPlatformPkg.dec + MdeModulePkg/MdeModulePkg.dec + MdePkg/MdePkg.dec + StandaloneMmPkg/StandaloneMmPkg.dec + +[LibraryClasses] + ArmSvcLib + BaseLib + BaseMemoryLib + DebugLib + MemoryAllocationLib + MmServicesTableLib + PcdLib + StandaloneMmDriverEntryPoint + +[Guids] + gEfiAuthenticatedVariableGuid + gEfiSystemNvDataFvGuid + gEfiVariableGuid + +[Pcd] + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize + +[Protocols] + gEfiSmmFirmwareVolumeBlockProtocolGuid ## PRODUCES + +[Depex] + TRUE diff --git a/Drivers/OpTeeRpmb/OpTeeRpmbFvb.h b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.h new file mode 100644 index 0000000000..717fc4a78b --- /dev/null +++ b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.h @@ -0,0 +1,35 @@ +/** @file + + Copyright (c) 2020, Linaro Ltd. All rights reserved.<BR> + SPDX-License-Identifier: BSD-2-Clause-Patent + +**/ + +#ifndef __OPTEE_RPMB_FV_ +#define __OPTEE_RPMB_FV_ [SAMI] This does not follow the edk2 coding standard. See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/53_include_files#5-3-5-all-include-file-contents-must-be-protected-by-a-include-guard [/SAMI] + +/* SVC Args */ +#define SP_SVC_RPMB_READ 0xC4000066 +#define SP_SVC_RPMB_WRITE 0xC4000067 [SAMI] Is there a specification reference for this? If so, please add to the file header. See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-3-1-every-new-file-shall-begin-with-a-file-header-comment-block. [/SAMI] + +#define FILENAME "EFI_VARS" + +#define FLASH_SIGNATURE SIGNATURE_32('r', 'p', 'm', 'b') +#define INSTANCE_FROM_FVB_THIS(a) CR(a, MEM_INSTANCE, FvbProtocol, \ + FLASH_SIGNATURE) + +typedef struct _MEM_INSTANCE MEM_INSTANCE; +typedef EFI_STATUS (*MEM_INITIALIZE) (MEM_INSTANCE* Instance); +struct _MEM_INSTANCE +{ + UINT32 Signature; + MEM_INITIALIZE Initialize; + BOOLEAN Initialized; + EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL FvbProtocol; + EFI_HANDLE Handle; + EFI_PHYSICAL_ADDRESS MemBaseAddress; + UINT16 BlockSize; + UINT16 NBlocks; +}; [SAMI] Please add documentation for structure. See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/56_declarations_and_types#5-6-3-2-structure-declaration-with-forward-reference-or-self-reference [/SAMI] + +#endif diff --git a/Drivers/OpTeeRpmb/FixupPcd.c b/Drivers/OpTeeRpmb/FixupPcd.c new file mode 100644 index 0000000000..3cc882fa94 --- /dev/null +++ b/Drivers/OpTeeRpmb/FixupPcd.c @@ -0,0 +1,74 @@ +/** @file + + Update the patched PCDs to their correct value + + Copyright (c) 2020, Linaro Ltd. All rights reserved.<BR> + + SPDX-License-Identifier: BSD-2-Clause-Patent + +**/ + +/** + * Patch the relevant PCDs of the RPMB driver with the correct address of the + * allocated memory + * +**/ +#include <Library/BaseLib.h> +#include <Library/DebugLib.h> +#include <Library/MmServicesTableLib.h> +#include <Library/PcdLib.h> + +#include <Protocol/FirmwareVolumeBlock.h> +#include <Protocol/SmmFirmwareVolumeBlock.h> + +#include "OpTeeRpmbFvb.h" + +/** + Fixup the Pcd values for variable storage + + Since the upper layers of EDK2 expect a memory mapped interface and we can't + offer that from an RPMB, the driver allocates memory on init and passes that + on the upper layers. Since the memory is dynamically allocated and we can't set the + PCD is StMM context, we need to patch it correctly on each access + + @retval EFI_SUCCESS Protocol was found and PCDs patched up + + **/ +EFI_STATUS +EFIAPI +FixPcdMemory ( + VOID + ) +{ + EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *FvbProtocol; + MEM_INSTANCE *Instance; + EFI_STATUS Status; + + // + // Locate SmmFirmwareVolumeBlockProtocol + // + Status = gMmst->MmLocateProtocol ( + &gEfiSmmFirmwareVolumeBlockProtocolGuid, + NULL, + (VOID **) &FvbProtocol + ); + ASSERT_EFI_ERROR (Status); + + Instance = INSTANCE_FROM_FVB_THIS(FvbProtocol); + // Patch PCDs with the the correct values + PatchPcdSet32 (PcdFlashNvStorageVariableBase, Instance->MemBaseAddress); + PatchPcdSet32 (PcdFlashNvStorageFtwWorkingBase, Instance->MemBaseAddress + + PcdGet32 (PcdFlashNvStorageVariableSize)); + PatchPcdSet32 (PcdFlashNvStorageFtwSpareBase, Instance->MemBaseAddress + + PcdGet32 (PcdFlashNvStorageVariableSize) + + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)); [SAMI] Should the 64-bit versions of the PCDs be used here. A recent change added similar support to the ArmPlatformPkg\Drivers\NorFlashDxe. [/SAMI] + + DEBUG ((DEBUG_INFO, "%a: Fixup PcdFlashNvStorageVariableBase: 0x%lx\n", + __FUNCTION__, PcdGet32 (PcdFlashNvStorageVariableBase))); + DEBUG ((DEBUG_INFO, "%a: Fixup PcdFlashNvStorageFtwWorkingBase: 0x%lx\n", + __FUNCTION__, PcdGet32 (PcdFlashNvStorageFtwWorkingBase))); + DEBUG ((DEBUG_INFO, "%a: Fixup PcdFlashNvStorageFtwSpareBase: 0x%lx\n", + __FUNCTION__, PcdGet32 (PcdFlashNvStorageFtwSpareBase))); + + return Status; +} diff --git a/Drivers/OpTeeRpmb/OpTeeRpmbFvb.c b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.c new file mode 100644 index 0000000000..71bb1f33b6 --- /dev/null +++ b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.c @@ -0,0 +1,803 @@ +/** @file + + FV block I/O protocol driver for RPMB eMMC accessed via OP-TEE + + Copyright (c) 2020, Linaro Ltd. All rights reserved.<BR> + + SPDX-License-Identifier: BSD-2-Clause-Patent + +**/ + +#include <Library/ArmSvcLib.h> +#include <Library/BaseLib.h> +#include <Library/BaseMemoryLib.h> +#include <Library/DebugLib.h> +#include <Library/MemoryAllocationLib.h> +#include <Library/MmServicesTableLib.h> +#include <Library/PcdLib.h> + +#include <IndustryStandard/ArmFfaSvc.h> +#include <IndustryStandard/ArmMmSvc.h> +#include <Protocol/FirmwareVolumeBlock.h> +#include <Protocol/SmmFirmwareVolumeBlock.h> +#include <Guid/VariableFormat.h> + +#include "OpTeeRpmbFvb.h" + +static const UINT16 mem_mgr_id = 3U; +static const UINT16 storage_id = 4U; [SAMI] Please change to STATIC CONST and also update the variable names according to the edk2 coding standard. See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/4_naming_conventions/43_identifiers#4-3-3-2-any-variable-with-file-scope-or-better-shall-be-prefixed-by-an-m-or-g [/SAMI] + +STATIC MEM_INSTANCE mInstance; + +/** + Sends an SVC call to OP-TEE for reading/writing an RPMB partition + + @param SvcAct SVC ID for read/write + @param Addr Base address of the buffer. When reading contents will be + copied to that buffer after reading them from the device. + When writing, the buffer holds the contents we want to + write cwtoin the device + @param NumBytes Number of bytes to read/write + @param Offset Offset into the RPMB file + + @retval OP-TEE return code +**/ + [SAMI] Remove blank line. Same at other places as well. [/SAMI] +STATIC +EFI_STATUS +ReadWriteRpmb ( + UINTN SvcAct, + UINTN Addr, + UINTN NumBytes, + UINTN Offset + ) +{ + ARM_SVC_ARGS SvcArgs; + EFI_STATUS Status; + + ZeroMem (&SvcArgs, sizeof (SvcArgs)); + + SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64; + SvcArgs.Arg1 = storage_id; + SvcArgs.Arg2 = 0; + SvcArgs.Arg3 = SvcAct; + SvcArgs.Arg4 = Addr; + SvcArgs.Arg5 = NumBytes; + SvcArgs.Arg6 = Offset; + + ArmCallSvc (&SvcArgs); + if (SvcArgs.Arg3) { + DEBUG ((DEBUG_ERROR, "%a: Svc Call 0x%08x Addr: 0x%08x len: 0x%x Offset: 0x%x failed with 0x%x\n", + __func__, SvcAct, Addr, NumBytes, Offset, SvcArgs.Arg3)); + } + + switch (SvcArgs.Arg3) { + case ARM_SVC_SPM_RET_SUCCESS: + Status = EFI_SUCCESS; + break; + + case ARM_SVC_SPM_RET_NOT_SUPPORTED: + Status = EFI_UNSUPPORTED; + break; + + case ARM_SVC_SPM_RET_INVALID_PARAMS: + Status = EFI_INVALID_PARAMETER; + break; + + case ARM_SVC_SPM_RET_DENIED: + Status = EFI_ACCESS_DENIED; + break; + + case ARM_SVC_SPM_RET_NO_MEMORY: + Status = EFI_BAD_BUFFER_SIZE; + break; + + default: + Status = EFI_ACCESS_DENIED; + } [SAMI] Should the error handling here be updated similar to the FF-A StandaloneMmPkg patches? [/SAMI] + + return Status; +} + +/** + The GetAttributes() function retrieves the attributes and + current settings of the block. + + @param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL instance. + + @param Attributes Pointer to EFI_FVB_ATTRIBUTES_2 in which the + attributes and current settings are + returned. Type EFI_FVB_ATTRIBUTES_2 is defined + in EFI_FIRMWARE_VOLUME_HEADER. + + @retval EFI_SUCCESS The firmware volume attributes were + returned. + +**/ +STATIC +EFI_STATUS +OpTeeRpmbFvbGetAttributes ( + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, + OUT EFI_FVB_ATTRIBUTES_2 *Attributes + ) +{ + *Attributes = EFI_FVB2_READ_ENABLED_CAP | // Reads may be enabled + EFI_FVB2_READ_STATUS | // Reads are currently enabled + EFI_FVB2_WRITE_STATUS | // Writes are currently enabled + EFI_FVB2_WRITE_ENABLED_CAP | // Writes may be enabled + EFI_FVB2_STICKY_WRITE | // A block erase is required to flip bits into EFI_FVB2_ERASE_POLARITY + EFI_FVB2_MEMORY_MAPPED | // It is memory mapped + EFI_FVB2_ERASE_POLARITY; // After erasure all bits take this value (i.e. '1') + + return EFI_SUCCESS; +} + +/** + The SetAttributes() function sets configurable firmware volume + attributes and returns the new settings of the firmware volume. + + @param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL instance. + + @param Attributes On input, Attributes is a pointer to + EFI_FVB_ATTRIBUTES_2 that contains the + desired firmware volume settings. On + successful return, it contains the new + settings of the firmware volume. Type + EFI_FVB_ATTRIBUTES_2 is defined in + EFI_FIRMWARE_VOLUME_HEADER. + + @retval EFI_SUCCESS The firmware volume attributes were returned. + + @retval EFI_INVALID_PARAMETER The attributes requested are in + conflict with the capabilities + as declared in the firmware + volume header. + +**/ +STATIC +EFI_STATUS +OpTeeRpmbFvbSetAttributes ( + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, + IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes + ) +{ + return EFI_SUCCESS; // ignore for now +} + +/** + The GetPhysicalAddress() function retrieves the base address of + a memory-mapped firmware volume. This function should be called + only for memory-mapped firmware volumes. + + @param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL instance. + + @param Address Pointer to a caller-allocated + EFI_PHYSICAL_ADDRESS that, on successful + return from GetPhysicalAddress(), contains the + base address of the firmware volume. + + @retval EFI_SUCCESS The firmware volume base address was returned. + + @retval EFI_UNSUPPORTED The firmware volume is not memory mapped. + +**/ +STATIC +EFI_STATUS +OpTeeRpmbFvbGetPhysicalAddress ( + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, + OUT EFI_PHYSICAL_ADDRESS *Address + ) +{ + MEM_INSTANCE *Instance; + + Instance = INSTANCE_FROM_FVB_THIS(This); + *Address = Instance->MemBaseAddress; + + return EFI_SUCCESS; +} + +/** + The GetBlockSize() function retrieves the size of the requested + block. It also returns the number of additional blocks with + the identical size. The GetBlockSize() function is used to + retrieve the block map (see EFI_FIRMWARE_VOLUME_HEADER). + + + @param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL instance. + + @param Lba Indicates the block for which to return the size. + + @param BlockSize Pointer to a caller-allocated UINTN in which + the size of the block is returned. + + @param NumberOfBlocks Pointer to a caller-allocated UINTN in + which the number of consecutive blocks, + starting with Lba, is returned. All + blocks in this range have a size of + BlockSize. + + + @retval EFI_SUCCESS The firmware volume base address was returned. + + @retval EFI_INVALID_PARAMETER The requested LBA is out of range. + +**/ +STATIC +EFI_STATUS +OpTeeRpmbFvbGetBlockSize ( + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, + IN EFI_LBA Lba, + OUT UINTN *BlockSize, + OUT UINTN *NumberOfBlocks + ) +{ + MEM_INSTANCE *Instance; + + Instance = INSTANCE_FROM_FVB_THIS(This); + if (Lba > Instance->NBlocks) { + return EFI_INVALID_PARAMETER; + } + + *NumberOfBlocks = Instance->NBlocks - (UINTN) Lba; + *BlockSize = Instance->BlockSize; + + return EFI_SUCCESS; +} + +/** + Reads the specified number of bytes into a buffer from the specified block. + + The Read() function reads the requested number of bytes from the + requested block and stores them in the provided buffer. + Implementations should be mindful that the firmware volume + might be in the ReadDisabled state. If it is in this state, + the Read() function must return the status code + EFI_ACCESS_DENIED without modifying the contents of the + buffer. The Read() function must also prevent spanning block + boundaries. If a read is requested that would span a block + boundary, the read must read up to the boundary but not + beyond. The output parameter NumBytes must be set to correctly + indicate the number of bytes actually read. The caller must be + aware that a read may be partially completed. + + @param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL instance. + + @param Lba The starting logical block index + from which to read. + + @param Offset Offset into the block at which to begin reading. + + @param NumBytes Pointer to a UINTN. At entry, *NumBytes + contains the total size of the buffer. At + exit, *NumBytes contains the total number of + bytes read. + + @param Buffer Pointer to a caller-allocated buffer that will + be used to hold the data that is read. + + @retval EFI_SUCCESS The firmware volume was read successfully, + and contents are in Buffer. + + @retval EFI_BAD_BUFFER_SIZE Read attempted across an LBA + boundary. On output, NumBytes + contains the total number of bytes + returned in Buffer. + + @retval EFI_ACCESS_DENIED The firmware volume is in the + ReadDisabled state. + + @retval EFI_DEVICE_ERROR The block device is not + functioning correctly and could + not be read. + +**/ + +STATIC +EFI_STATUS +OpTeeRpmbFvbRead ( + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, + IN EFI_LBA Lba, + IN UINTN Offset, + IN OUT UINTN *NumBytes, + IN OUT UINT8 *Buffer + ) +{ + EFI_STATUS Status = EFI_SUCCESS; + MEM_INSTANCE *Instance; + VOID *Base; + + Instance = INSTANCE_FROM_FVB_THIS(This); + if (Instance->Initialized == FALSE) { [SAMI] if (!Instance->Initialized) ? See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false [/SAMI] + Instance->Initialize (Instance); + } + + Base = (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize + Offset; + // We could read the data from the RPMB instead of memory + // The 2 copies should already be identical + // Copy from memory image + CopyMem (Buffer, Base, *NumBytes); + + return Status; +} + +/** + Writes the specified number of bytes from the input buffer to the block. + + The Write() function writes the specified number of bytes from + the provided buffer to the specified block and offset. If the + firmware volume is sticky write, the caller must ensure that + all the bits of the specified range to write are in the + EFI_FVB_ERASE_POLARITY state before calling the Write() + function, or else the result will be unpredictable. This + unpredictability arises because, for a sticky-write firmware + volume, a write may negate a bit in the EFI_FVB_ERASE_POLARITY + state but cannot flip it back again. Before calling the + Write() function, it is recommended for the caller to first call + the EraseBlocks() function to erase the specified block to + write. A block erase cycle will transition bits from the + (NOT)EFI_FVB_ERASE_POLARITY state back to the + EFI_FVB_ERASE_POLARITY state. Implementations should be + mindful that the firmware volume might be in the WriteDisabled + state. If it is in this state, the Write() function must + return the status code EFI_ACCESS_DENIED without modifying the + contents of the firmware volume. The Write() function must + also prevent spanning block boundaries. If a write is + requested that spans a block boundary, the write must store up + to the boundary but not beyond. The output parameter NumBytes + must be set to correctly indicate the number of bytes actually + written. The caller must be aware that a write may be + partially completed. All writes, partial or otherwise, must be + fully flushed to the hardware before the Write() service + returns. + + @param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL instance. + + @param Lba The starting logical block index to write to. + + @param Offset Offset into the block at which to begin writing. + + @param NumBytes The pointer to a UINTN. At entry, *NumBytes + contains the total size of the buffer. At + exit, *NumBytes contains the total number of + bytes actually written. + + @param Buffer The pointer to a caller-allocated buffer that + contains the source for the write. + + @retval EFI_SUCCESS The firmware volume was written successfully. + + @retval EFI_BAD_BUFFER_SIZE The write was attempted across an + LBA boundary. On output, NumBytes + contains the total number of bytes + actually written. + + @retval EFI_ACCESS_DENIED The firmware volume is in the + WriteDisabled state. + + @retval EFI_DEVICE_ERROR The block device is malfunctioning + and could not be written. + + +**/ +STATIC +EFI_STATUS +OpTeeRpmbFvbWrite ( + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, + IN EFI_LBA Lba, + IN UINTN Offset, + IN OUT UINTN *NumBytes, + IN UINT8 *Buffer + ) +{ + MEM_INSTANCE *Instance; + EFI_STATUS Status = EFI_SUCCESS; + VOID *Base; + + Instance = INSTANCE_FROM_FVB_THIS(This); + if (Instance->Initialized == FALSE) { [SAMI] if (!Instance->Initialized) ? See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false [/SAMI] + Instance->Initialize (Instance); + } + Base = (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize + Offset; + Status = ReadWriteRpmb (SP_SVC_RPMB_WRITE, (UINTN) Buffer, *NumBytes, + Lba * Instance->BlockSize + Offset); + if (Status != EFI_SUCCESS) { [SAMI] if (EFI_ERROR (Status)) ? [/SAMI] + return Status; + } + + // Update the memory copy + CopyMem (Base, Buffer, *NumBytes); + + return EFI_SUCCESS; +} + +/** + Erases and initializes a firmware volume block. + + The EraseBlocks() function erases one or more blocks as denoted + by the variable argument list. The entire parameter list of + blocks must be verified before erasing any blocks. If a block is + requested that does not exist within the associated firmware + volume (it has a larger index than the last block of the + firmware volume), the EraseBlocks() function must return the + status code EFI_INVALID_PARAMETER without modifying the contents + of the firmware volume. Implementations should be mindful that + the firmware volume might be in the WriteDisabled state. If it + is in this state, the EraseBlocks() function must return the + status code EFI_ACCESS_DENIED without modifying the contents of + the firmware volume. All calls to EraseBlocks() must be fully + flushed to the hardware before the EraseBlocks() service + returns. + + @param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL + instance. + + @param ... The variable argument list is a list of tuples. + Each tuple describes a range of LBAs to erase + and consists of the following: + - An EFI_LBA that indicates the starting LBA + - A UINTN that indicates the number of blocks to + erase. + + The list is terminated with an + EFI_LBA_LIST_TERMINATOR. For example, the + following indicates that two ranges of blocks + (5-7 and 10-11) are to be erased: EraseBlocks + (This, 5, 3, 10, 2, EFI_LBA_LIST_TERMINATOR); + + @retval EFI_SUCCESS The erase request successfully + completed. + + @retval EFI_ACCESS_DENIED The firmware volume is in the + WriteDisabled state. + @retval EFI_DEVICE_ERROR The block device is not functioning + correctly and could not be written. + The firmware device may have been + partially erased. + @retval EFI_INVALID_PARAMETER One or more of the LBAs listed + in the variable argument list do + not exist in the firmware volume. + +**/ +STATIC +EFI_STATUS +OpTeeRpmbFvbErase ( + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, + ... + ) +{ + MEM_INSTANCE *Instance; + UINTN NumBytes; + UINTN NumLba; + EFI_LBA Start; + VOID *Base; + VOID *Buf; + VA_LIST Args; + EFI_STATUS Status; + + Instance = INSTANCE_FROM_FVB_THIS(This); + + VA_START (Args, This); + for (Start = VA_ARG (Args, EFI_LBA); + Start != EFI_LBA_LIST_TERMINATOR; + Start = VA_ARG (Args, EFI_LBA)) { + NumLba = VA_ARG (Args, UINTN); + if (NumLba == 0 || Start + NumLba > Instance->NBlocks) { + return EFI_INVALID_PARAMETER; + } + NumBytes = NumLba * Instance->BlockSize; + Base = (VOID *)Instance->MemBaseAddress + Start * Instance->BlockSize; + Buf = AllocatePool(NumLba * Instance->BlockSize); + if (!Buf) { + return EFI_DEVICE_ERROR; + } + SetMem64 (Buf, NumLba * Instance->BlockSize, ~0UL); + // Write the device + Status = ReadWriteRpmb (SP_SVC_RPMB_WRITE, (UINTN) Buf, NumBytes, + Start * Instance->BlockSize); + if (Status != EFI_SUCCESS) { [SAMI] if (EFI_ERROR (Status)) ? [/SAMI] + return Status; + } + // Update the in memory copy + SetMem64 (Base, NumLba * Instance->BlockSize, ~0UL); + FreePool (Buf); + } + + VA_END (Args); + + return EFI_SUCCESS; +} + +/** + Since we use a memory backed storage we need to restore the RPMB contents + into memory before we register the Fvb protocol. + + @param Instace Address to copy flash contents to + + @retval 0 on success, OP-TEE error on failure +**/ +STATIC +VOID +ReadEntireFlash ( + MEM_INSTANCE *Instance + ) +{ + UINTN ReadAddr; + + UINTN StorageFtwWorkingSize = PcdGet32(PcdFlashNvStorageFtwWorkingSize); + UINTN StorageVariableSize = PcdGet32(PcdFlashNvStorageVariableSize); + UINTN StorageFtwSpareSize = PcdGet32(PcdFlashNvStorageFtwSpareSize); + + ReadAddr = Instance->MemBaseAddress; + // There's no need to check if the read failed here. The upper EDK2 layers + // will initialize the flash correctly if the in-memory copy is wrong + ReadWriteRpmb(SP_SVC_RPMB_READ, ReadAddr, StorageVariableSize + + StorageFtwWorkingSize + StorageFtwSpareSize , 0); +} + + +STATIC +EFI_STATUS +EFIAPI +ValidateFvHeader ( + IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader + ) +{ + UINT16 Checksum; + VARIABLE_STORE_HEADER *VariableStoreHeader; + UINTN VariableStoreLength; + UINTN FvLength; + + FvLength = PcdGet32(PcdFlashNvStorageVariableSize) + + PcdGet32(PcdFlashNvStorageFtwWorkingSize) + + PcdGet32(PcdFlashNvStorageFtwSpareSize); + + // Verify the header revision, header signature, length + // Length of FvBlock cannot be 2**64-1 + // HeaderLength cannot be an odd number + // + if ( (FwVolHeader->Revision != EFI_FVH_REVISION) + || (FwVolHeader->Signature != EFI_FVH_SIGNATURE) + || (FwVolHeader->FvLength != FvLength) + ) + { + DEBUG ((DEBUG_INFO, "%a: No Firmware Volume header present\n", + __FUNCTION__)); + return EFI_NOT_FOUND; + } + + // Check the Firmware Volume Guid + if (!CompareGuid (&FwVolHeader->FileSystemGuid, &gEfiSystemNvDataFvGuid)) { [SAMI] Not a comment for this patch but I noticed that BaseTools has an implementation of CompareGuid() that returns INTN. I wonder if that is intentional. Moreover, it also appears that the CompareGuid() usage in BaseTools does not consistently follow the explicit comparison rule (https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false). [/SAMI] + DEBUG ((DEBUG_INFO, "%a: Firmware Volume Guid non-compatible\n", + __FUNCTION__)); + return EFI_NOT_FOUND; + } + + // Verify the header checksum + Checksum = CalculateSum16((UINT16*)FwVolHeader, FwVolHeader->HeaderLength); + if (Checksum != 0) { + DEBUG ((DEBUG_INFO, "%a: FV checksum is invalid (Checksum:0x%X)\n", + __FUNCTION__, Checksum)); + return EFI_NOT_FOUND; [SAMI] Minor: By this point we should be fairly certain that this is a Firmware Volume header. So, should the error code returned here be EFI_VOLUME_CORRUPTED? Would the same apply to the remaining checks below? [/SAMI] + } + + VariableStoreHeader = (VOID *)((UINTN)FwVolHeader + [SAMI] Should the typecast be (VARIABLE_STORE_HEADER*) instead of (VOID *)? [/SAMI] + FwVolHeader->HeaderLength); + + // Check the Variable Store Guid + if (!CompareGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid) && + !CompareGuid (&VariableStoreHeader->Signature, + &gEfiAuthenticatedVariableGuid)) { + DEBUG ((DEBUG_INFO, "%a: Variable Store Guid non-compatible\n", + __FUNCTION__)); + return EFI_NOT_FOUND; + } + + VariableStoreLength = PcdGet32 (PcdFlashNvStorageVariableSize) - + FwVolHeader->HeaderLength; + if (VariableStoreHeader->Size != VariableStoreLength) { + DEBUG ((DEBUG_INFO, "%a: Variable Store Length does not match\n", + __FUNCTION__)); + return EFI_NOT_FOUND; + } + + return EFI_SUCCESS; + +} + +STATIC +EFI_STATUS +InitializeFvAndVariableStoreHeaders ( + MEM_INSTANCE *Instance + ) +{ + EFI_FIRMWARE_VOLUME_HEADER *FirmwareVolumeHeader; + VARIABLE_STORE_HEADER *VariableStoreHeader; + EFI_STATUS Status = EFI_SUCCESS; + UINTN HeadersLength; + VOID* Headers; + + HeadersLength = sizeof(EFI_FIRMWARE_VOLUME_HEADER) + + sizeof(EFI_FV_BLOCK_MAP_ENTRY) + + sizeof(VARIABLE_STORE_HEADER); + Headers = AllocateZeroPool(HeadersLength); + + // + // EFI_FIRMWARE_VOLUME_HEADER + // + FirmwareVolumeHeader = (EFI_FIRMWARE_VOLUME_HEADER*)Headers; + CopyGuid (&FirmwareVolumeHeader->FileSystemGuid, &gEfiSystemNvDataFvGuid); + FirmwareVolumeHeader->FvLength = + PcdGet32(PcdFlashNvStorageVariableSize) + + PcdGet32(PcdFlashNvStorageFtwWorkingSize) + + PcdGet32(PcdFlashNvStorageFtwSpareSize); + FirmwareVolumeHeader->Signature = EFI_FVH_SIGNATURE; + FirmwareVolumeHeader->Attributes = EFI_FVB2_READ_ENABLED_CAP | + EFI_FVB2_READ_STATUS | + EFI_FVB2_STICKY_WRITE | + EFI_FVB2_MEMORY_MAPPED | + EFI_FVB2_ERASE_POLARITY | + EFI_FVB2_WRITE_STATUS | + EFI_FVB2_WRITE_ENABLED_CAP; + + FirmwareVolumeHeader->HeaderLength = sizeof(EFI_FIRMWARE_VOLUME_HEADER) + + sizeof(EFI_FV_BLOCK_MAP_ENTRY); + FirmwareVolumeHeader->Revision = EFI_FVH_REVISION; + FirmwareVolumeHeader->BlockMap[0].NumBlocks = Instance->NBlocks; + FirmwareVolumeHeader->BlockMap[0].Length = Instance->BlockSize; + FirmwareVolumeHeader->BlockMap[1].NumBlocks = 0; + FirmwareVolumeHeader->BlockMap[1].Length = 0; + FirmwareVolumeHeader->Checksum = CalculateCheckSum16 ( + (UINT16*)FirmwareVolumeHeader, + FirmwareVolumeHeader->HeaderLength); + + // + // VARIABLE_STORE_HEADER + // + VariableStoreHeader = (VOID *)((UINTN)Headers + [SAMI] Should the typecase be (VARIABLE_STORE_HEADER*) instead of (VOID *)? [/SAMI] + FirmwareVolumeHeader->HeaderLength); + CopyGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid); + VariableStoreHeader->Size = PcdGet32(PcdFlashNvStorageVariableSize) - + FirmwareVolumeHeader->HeaderLength; + VariableStoreHeader->Format = VARIABLE_STORE_FORMATTED; + VariableStoreHeader->State = VARIABLE_STORE_HEALTHY; + + Status = ReadWriteRpmb(SP_SVC_RPMB_WRITE, (UINTN) Headers, HeadersLength, 0); + if (Status != EFI_SUCCESS) { [SAMI] if (EFI_ERROR (Status)) ? [/SAMI] + goto Exit; + } + // Install the combined header in memory + CopyMem ((VOID*) Instance->MemBaseAddress, Headers, HeadersLength); + +Exit: + FreePool (Headers); + return Status; +} + +STATIC +EFI_STATUS +EFIAPI +FvbInitialize ( + MEM_INSTANCE *Instance + ) +{ + EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader; + EFI_STATUS Status; + + if (Instance->Initialized == TRUE) { [SAMI] if (Instance->Initialized) ? See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false [/SAMI] + return EFI_SUCCESS; + } + + // FirmwareVolumeHeader->FvLength is declared to have the Variable area + // AND the FTW working area AND the FTW Spare contiguous. + ASSERT (PcdGet32 (PcdFlashNvStorageVariableBase) + + PcdGet32 (PcdFlashNvStorageVariableSize) == + PcdGet32 (PcdFlashNvStorageFtwWorkingBase)); + ASSERT (PcdGet32 (PcdFlashNvStorageFtwWorkingBase) + + PcdGet32 (PcdFlashNvStorageFtwWorkingSize) == + PcdGet32 (PcdFlashNvStorageFtwSpareBase)); + + // Check if the size of the area is at least one block size + ASSERT ((PcdGet32 (PcdFlashNvStorageVariableSize) > 0) && + (PcdGet32 (PcdFlashNvStorageVariableSize) / Instance->BlockSize > 0)); + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > 0) && + (PcdGet32 (PcdFlashNvStorageFtwWorkingSize) / Instance->BlockSize > 0)); + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwSpareSize) > 0) && + (PcdGet32 (PcdFlashNvStorageFtwSpareSize) / Instance->BlockSize > 0)); + + // Ensure the Variable areas are aligned on block size boundaries + ASSERT ((PcdGet32 (PcdFlashNvStorageVariableBase) % Instance->BlockSize) == 0); + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwWorkingBase) % Instance->BlockSize) == 0); + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwSpareBase) % Instance->BlockSize) == 0); [SAMI] These asserts may need to be adjusted if 64-bit versions of the PCDs are used. [/SAMI] + + // Read the file from disk and copy it to memory + ReadEntireFlash (Instance); + + FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *) Instance->MemBaseAddress; + Status = ValidateFvHeader(FwVolHeader); + if (EFI_ERROR (Status)) { + // There is no valid header, so time to install one. + DEBUG ((DEBUG_INFO, "%a: The FVB Header is not valid.\n", __FUNCTION__)); + + // Reset memory + SetMem64 ((VOID *)Instance->MemBaseAddress, Instance->NBlocks * Instance->BlockSize, ~0UL); + DEBUG ((DEBUG_INFO, "%a: Erasing Flash.\n", __FUNCTION__)); + Status = ReadWriteRpmb(SP_SVC_RPMB_WRITE, Instance->MemBaseAddress, + PcdGet32(PcdFlashNvStorageVariableSize) + + PcdGet32(PcdFlashNvStorageFtwWorkingSize) + + PcdGet32(PcdFlashNvStorageFtwSpareSize), 0); + if (Status != EFI_SUCCESS) { [SAMI] if (EFI_ERROR (Status)) ? [/SAMI] + return Status; + } + // Install all appropriate headers + DEBUG ((DEBUG_INFO, "%a: Installing a correct one for this volume.\n", + __FUNCTION__)); + Status = InitializeFvAndVariableStoreHeaders (Instance); + if (EFI_ERROR (Status)) { + return Status; + } + } else { + DEBUG ((DEBUG_INFO, "%a: Found valid FVB Header.\n", __FUNCTION__)); + } + Instance->Initialized = TRUE; + + return EFI_SUCCESS; [SAMI] return Status; ? [/SAMI] +} + +EFI_STATUS +EFIAPI +OpTeeRpmbFvbInit ( + IN EFI_HANDLE ImageHandle, + IN EFI_MM_SYSTEM_TABLE *SystemTable + ) +{ + EFI_STATUS Status; + VOID *Addr; + UINTN FvLength; + UINTN NBlocks; + + FvLength = PcdGet32(PcdFlashNvStorageVariableSize) + + PcdGet32(PcdFlashNvStorageFtwWorkingSize) + + PcdGet32(PcdFlashNvStorageFtwSpareSize); + + NBlocks = EFI_SIZE_TO_PAGES(ALIGN_VARIABLE(FvLength)); + Addr = AllocatePages(NBlocks); + ASSERT (Addr != NULL); [SAMI] Asserts will vanish in release builds and a failure to allocate memory would result in incorrect results. Please handle this error and return EFI_OUT_OF_RESOURCES. [/SAMI] + + SetMem (&mInstance, sizeof (mInstance), 0); + + mInstance.FvbProtocol.GetPhysicalAddress = OpTeeRpmbFvbGetPhysicalAddress; + mInstance.FvbProtocol.GetAttributes = OpTeeRpmbFvbGetAttributes; + mInstance.FvbProtocol.SetAttributes = OpTeeRpmbFvbSetAttributes; + mInstance.FvbProtocol.GetBlockSize = OpTeeRpmbFvbGetBlockSize; + mInstance.FvbProtocol.EraseBlocks = OpTeeRpmbFvbErase; + mInstance.FvbProtocol.Write = OpTeeRpmbFvbWrite; + mInstance.FvbProtocol.Read = OpTeeRpmbFvbRead; + + mInstance.MemBaseAddress = (EFI_PHYSICAL_ADDRESS) Addr; + mInstance.Signature = FLASH_SIGNATURE; + mInstance.Initialize = FvbInitialize; + mInstance.BlockSize = EFI_PAGE_SIZE; + mInstance.NBlocks = NBlocks; + + // Update the defined PCDs related to Variable Storage + PatchPcdSet32 (PcdFlashNvStorageVariableBase, mInstance.MemBaseAddress); [SAMI] Should the 64-bit versions of the PCDs be used here. A recent change added similar support to the ArmPlatformPkg\Drivers\NorFlashDxe. [/SAMI] + PatchPcdSet32 (PcdFlashNvStorageFtwWorkingBase, mInstance.MemBaseAddress + + PcdGet32 (PcdFlashNvStorageVariableSize)); + PatchPcdSet32 (PcdFlashNvStorageFtwSpareBase, mInstance.MemBaseAddress + + PcdGet32 (PcdFlashNvStorageVariableSize) + + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)); + + Status = gMmst->MmInstallProtocolInterface ( + &mInstance.Handle, + &gEfiSmmFirmwareVolumeBlockProtocolGuid, + EFI_NATIVE_INTERFACE, + &mInstance.FvbProtocol + ); + ASSERT_EFI_ERROR (Status); + + DEBUG ((DEBUG_INFO, "%a: Register OP-TEE RPMB Fvb\n", __FUNCTION__)); + DEBUG ((DEBUG_INFO, "%a: Using NV store FV in-memory copy at 0x%lx\n", + __FUNCTION__, PatchPcdGet32 (PcdFlashNvStorageVariableBase))); + + return Status; +} -- 2.17.1 IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver 2021-01-27 17:10 ` Sami Mujawar @ 2021-01-29 8:02 ` Ilias Apalodimas 2021-01-29 11:45 ` Sami Mujawar 2021-02-01 14:00 ` Ilias Apalodimas 0 siblings, 2 replies; 14+ messages in thread From: Ilias Apalodimas @ 2021-01-29 8:02 UTC (permalink / raw) To: Sami Mujawar Cc: Sughosh Ganu, devel@edk2.groups.io, Ard Biesheuvel, Leif Lindholm, Sahil Malhotra Hi Sami, Thanks for taking the time On Wed, 27 Jan 2021 at 19:10, Sami Mujawar <Sami.Mujawar@arm.com> wrote: > > Hi Sughosh, > > There are some edk2 coding standard incompatibilities (like space between function name and opening bracket, function parameter alignment etc.) and documentation for some functions is missing in the patch. Can you fix these, please? > Please also run the ECC tool in Drivers/OpTeeRpmb folder and fix any reported issues. The ECC errors 10002, 10006 and 10022 can be ignored for now. > > Other than that, please find my response inline marked [SAMI]. > > Regards, > > Sami Mujawar > > -----Original Message----- > From: Sughosh Ganu <sughosh.ganu@linaro.org> > Sent: 16 December 2020 11:09 AM > To: devel@edk2.groups.io > Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Leif Lindholm <leif@nuviainc.com>; Sahil Malhotra <sahil.malhotra@linaro.org>; Ilias Apalodimas <ilias.apalodimas@linaro.org> > Subject: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver > > From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > A following patch is adding support for building StMM in order to run it > from OP-TEE. > OP-TEE in combination with a NS-world supplicant can use the RPMB > partition of an eMMC to store EFI variables. The supplicant > functionality is currently available in U-Boot only but can be ported > into EDK2. Assuming similar functionality is added in EDK2, this will > allow any hardware with an RPMB partition to store EFI variables > securely. > > So let's add a driver that enables access of the RPMB partition through > OP-TEE. Since the upper layers expect a byte addressable interface, > the driver allocates memory and patches the PCDs, while syncing the > memory/hardware on read/write callbacks. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> [...] > --- /dev/null > +++ b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.h > @@ -0,0 +1,35 @@ > +/** @file > + > + Copyright (c) 2020, Linaro Ltd. All rights reserved.<BR> > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#ifndef __OPTEE_RPMB_FV_ > +#define __OPTEE_RPMB_FV_ > [SAMI] This does not follow the edk2 coding standard. See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/53_include_files#5-3-5-all-include-file-contents-must-be-protected-by-a-include-guard > [/SAMI] Ok > + > +/* SVC Args */ > +#define SP_SVC_RPMB_READ 0xC4000066 > +#define SP_SVC_RPMB_WRITE 0xC4000067 > [SAMI] Is there a specification reference for this? If so, please add to the file header. > See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-3-1-every-new-file-shall-begin-with-a-file-header-comment-block. > [/SAMI] No there isn't. Since those are FFA calls it's something we define internally in OP-TEE. We had a discussion with Arm about standardizing this in the future, but it can wait until we get full FFA and SP support in OP-TEE. The rpmb 'driver' is essentially a bunch of OP-TEE SVC calls that end up using the OP-TEE APIs for accessing an RPMB partition. I could add a reference to the OP-TEE code if that makes sense? > + > +#define FILENAME "EFI_VARS" > + > +#define FLASH_SIGNATURE SIGNATURE_32('r', 'p', 'm', 'b') > +#define INSTANCE_FROM_FVB_THIS(a) CR(a, MEM_INSTANCE, FvbProtocol, \ > + FLASH_SIGNATURE) > + > +typedef struct _MEM_INSTANCE MEM_INSTANCE; > +typedef EFI_STATUS (*MEM_INITIALIZE) (MEM_INSTANCE* Instance); > +struct _MEM_INSTANCE > +{ > + UINT32 Signature; > + MEM_INITIALIZE Initialize; > + BOOLEAN Initialized; > + EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL FvbProtocol; > + EFI_HANDLE Handle; > + EFI_PHYSICAL_ADDRESS MemBaseAddress; > + UINT16 BlockSize; > + UINT16 NBlocks; > +}; > [SAMI] Please add documentation for structure. See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/56_declarations_and_types#5-6-3-2-structure-declaration-with-forward-reference-or-self-reference > [/SAMI] Ok > + > +#endif > diff --git a/Drivers/OpTeeRpmb/FixupPcd.c b/Drivers/OpTeeRpmb/FixupPcd.c > new file mode 100644 [...] > + > + Instance = INSTANCE_FROM_FVB_THIS(FvbProtocol); > + // Patch PCDs with the the correct values > + PatchPcdSet32 (PcdFlashNvStorageVariableBase, Instance->MemBaseAddress); > + PatchPcdSet32 (PcdFlashNvStorageFtwWorkingBase, Instance->MemBaseAddress + > + PcdGet32 (PcdFlashNvStorageVariableSize)); > + PatchPcdSet32 (PcdFlashNvStorageFtwSpareBase, Instance->MemBaseAddress + > + PcdGet32 (PcdFlashNvStorageVariableSize) + > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize));NorFlashDxe > [SAMI] Should the 64-bit versions of the PCDs be used here. > A recent change added similar support to the ArmPlatformPkg\Drivers\NorFlashDxe. > [/SAMI] Looking at the NorFlashDxe commit, this is needed for NOR flash devices connected in a 64-bit address space. In our case OP-TEE maps the memory StMM can use, so that depends on the platform and how OP-TEE is layed out in memory. I'll have a look on the current OP-TEE supported platforms and if that's a case I'll change it. > + > + DEBUG ((DEBUG_INFO, "%a: Fixup PcdFlashNvStorageVariableBase: 0x%lx\n", [...] > + > +#include "OpTeeRpmbFvb.h" > + > +static const UINT16 mem_mgr_id = 3U; > +static const UINT16 storage_id = 4U; > [SAMI] Please change to STATIC CONST and also update the variable names according to the edk2 coding standard. See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/4_naming_conventions/43_identifiers#4-3-3-2-any-variable-with-file-scope-or-better-shall-be-prefixed-by-an-m-or-g > [/SAMI] Ok > + > +STATIC MEM_INSTANCE mInstance; > + > +/** > + Sends an SVC call to OP-TEE for reading/writing an RPMB partition > + > + @param SvcAct SVC ID for read/write > + @param Addr Base address of the buffer. When reading contents will be > + copied to that buffer after reading them from the device. > + When writing, the buffer holds the contents we want to > + write cwtoin the device > + @param NumBytes Number of bytes to read/write > + @param Offset Offset into the RPMB file > + > + @retval OP-TEE return code > +**/ > + > [SAMI] Remove blank line. Same at other places as well. > [/SAMI] Ok > +STATIC > +EFI_STATUS > +ReadWriteRpmb ( > + UINTN SvcAct, > + UINTN Addr, > + UINTN NumBytes, > + UINTN Offset > + ) > +{ > + ARM_SVC_ARGS SvcArgs; > + EFI_STATUS Status; > + > + ZeroMem (&SvcArgs, sizeof (SvcArgs)); > + > + SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64; > + SvcArgs.Arg1 = storage_id; > + SvcArgs.Arg2 = 0; > + SvcArgs.Arg3 = SvcAct; > + SvcArgs.Arg4 = Addr; > + SvcArgs.Arg5 = NumBytes; > + SvcArgs.Arg6 = Offset; > + > + ArmCallSvc (&SvcArgs); > + if (SvcArgs.Arg3) { > + DEBUG ((DEBUG_ERROR, "%a: Svc Call 0x%08x Addr: 0x%08x len: 0x%x Offset: 0x%x failed with 0x%x\n", > + __func__, SvcAct, Addr, NumBytes, Offset, SvcArgs.Arg3)); > + } > + > + switch (SvcArgs.Arg3) { > + case ARM_SVC_SPM_RET_SUCCESS: > + Status = EFI_SUCCESS; > + break; > + > + case ARM_SVC_SPM_RET_NOT_SUPPORTED: > + Status = EFI_UNSUPPORTED; > + break; > + > + case ARM_SVC_SPM_RET_INVALID_PARAMS: > + Status = EFI_INVALID_PARAMETER; > + break; > + > + case ARM_SVC_SPM_RET_DENIED: > + Status = EFI_ACCESS_DENIED; > + break; > + > + case ARM_SVC_SPM_RET_NO_MEMORY: > + Status = EFI_BAD_BUFFER_SIZE; > + break; > + > + default: > + Status = EFI_ACCESS_DENIED; > + } > [SAMI] Should the error handling here be updated similar to the FF-A StandaloneMmPkg patches? > [/SAMI] I actually picked up the error handling from the previous non-FFA code. I'll check what's on Sughosh latest patches and fix it if there are any differences. Looking at it again EFI_BAD_BUFFER_SIZE can change to indicate out of memory properly anyway. > + > + return Status; > +} [...] > +STATIC > +EFI_STATUS > +OpTeeRpmbFvbRead ( > + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, > + IN EFI_LBA Lba, > + IN UINTN Offset, > + IN OUT UINTN *NumBytes, > + IN OUT UINT8 *Buffer > + ) > +{ > + EFI_STATUS Status = EFI_SUCCESS; > + MEM_INSTANCE *Instance; > + VOID *Base; > + > + Instance = INSTANCE_FROM_FVB_THIS(This); > + if (Instance->Initialized == FALSE) { > [SAMI] if (!Instance->Initialized) ? > See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false > [/SAMI] Ok > + Instance->Initialize (Instance); > + } > + > + Base = (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize + Offset; [...] > + MEM_INSTANCE *Instance; > + EFI_STATUS Status = EFI_SUCCESS; > + VOID *Base; > + > + Instance = INSTANCE_FROM_FVB_THIS(This); > + if (Instance->Initialized == FALSE) { > [SAMI] if (!Instance->Initialized) ? > See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false > [/SAMI] sure > + Instance->Initialize (Instance); > + } > + Base = (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize + Offset; > + Status = ReadWriteRpmb (SP_SVC_RPMB_WRITE, (UINTN) Buffer, *NumBytes, > + Lba * Instance->BlockSize + Offset); > + if (Status != EFI_SUCCESS) { > [SAMI] if (EFI_ERROR (Status)) ? > [/SAMI] Ok > + return Status; > + } [...] > + if (!Buf) { > + return EFI_DEVICE_ERROR; > + } > + SetMem64 (Buf, NumLba * Instance->BlockSize, ~0UL); > + // Write the device > + Status = ReadWriteRpmb (SP_SVC_RPMB_WRITE, (UINTN) Buf, NumBytes, > + Start * Instance->BlockSize); > + if (Status != EFI_SUCCESS) { > [SAMI] if (EFI_ERROR (Status)) ? > [/SAMI] > + return Status; Ok > + } > + // Update the in memory copy > + SetMem64 (Base, NumLba * Instance->BlockSize, ~0UL); > + FreePool (Buf); > + } > + > + VA_END (Args); > + > + return EFI_SUCCESS; > +} > + > +/** > + Since we use a memory backed storage we need to restore the RPMB contents > + into memory before we register the Fvb protocol. > + > + @param Instace Address to copy flash contents to > + > + @retval 0 on success, OP-TEE error on failure > +**/ > +STATIC > +VOID > +ReadEntireFlash ( > + MEM_INSTANCE *Instance > + ) > +{ > + UINTN ReadAddr; > + > + UINTN StorageFtwWorkingSize = PcdGet32(PcdFlashNvStorageFtwWorkingSize); > + UINTN StorageVariableSize = PcdGet32(PcdFlashNvStorageVariableSize); > + UINTN StorageFtwSpareSize = PcdGet32(PcdFlashNvStorageFtwSpareSize); > + > + ReadAddr = Instance->MemBaseAddress; > + // There's no need to check if the read failed here. The upper EDK2 layers > + // will initialize the flash correctly if the in-memory copy is wrong > + ReadWriteRpmb(SP_SVC_RPMB_READ, ReadAddr, StorageVariableSize + > + StorageFtwWorkingSize + StorageFtwSpareSize , 0); > +} > + > + > +STATIC > +EFI_STATUS > +EFIAPI > +ValidateFvHeader ( > + IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader > + ) > +{ > + UINT16 Checksum; > + VARIABLE_STORE_HEADER *VariableStoreHeader; > + UINTN VariableStoreLength; > + UINTN FvLength; > + > + FvLength = PcdGet32(PcdFlashNvStorageVariableSize) + > + PcdGet32(PcdFlashNvStorageFtwWorkingSize) + > + PcdGet32(PcdFlashNvStorageFtwSpareSize); > + > + // Verify the header revision, header signature, length > + // Length of FvBlock cannot be 2**64-1 > + // HeaderLength cannot be an odd number > + // > + if ( (FwVolHeader->Revision != EFI_FVH_REVISION) > + || (FwVolHeader->Signature != EFI_FVH_SIGNATURE) > + || (FwVolHeader->FvLength != FvLength) > + ) > + { > + DEBUG ((DEBUG_INFO, "%a: No Firmware Volume header present\n", > + __FUNCTION__)); > + return EFI_NOT_FOUND; > + } > + > + // Check the Firmware Volume Guid > + if (!CompareGuid (&FwVolHeader->FileSystemGuid, &gEfiSystemNvDataFvGuid)) { > [SAMI] Not a comment for this patch but I noticed that BaseTools has an implementation of CompareGuid() that returns INTN. > I wonder if that is intentional. Moreover, it also appears that the CompareGuid() usage in BaseTools does not consistently follow the explicit comparison rule (https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false). > [/SAMI] > + DEBUG ((DEBUG_INFO, "%a: Firmware Volume Guid non-compatible\n", > + __FUNCTION__)); > + return EFI_NOT_FOUND; > + } > + > + // Verify the header checksum > + Checksum = CalculateSum16((UINT16*)FwVolHeader, FwVolHeader->HeaderLength); > + if (Checksum != 0) { > + DEBUG ((DEBUG_INFO, "%a: FV checksum is invalid (Checksum:0x%X)\n", > + __FUNCTION__, Checksum)); > + return EFI_NOT_FOUND; > [SAMI] Minor: By this point we should be fairly certain that this is a Firmware Volume header. > So, should the error code returned here be EFI_VOLUME_CORRUPTED? Would the same apply to the remaining checks below? > [/SAMI] I think that makes sense, but I'll have to check if there's a difference in behavior of the upper layers depending on the return value. The relevant Volume Header checks is a c/p from ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c and I kept it for consistency. Should we change the other driver as well at some point? > + } > + > + VariableStoreHeader = (VOID *)((UINTN)FwVolHeader + > [SAMI] Should the typecast be (VARIABLE_STORE_HEADER*) instead of (VOID *)? > [/SAMI] Yes > + FwVolHeader->HeaderLength); > + > + // Check the Variable Store Guid [...] > + FirmwareVolumeHeader->Revision = EFI_FVH_REVISION; > + FirmwareVolumeHeader->BlockMap[0].NumBlocks = Instance->NBlocks; > + FirmwareVolumeHeader->BlockMap[0].Length = Instance->BlockSize; > + FirmwareVolumeHeader->BlockMap[1].NumBlocks = 0; > + FirmwareVolumeHeader->BlockMap[1].Length = 0; > + FirmwareVolumeHeader->Checksum = CalculateCheckSum16 ( > + (UINT16*)FirmwareVolumeHeader, > + FirmwareVolumeHeader->HeaderLength); > + > + // > + // VARIABLE_STORE_HEADER > + // > + VariableStoreHeader = (VOID *)((UINTN)Headers + > [SAMI] Should the typecase be (VARIABLE_STORE_HEADER*) instead of (VOID *)? > [/SAMI] Yes > + FirmwareVolumeHeader->HeaderLength); > + CopyGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid); > + VariableStoreHeader->Size = PcdGet32(PcdFlashNvStorageVariableSize) - > + FirmwareVolumeHeader->HeaderLength; > + VariableStoreHeader->Format = VARIABLE_STORE_FORMATTED; > + VariableStoreHeader->State = VARIABLE_STORE_HEALTHY; > + > + Status = ReadWriteRpmb(SP_SVC_RPMB_WRITE, (UINTN) Headers, HeadersLength, 0); > + if (Status != EFI_SUCCESS) { > [SAMI] if (EFI_ERROR (Status)) ? > [/SAMI] Ok > + goto Exit; > + } > + // Install the combined header in memory > + CopyMem ((VOID*) Instance->MemBaseAddress, Headers, HeadersLength); > + > +Exit: > + FreePool (Headers); > + return Status; > +} > + > +STATIC > +EFI_STATUS > +EFIAPI > +FvbInitialize ( > + MEM_INSTANCE *Instance > + ) > +{ > + EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader; > + EFI_STATUS Status; > + > + if (Instance->Initialized == TRUE) { > [SAMI] if (Instance->Initialized) ? > See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false > [/SAMI] Sure > + return EFI_SUCCESS; > + } > + > + // FirmwareVolumeHeader->FvLength is declared to have the Variable area > + // AND the FTW working area AND the FTW Spare contiguous. > + ASSERT (PcdGet32 (PcdFlashNvStorageVariableBase) + > + PcdGet32 (PcdFlashNvStorageVariableSize) == > + PcdGet32 (PcdFlashNvStorageFtwWorkingBase)); > + ASSERT (PcdGet32 (PcdFlashNvStorageFtwWorkingBase) + > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize) == > + PcdGet32 (PcdFlashNvStorageFtwSpareBase)); > + > + // Check if the size of the area is at least one block size > + ASSERT ((PcdGet32 (PcdFlashNvStorageVariableSize) > 0) && > + (PcdGet32 (PcdFlashNvStorageVariableSize) / Instance->BlockSize > 0)); > + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > 0) && > + (PcdGet32 (PcdFlashNvStorageFtwWorkingSize) / Instance->BlockSize > 0)); > + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwSpareSize) > 0) && > + (PcdGet32 (PcdFlashNvStorageFtwSpareSize) / Instance->BlockSize > 0)); > + > + // Ensure the Variable areas are aligned on block size boundaries > + ASSERT ((PcdGet32 (PcdFlashNvStorageVariableBase) % Instance->BlockSize) == 0); > + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwWorkingBase) % Instance->BlockSize) == 0); > + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwSpareBase) % Instance->BlockSize) == 0); > [SAMI] These asserts may need to be adjusted if 64-bit versions of the PCDs are used. > [/SAMI] Noted > + > + // Read the file from disk and copy it to memory > + ReadEntireFlash (Instance); > + > + FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *) Instance->MemBaseAddress; > + Status = ValidateFvHeader(FwVolHeader); > + if (EFI_ERROR (Status)) { > + // There is no valid header, so time to install one. > + DEBUG ((DEBUG_INFO, "%a: The FVB Header is not valid.\n", __FUNCTION__)); > + > + // Reset memory > + SetMem64 ((VOID *)Instance->MemBaseAddress, Instance->NBlocks * Instance->BlockSize, ~0UL); > + DEBUG ((DEBUG_INFO, "%a: Erasing Flash.\n", __FUNCTION__)); > + Status = ReadWriteRpmb(SP_SVC_RPMB_WRITE, Instance->MemBaseAddress, > + PcdGet32(PcdFlashNvStorageVariableSize) + > + PcdGet32(PcdFlashNvStorageFtwWorkingSize) + > + PcdGet32(PcdFlashNvStorageFtwSpareSize), 0); > + if (Status != EFI_SUCCESS) { > [SAMI] if (EFI_ERROR (Status)) ? > [/SAMI] Sure > + return Status; > + } > + // Install all appropriate headers > + DEBUG ((DEBUG_INFO, "%a: Installing a correct one for this volume.\n", > + __FUNCTION__)); > + Status = InitializeFvAndVariableStoreHeaders (Instance); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + } else { > + DEBUG ((DEBUG_INFO, "%a: Found valid FVB Header.\n", __FUNCTION__)); > + } > + Instance->Initialized = TRUE; > + > + return EFI_SUCCESS; > [SAMI] return Status; ? > [/SAMI] > +} Sure > + > +EFI_STATUS > +EFIAPI > +OpTeeRpmbFvbInit ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_MM_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + VOID *Addr; > + UINTN FvLength; > + UINTN NBlocks; > + > + FvLength = PcdGet32(PcdFlashNvStorageVariableSize) + > + PcdGet32(PcdFlashNvStorageFtwWorkingSize) + > + PcdGet32(PcdFlashNvStorageFtwSpareSize); > + > + NBlocks = EFI_SIZE_TO_PAGES(ALIGN_VARIABLE(FvLength)); > + Addr = AllocatePages(NBlocks); > + ASSERT (Addr != NULL); > [SAMI] Asserts will vanish in release builds and a failure to allocate memory would result in incorrect results. > Please handle this error and return EFI_OUT_OF_RESOURCES. > [/SAMI] Fair enough > + > + SetMem (&mInstance, sizeof (mInstance), 0); > + > + mInstance.FvbProtocol.GetPhysicalAddress = OpTeeRpmbFvbGetPhysicalAddress; > + mInstance.FvbProtocol.GetAttributes = OpTeeRpmbFvbGetAttributes; > + mInstance.FvbProtocol.SetAttributes = OpTeeRpmbFvbSetAttributes; > + mInstance.FvbProtocol.GetBlockSize = OpTeeRpmbFvbGetBlockSize; > + mInstance.FvbProtocol.EraseBlocks = OpTeeRpmbFvbErase; > + mInstance.FvbProtocol.Write = OpTeeRpmbFvbWrite; > + mInstance.FvbProtocol.Read = OpTeeRpmbFvbRead; > + > + mInstance.MemBaseAddress = (EFI_PHYSICAL_ADDRESS) Addr; > + mInstance.Signature = FLASH_SIGNATURE; > + mInstance.Initialize = FvbInitialize; > + mInstance.BlockSize = EFI_PAGE_SIZE; > + mInstance.NBlocks = NBlocks; > + > + // Update the defined PCDs related to Variable Storage > + PatchPcdSet32 (PcdFlashNvStorageVariableBase, mInstance.MemBaseAddress); > [SAMI] Should the 64-bit versions of the PCDs be used here. > A recent change added similar support to the ArmPlatformPkg\Drivers\NorFlashDxe. > [/SAMI] Noted > + PatchPcdSet32 (PcdFlashNvStorageFtwWorkingBase, mInstance.MemBaseAddress + > + PcdGet32 (PcdFlashNvStorageVariableSize)); > + PatchPcdSet32 (PcdFlashNvStorageFtwSpareBase, mInstance.MemBaseAddress + > + PcdGet32 (PcdFlashNvStorageVariableSize) + > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)); > + > + Status = gMmst->MmInstallProtocolInterface ( > + &mInstance.Handle, > + &gEfiSmmFirmwareVolumeBlockProtocolGuid, > + EFI_NATIVE_INTERFACE, > + &mInstance.FvbProtocol > + ); > + ASSERT_EFI_ERROR (Status); > + > + DEBUG ((DEBUG_INFO, "%a: Register OP-TEE RPMB Fvb\n", __FUNCTION__)); > + DEBUG ((DEBUG_INFO, "%a: Using NV store FV in-memory copy at 0x%lx\n", > + __FUNCTION__, PatchPcdGet32 (PcdFlashNvStorageVariableBase))); > + > + return Status; > +} > -- > 2.17.1 > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver 2021-01-29 8:02 ` Ilias Apalodimas @ 2021-01-29 11:45 ` Sami Mujawar 2021-02-01 14:00 ` Ilias Apalodimas 1 sibling, 0 replies; 14+ messages in thread From: Sami Mujawar @ 2021-01-29 11:45 UTC (permalink / raw) To: Ilias Apalodimas Cc: Sughosh Ganu, devel@edk2.groups.io, ardb+tianocore@kernel.org, Leif Lindholm, Sahil Malhotra, nd Hi Ilias, Please find my response inline marked [SAMI]. Regards, Sami Mujawar -----Original Message----- From: Ilias Apalodimas <ilias.apalodimas@linaro.org> Sent: 29 January 2021 08:02 AM To: Sami Mujawar <Sami.Mujawar@arm.com> Cc: Sughosh Ganu <sughosh.ganu@linaro.org>; devel@edk2.groups.io; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Leif Lindholm <leif@nuviainc.com>; Sahil Malhotra <sahil.malhotra@linaro.org> Subject: Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver Hi Sami, Thanks for taking the time On Wed, 27 Jan 2021 at 19:10, Sami Mujawar <Sami.Mujawar@arm.com> wrote: > > Hi Sughosh, > > There are some edk2 coding standard incompatibilities (like space between function name and opening bracket, function parameter alignment etc.) and documentation for some functions is missing in the patch. Can you fix these, please? > Please also run the ECC tool in Drivers/OpTeeRpmb folder and fix any reported issues. The ECC errors 10002, 10006 and 10022 can be ignored for now. > > Other than that, please find my response inline marked [SAMI]. > > Regards, > > Sami Mujawar > > -----Original Message----- > From: Sughosh Ganu <sughosh.ganu@linaro.org> > Sent: 16 December 2020 11:09 AM > To: devel@edk2.groups.io > Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Leif Lindholm <leif@nuviainc.com>; Sahil Malhotra <sahil.malhotra@linaro.org>; Ilias Apalodimas <ilias.apalodimas@linaro.org> > Subject: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver > > From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > A following patch is adding support for building StMM in order to run it > from OP-TEE. > OP-TEE in combination with a NS-world supplicant can use the RPMB > partition of an eMMC to store EFI variables. The supplicant > functionality is currently available in U-Boot only but can be ported > into EDK2. Assuming similar functionality is added in EDK2, this will > allow any hardware with an RPMB partition to store EFI variables > securely. > > So let's add a driver that enables access of the RPMB partition through > OP-TEE. Since the upper layers expect a byte addressable interface, > the driver allocates memory and patches the PCDs, while syncing the > memory/hardware on read/write callbacks. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> [...] > --- /dev/null > +++ b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.h > @@ -0,0 +1,35 @@ > +/** @file > + > + Copyright (c) 2020, Linaro Ltd. All rights reserved.<BR> > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#ifndef __OPTEE_RPMB_FV_ > +#define __OPTEE_RPMB_FV_ > [SAMI] This does not follow the edk2 coding standard. See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/53_include_files#5-3-5-all-include-file-contents-must-be-protected-by-a-include-guard > [/SAMI] Ok > + > +/* SVC Args */ > +#define SP_SVC_RPMB_READ 0xC4000066 > +#define SP_SVC_RPMB_WRITE 0xC4000067 > [SAMI] Is there a specification reference for this? If so, please add to the file header. > See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-3-1-every-new-file-shall-begin-with-a-file-header-comment-block. > [/SAMI] No there isn't. Since those are FFA calls it's something we define internally in OP-TEE. We had a discussion with Arm about standardizing this in the future, but it can wait until we get full FFA and SP support in OP-TEE. The rpmb 'driver' is essentially a bunch of OP-TEE SVC calls that end up using the OP-TEE APIs for accessing an RPMB partition. I could add a reference to the OP-TEE code if that makes sense? [SAMI] I think that may be a good reference point for now. [/SAMI] > + > +#define FILENAME "EFI_VARS" > + > +#define FLASH_SIGNATURE SIGNATURE_32('r', 'p', 'm', 'b') > +#define INSTANCE_FROM_FVB_THIS(a) CR(a, MEM_INSTANCE, FvbProtocol, \ > + FLASH_SIGNATURE) > + > +typedef struct _MEM_INSTANCE MEM_INSTANCE; > +typedef EFI_STATUS (*MEM_INITIALIZE) (MEM_INSTANCE* Instance); > +struct _MEM_INSTANCE > +{ > + UINT32 Signature; > + MEM_INITIALIZE Initialize; > + BOOLEAN Initialized; > + EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL FvbProtocol; > + EFI_HANDLE Handle; > + EFI_PHYSICAL_ADDRESS MemBaseAddress; > + UINT16 BlockSize; > + UINT16 NBlocks; > +}; > [SAMI] Please add documentation for structure. See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/56_declarations_and_types#5-6-3-2-structure-declaration-with-forward-reference-or-self-reference > [/SAMI] Ok > + > +#endif > diff --git a/Drivers/OpTeeRpmb/FixupPcd.c b/Drivers/OpTeeRpmb/FixupPcd.c > new file mode 100644 [...] > + > + Instance = INSTANCE_FROM_FVB_THIS(FvbProtocol); > + // Patch PCDs with the the correct values > + PatchPcdSet32 (PcdFlashNvStorageVariableBase, Instance->MemBaseAddress); > + PatchPcdSet32 (PcdFlashNvStorageFtwWorkingBase, Instance->MemBaseAddress + > + PcdGet32 (PcdFlashNvStorageVariableSize)); > + PatchPcdSet32 (PcdFlashNvStorageFtwSpareBase, Instance->MemBaseAddress + > + PcdGet32 (PcdFlashNvStorageVariableSize) + > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize));NorFlashDxe > [SAMI] Should the 64-bit versions of the PCDs be used here. > A recent change added similar support to the ArmPlatformPkg\Drivers\NorFlashDxe. > [/SAMI] Looking at the NorFlashDxe commit, this is needed for NOR flash devices connected in a 64-bit address space. In our case OP-TEE maps the memory StMM can use, so that depends on the platform and how OP-TEE is layed out in memory. I'll have a look on the current OP-TEE supported platforms and if that's a case I'll change it. > + > + DEBUG ((DEBUG_INFO, "%a: Fixup PcdFlashNvStorageVariableBase: 0x%lx\n", [...] > + > +#include "OpTeeRpmbFvb.h" > + > +static const UINT16 mem_mgr_id = 3U; > +static const UINT16 storage_id = 4U; > [SAMI] Please change to STATIC CONST and also update the variable names according to the edk2 coding standard. See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/4_naming_conventions/43_identifiers#4-3-3-2-any-variable-with-file-scope-or-better-shall-be-prefixed-by-an-m-or-g > [/SAMI] Ok > + > +STATIC MEM_INSTANCE mInstance; > + > +/** > + Sends an SVC call to OP-TEE for reading/writing an RPMB partition > + > + @param SvcAct SVC ID for read/write > + @param Addr Base address of the buffer. When reading contents will be > + copied to that buffer after reading them from the device. > + When writing, the buffer holds the contents we want to > + write cwtoin the device > + @param NumBytes Number of bytes to read/write > + @param Offset Offset into the RPMB file > + > + @retval OP-TEE return code > +**/ > + > [SAMI] Remove blank line. Same at other places as well. > [/SAMI] Ok > +STATIC > +EFI_STATUS > +ReadWriteRpmb ( > + UINTN SvcAct, > + UINTN Addr, > + UINTN NumBytes, > + UINTN Offset > + ) > +{ > + ARM_SVC_ARGS SvcArgs; > + EFI_STATUS Status; > + > + ZeroMem (&SvcArgs, sizeof (SvcArgs)); > + > + SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64; > + SvcArgs.Arg1 = storage_id; > + SvcArgs.Arg2 = 0; > + SvcArgs.Arg3 = SvcAct; > + SvcArgs.Arg4 = Addr; > + SvcArgs.Arg5 = NumBytes; > + SvcArgs.Arg6 = Offset; > + > + ArmCallSvc (&SvcArgs); > + if (SvcArgs.Arg3) { > + DEBUG ((DEBUG_ERROR, "%a: Svc Call 0x%08x Addr: 0x%08x len: 0x%x Offset: 0x%x failed with 0x%x\n", > + __func__, SvcAct, Addr, NumBytes, Offset, SvcArgs.Arg3)); > + } > + > + switch (SvcArgs.Arg3) { > + case ARM_SVC_SPM_RET_SUCCESS: > + Status = EFI_SUCCESS; > + break; > + > + case ARM_SVC_SPM_RET_NOT_SUPPORTED: > + Status = EFI_UNSUPPORTED; > + break; > + > + case ARM_SVC_SPM_RET_INVALID_PARAMS: > + Status = EFI_INVALID_PARAMETER; > + break; > + > + case ARM_SVC_SPM_RET_DENIED: > + Status = EFI_ACCESS_DENIED; > + break; > + > + case ARM_SVC_SPM_RET_NO_MEMORY: > + Status = EFI_BAD_BUFFER_SIZE; > + break; > + > + default: > + Status = EFI_ACCESS_DENIED; > + } > [SAMI] Should the error handling here be updated similar to the FF-A StandaloneMmPkg patches? > [/SAMI] I actually picked up the error handling from the previous non-FFA code. I'll check what's on Sughosh latest patches and fix it if there are any differences. Looking at it again EFI_BAD_BUFFER_SIZE can change to indicate out of memory properly anyway. > + > + return Status; > +} [...] > +STATIC > +EFI_STATUS > +OpTeeRpmbFvbRead ( > + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, > + IN EFI_LBA Lba, > + IN UINTN Offset, > + IN OUT UINTN *NumBytes, > + IN OUT UINT8 *Buffer > + ) > +{ > + EFI_STATUS Status = EFI_SUCCESS; > + MEM_INSTANCE *Instance; > + VOID *Base; > + > + Instance = INSTANCE_FROM_FVB_THIS(This); > + if (Instance->Initialized == FALSE) { > [SAMI] if (!Instance->Initialized) ? > See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false > [/SAMI] Ok > + Instance->Initialize (Instance); > + } > + > + Base = (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize + Offset; [...] > + MEM_INSTANCE *Instance; > + EFI_STATUS Status = EFI_SUCCESS; > + VOID *Base; > + > + Instance = INSTANCE_FROM_FVB_THIS(This); > + if (Instance->Initialized == FALSE) { > [SAMI] if (!Instance->Initialized) ? > See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false > [/SAMI] sure > + Instance->Initialize (Instance); > + } > + Base = (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize + Offset; > + Status = ReadWriteRpmb (SP_SVC_RPMB_WRITE, (UINTN) Buffer, *NumBytes, > + Lba * Instance->BlockSize + Offset); > + if (Status != EFI_SUCCESS) { > [SAMI] if (EFI_ERROR (Status)) ? > [/SAMI] Ok > + return Status; > + } [...] > + if (!Buf) { > + return EFI_DEVICE_ERROR; > + } > + SetMem64 (Buf, NumLba * Instance->BlockSize, ~0UL); > + // Write the device > + Status = ReadWriteRpmb (SP_SVC_RPMB_WRITE, (UINTN) Buf, NumBytes, > + Start * Instance->BlockSize); > + if (Status != EFI_SUCCESS) { > [SAMI] if (EFI_ERROR (Status)) ? > [/SAMI] > + return Status; Ok > + } > + // Update the in memory copy > + SetMem64 (Base, NumLba * Instance->BlockSize, ~0UL); > + FreePool (Buf); > + } > + > + VA_END (Args); > + > + return EFI_SUCCESS; > +} > + > +/** > + Since we use a memory backed storage we need to restore the RPMB contents > + into memory before we register the Fvb protocol. > + > + @param Instace Address to copy flash contents to > + > + @retval 0 on success, OP-TEE error on failure > +**/ > +STATIC > +VOID > +ReadEntireFlash ( > + MEM_INSTANCE *Instance > + ) > +{ > + UINTN ReadAddr; > + > + UINTN StorageFtwWorkingSize = PcdGet32(PcdFlashNvStorageFtwWorkingSize); > + UINTN StorageVariableSize = PcdGet32(PcdFlashNvStorageVariableSize); > + UINTN StorageFtwSpareSize = PcdGet32(PcdFlashNvStorageFtwSpareSize); > + > + ReadAddr = Instance->MemBaseAddress; > + // There's no need to check if the read failed here. The upper EDK2 layers > + // will initialize the flash correctly if the in-memory copy is wrong > + ReadWriteRpmb(SP_SVC_RPMB_READ, ReadAddr, StorageVariableSize + > + StorageFtwWorkingSize + StorageFtwSpareSize , 0); > +} > + > + > +STATIC > +EFI_STATUS > +EFIAPI > +ValidateFvHeader ( > + IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader > + ) > +{ > + UINT16 Checksum; > + VARIABLE_STORE_HEADER *VariableStoreHeader; > + UINTN VariableStoreLength; > + UINTN FvLength; > + > + FvLength = PcdGet32(PcdFlashNvStorageVariableSize) + > + PcdGet32(PcdFlashNvStorageFtwWorkingSize) + > + PcdGet32(PcdFlashNvStorageFtwSpareSize); > + > + // Verify the header revision, header signature, length > + // Length of FvBlock cannot be 2**64-1 > + // HeaderLength cannot be an odd number > + // > + if ( (FwVolHeader->Revision != EFI_FVH_REVISION) > + || (FwVolHeader->Signature != EFI_FVH_SIGNATURE) > + || (FwVolHeader->FvLength != FvLength) > + ) > + { > + DEBUG ((DEBUG_INFO, "%a: No Firmware Volume header present\n", > + __FUNCTION__)); > + return EFI_NOT_FOUND; > + } > + > + // Check the Firmware Volume Guid > + if (!CompareGuid (&FwVolHeader->FileSystemGuid, &gEfiSystemNvDataFvGuid)) { > [SAMI] Not a comment for this patch but I noticed that BaseTools has an implementation of CompareGuid() that returns INTN. > I wonder if that is intentional. Moreover, it also appears that the CompareGuid() usage in BaseTools does not consistently follow the explicit comparison rule (https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false). > [/SAMI] > + DEBUG ((DEBUG_INFO, "%a: Firmware Volume Guid non-compatible\n", > + __FUNCTION__)); > + return EFI_NOT_FOUND; > + } > + > + // Verify the header checksum > + Checksum = CalculateSum16((UINT16*)FwVolHeader, FwVolHeader->HeaderLength); > + if (Checksum != 0) { > + DEBUG ((DEBUG_INFO, "%a: FV checksum is invalid (Checksum:0x%X)\n", > + __FUNCTION__, Checksum)); > + return EFI_NOT_FOUND; > [SAMI] Minor: By this point we should be fairly certain that this is a Firmware Volume header. > So, should the error code returned here be EFI_VOLUME_CORRUPTED? Would the same apply to the remaining checks below? > [/SAMI] I think that makes sense, but I'll have to check if there's a difference in behavior of the upper layers depending on the return value. The relevant Volume Header checks is a c/p from ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c and I kept it for consistency. Should we change the other driver as well at some point? [SAMI] Looking at the NorFlashDxe code, there may be more needed. But I think it will be a good improvement. [/SAMI] > + } > + > + VariableStoreHeader = (VOID *)((UINTN)FwVolHeader + > [SAMI] Should the typecast be (VARIABLE_STORE_HEADER*) instead of (VOID *)? > [/SAMI] Yes > + FwVolHeader->HeaderLength); > + > + // Check the Variable Store Guid [...] > + FirmwareVolumeHeader->Revision = EFI_FVH_REVISION; > + FirmwareVolumeHeader->BlockMap[0].NumBlocks = Instance->NBlocks; > + FirmwareVolumeHeader->BlockMap[0].Length = Instance->BlockSize; > + FirmwareVolumeHeader->BlockMap[1].NumBlocks = 0; > + FirmwareVolumeHeader->BlockMap[1].Length = 0; > + FirmwareVolumeHeader->Checksum = CalculateCheckSum16 ( > + (UINT16*)FirmwareVolumeHeader, > + FirmwareVolumeHeader->HeaderLength); > + > + // > + // VARIABLE_STORE_HEADER > + // > + VariableStoreHeader = (VOID *)((UINTN)Headers + > [SAMI] Should the typecase be (VARIABLE_STORE_HEADER*) instead of (VOID *)? > [/SAMI] Yes > + FirmwareVolumeHeader->HeaderLength); > + CopyGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid); > + VariableStoreHeader->Size = PcdGet32(PcdFlashNvStorageVariableSize) - > + FirmwareVolumeHeader->HeaderLength; > + VariableStoreHeader->Format = VARIABLE_STORE_FORMATTED; > + VariableStoreHeader->State = VARIABLE_STORE_HEALTHY; > + > + Status = ReadWriteRpmb(SP_SVC_RPMB_WRITE, (UINTN) Headers, HeadersLength, 0); > + if (Status != EFI_SUCCESS) { > [SAMI] if (EFI_ERROR (Status)) ? > [/SAMI] Ok > + goto Exit; > + } > + // Install the combined header in memory > + CopyMem ((VOID*) Instance->MemBaseAddress, Headers, HeadersLength); > + > +Exit: > + FreePool (Headers); > + return Status; > +} > + > +STATIC > +EFI_STATUS > +EFIAPI > +FvbInitialize ( > + MEM_INSTANCE *Instance > + ) > +{ > + EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader; > + EFI_STATUS Status; > + > + if (Instance->Initialized == TRUE) { > [SAMI] if (Instance->Initialized) ? > See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false > [/SAMI] Sure > + return EFI_SUCCESS; > + } > + > + // FirmwareVolumeHeader->FvLength is declared to have the Variable area > + // AND the FTW working area AND the FTW Spare contiguous. > + ASSERT (PcdGet32 (PcdFlashNvStorageVariableBase) + > + PcdGet32 (PcdFlashNvStorageVariableSize) == > + PcdGet32 (PcdFlashNvStorageFtwWorkingBase)); > + ASSERT (PcdGet32 (PcdFlashNvStorageFtwWorkingBase) + > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize) == > + PcdGet32 (PcdFlashNvStorageFtwSpareBase)); > + > + // Check if the size of the area is at least one block size > + ASSERT ((PcdGet32 (PcdFlashNvStorageVariableSize) > 0) && > + (PcdGet32 (PcdFlashNvStorageVariableSize) / Instance->BlockSize > 0)); > + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > 0) && > + (PcdGet32 (PcdFlashNvStorageFtwWorkingSize) / Instance->BlockSize > 0)); > + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwSpareSize) > 0) && > + (PcdGet32 (PcdFlashNvStorageFtwSpareSize) / Instance->BlockSize > 0)); > + > + // Ensure the Variable areas are aligned on block size boundaries > + ASSERT ((PcdGet32 (PcdFlashNvStorageVariableBase) % Instance->BlockSize) == 0); > + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwWorkingBase) % Instance->BlockSize) == 0); > + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwSpareBase) % Instance->BlockSize) == 0); > [SAMI] These asserts may need to be adjusted if 64-bit versions of the PCDs are used. > [/SAMI] Noted > + > + // Read the file from disk and copy it to memory > + ReadEntireFlash (Instance); > + > + FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *) Instance->MemBaseAddress; > + Status = ValidateFvHeader(FwVolHeader); > + if (EFI_ERROR (Status)) { > + // There is no valid header, so time to install one. > + DEBUG ((DEBUG_INFO, "%a: The FVB Header is not valid.\n", __FUNCTION__)); > + > + // Reset memory > + SetMem64 ((VOID *)Instance->MemBaseAddress, Instance->NBlocks * Instance->BlockSize, ~0UL); > + DEBUG ((DEBUG_INFO, "%a: Erasing Flash.\n", __FUNCTION__)); > + Status = ReadWriteRpmb(SP_SVC_RPMB_WRITE, Instance->MemBaseAddress, > + PcdGet32(PcdFlashNvStorageVariableSize) + > + PcdGet32(PcdFlashNvStorageFtwWorkingSize) + > + PcdGet32(PcdFlashNvStorageFtwSpareSize), 0); > + if (Status != EFI_SUCCESS) { > [SAMI] if (EFI_ERROR (Status)) ? > [/SAMI] Sure > + return Status; > + } > + // Install all appropriate headers > + DEBUG ((DEBUG_INFO, "%a: Installing a correct one for this volume.\n", > + __FUNCTION__)); > + Status = InitializeFvAndVariableStoreHeaders (Instance); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + } else { > + DEBUG ((DEBUG_INFO, "%a: Found valid FVB Header.\n", __FUNCTION__)); > + } > + Instance->Initialized = TRUE; > + > + return EFI_SUCCESS; > [SAMI] return Status; ? > [/SAMI] > +} Sure > + > +EFI_STATUS > +EFIAPI > +OpTeeRpmbFvbInit ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_MM_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + VOID *Addr; > + UINTN FvLength; > + UINTN NBlocks; > + > + FvLength = PcdGet32(PcdFlashNvStorageVariableSize) + > + PcdGet32(PcdFlashNvStorageFtwWorkingSize) + > + PcdGet32(PcdFlashNvStorageFtwSpareSize); > + > + NBlocks = EFI_SIZE_TO_PAGES(ALIGN_VARIABLE(FvLength)); > + Addr = AllocatePages(NBlocks); > + ASSERT (Addr != NULL); > [SAMI] Asserts will vanish in release builds and a failure to allocate memory would result in incorrect results. > Please handle this error and return EFI_OUT_OF_RESOURCES. > [/SAMI] Fair enough > + > + SetMem (&mInstance, sizeof (mInstance), 0); > + > + mInstance.FvbProtocol.GetPhysicalAddress = OpTeeRpmbFvbGetPhysicalAddress; > + mInstance.FvbProtocol.GetAttributes = OpTeeRpmbFvbGetAttributes; > + mInstance.FvbProtocol.SetAttributes = OpTeeRpmbFvbSetAttributes; > + mInstance.FvbProtocol.GetBlockSize = OpTeeRpmbFvbGetBlockSize; > + mInstance.FvbProtocol.EraseBlocks = OpTeeRpmbFvbErase; > + mInstance.FvbProtocol.Write = OpTeeRpmbFvbWrite; > + mInstance.FvbProtocol.Read = OpTeeRpmbFvbRead; > + > + mInstance.MemBaseAddress = (EFI_PHYSICAL_ADDRESS) Addr; > + mInstance.Signature = FLASH_SIGNATURE; > + mInstance.Initialize = FvbInitialize; > + mInstance.BlockSize = EFI_PAGE_SIZE; > + mInstance.NBlocks = NBlocks; > + > + // Update the defined PCDs related to Variable Storage > + PatchPcdSet32 (PcdFlashNvStorageVariableBase, mInstance.MemBaseAddress); > [SAMI] Should the 64-bit versions of the PCDs be used here. > A recent change added similar support to the ArmPlatformPkg\Drivers\NorFlashDxe. > [/SAMI] Noted > + PatchPcdSet32 (PcdFlashNvStorageFtwWorkingBase, mInstance.MemBaseAddress + > + PcdGet32 (PcdFlashNvStorageVariableSize)); > + PatchPcdSet32 (PcdFlashNvStorageFtwSpareBase, mInstance.MemBaseAddress + > + PcdGet32 (PcdFlashNvStorageVariableSize) + > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)); > + > + Status = gMmst->MmInstallProtocolInterface ( > + &mInstance.Handle, > + &gEfiSmmFirmwareVolumeBlockProtocolGuid, > + EFI_NATIVE_INTERFACE, > + &mInstance.FvbProtocol > + ); > + ASSERT_EFI_ERROR (Status); > + > + DEBUG ((DEBUG_INFO, "%a: Register OP-TEE RPMB Fvb\n", __FUNCTION__)); > + DEBUG ((DEBUG_INFO, "%a: Using NV store FV in-memory copy at 0x%lx\n", > + __FUNCTION__, PatchPcdGet32 (PcdFlashNvStorageVariableBase))); > + > + return Status; > +} > -- > 2.17.1 > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver 2021-01-29 8:02 ` Ilias Apalodimas 2021-01-29 11:45 ` Sami Mujawar @ 2021-02-01 14:00 ` Ilias Apalodimas 2021-02-02 10:40 ` Sami Mujawar 1 sibling, 1 reply; 14+ messages in thread From: Ilias Apalodimas @ 2021-02-01 14:00 UTC (permalink / raw) To: Sami Mujawar Cc: Sughosh Ganu, devel@edk2.groups.io, Ard Biesheuvel, Leif Lindholm, Sahil Malhotra Hi Sami, [...] > > +STATIC > > +EFI_STATUS > > +ReadWriteRpmb ( > > + UINTN SvcAct, > > + UINTN Addr, > > + UINTN NumBytes, > > + UINTN Offset > > + ) > > +{ > > + ARM_SVC_ARGS SvcArgs; > > + EFI_STATUS Status; > > + > > + ZeroMem (&SvcArgs, sizeof (SvcArgs)); > > + > > + SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64; > > + SvcArgs.Arg1 = storage_id; > > + SvcArgs.Arg2 = 0; > > + SvcArgs.Arg3 = SvcAct; > > + SvcArgs.Arg4 = Addr; > > + SvcArgs.Arg5 = NumBytes; > > + SvcArgs.Arg6 = Offset; > > + > > + ArmCallSvc (&SvcArgs); > > + if (SvcArgs.Arg3) { > > + DEBUG ((DEBUG_ERROR, "%a: Svc Call 0x%08x Addr: 0x%08x len: 0x%x Offset: 0x%x failed with 0x%x\n", > > + __func__, SvcAct, Addr, NumBytes, Offset, SvcArgs.Arg3)); > > + } > > + > > + switch (SvcArgs.Arg3) { > > + case ARM_SVC_SPM_RET_SUCCESS: > > + Status = EFI_SUCCESS; > > + break; > > + > > + case ARM_SVC_SPM_RET_NOT_SUPPORTED: > > + Status = EFI_UNSUPPORTED; > > + break; > > + > > + case ARM_SVC_SPM_RET_INVALID_PARAMS: > > + Status = EFI_INVALID_PARAMETER; > > + break; > > + > > + case ARM_SVC_SPM_RET_DENIED: > > + Status = EFI_ACCESS_DENIED; > > + break; > > + > > + case ARM_SVC_SPM_RET_NO_MEMORY: > > + Status = EFI_BAD_BUFFER_SIZE; > > + break; > > + > > + default: > > + Status = EFI_ACCESS_DENIED; > > + } > > [SAMI] Should the error handling here be updated similar to the FF-A StandaloneMmPkg patches? > > [/SAMI] > > I actually picked up the error handling from the previous non-FFA code. > I'll check what's on Sughosh latest patches and fix it if there are > any differences. > Looking at it again EFI_BAD_BUFFER_SIZE can change to indicate out of > memory properly anyway. > Had another look at this. This seems fine if I just change EFI_BAD_BUFFER_SIZE -> EFI OUT_OF_RESOURCES because OP-TEE is only using these errors from FFA. Eventually the OP-TEE code that launches StMM today, will move to FFA and become a separate SP, so that will naturally be handled once that's done. I don't see a point of adding unused error cases. Regards /Ilias ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver 2021-02-01 14:00 ` Ilias Apalodimas @ 2021-02-02 10:40 ` Sami Mujawar 2021-02-02 12:33 ` Ilias Apalodimas 0 siblings, 1 reply; 14+ messages in thread From: Sami Mujawar @ 2021-02-02 10:40 UTC (permalink / raw) To: Ilias Apalodimas Cc: Sughosh Ganu, devel@edk2.groups.io, Ard Biesheuvel, Leif Lindholm, Sahil Malhotra, nd Hi Ilias, Please see my response inline marked [SAMI]. Regards, Sami Mujawar -----Original Message----- From: Ilias Apalodimas <ilias.apalodimas@linaro.org> Sent: 01 February 2021 02:01 PM To: Sami Mujawar <Sami.Mujawar@arm.com> Cc: Sughosh Ganu <sughosh.ganu@linaro.org>; devel@edk2.groups.io; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Leif Lindholm <leif@nuviainc.com>; Sahil Malhotra <sahil.malhotra@linaro.org> Subject: Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver Hi Sami, [...] > > +STATIC > > +EFI_STATUS > > +ReadWriteRpmb ( > > + UINTN SvcAct, > > + UINTN Addr, > > + UINTN NumBytes, > > + UINTN Offset > > + ) > > +{ > > + ARM_SVC_ARGS SvcArgs; > > + EFI_STATUS Status; > > + > > + ZeroMem (&SvcArgs, sizeof (SvcArgs)); > > + > > + SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64; > > + SvcArgs.Arg1 = storage_id; > > + SvcArgs.Arg2 = 0; > > + SvcArgs.Arg3 = SvcAct; > > + SvcArgs.Arg4 = Addr; > > + SvcArgs.Arg5 = NumBytes; > > + SvcArgs.Arg6 = Offset; > > + > > + ArmCallSvc (&SvcArgs); > > + if (SvcArgs.Arg3) { > > + DEBUG ((DEBUG_ERROR, "%a: Svc Call 0x%08x Addr: 0x%08x len: 0x%x Offset: 0x%x failed with 0x%x\n", > > + __func__, SvcAct, Addr, NumBytes, Offset, SvcArgs.Arg3)); > > + } > > + > > + switch (SvcArgs.Arg3) { > > + case ARM_SVC_SPM_RET_SUCCESS: > > + Status = EFI_SUCCESS; > > + break; > > + > > + case ARM_SVC_SPM_RET_NOT_SUPPORTED: > > + Status = EFI_UNSUPPORTED; > > + break; > > + > > + case ARM_SVC_SPM_RET_INVALID_PARAMS: > > + Status = EFI_INVALID_PARAMETER; > > + break; > > + > > + case ARM_SVC_SPM_RET_DENIED: > > + Status = EFI_ACCESS_DENIED; > > + break; > > + > > + case ARM_SVC_SPM_RET_NO_MEMORY: > > + Status = EFI_BAD_BUFFER_SIZE; > > + break; > > + > > + default: > > + Status = EFI_ACCESS_DENIED; > > + } > > [SAMI] Should the error handling here be updated similar to the FF-A StandaloneMmPkg patches? > > [/SAMI] > > I actually picked up the error handling from the previous non-FFA code. > I'll check what's on Sughosh latest patches and fix it if there are > any differences. > Looking at it again EFI_BAD_BUFFER_SIZE can change to indicate out of > memory properly anyway. > Had another look at this. This seems fine if I just change EFI_BAD_BUFFER_SIZE -> EFI OUT_OF_RESOURCES because OP-TEE is only using these errors from FFA. Eventually the OP-TEE code that launches StMM today, will move to FFA and become a separate SP, so that will naturally be handled once that's done. I don't see a point of adding unused error cases. [SAMI] Referring to the FFA specification, DEN0077A, v1.0, section 10.2 FFA_MSG_SEND_DIRECT_REQ and Table 10.8: FFA_ERROR encoding, I think the error codes being handled above would be returned in SvcArgs.Arg2. The message flow would be as follows: - Caller sends FFA_MSG_SEND_DIRECT_REQ to the target endpoint. - if the message does not reach the target endpoint, an error code from Table 10.8 may be returned in w2 (i.e. SvcArgs.Arg2) - If the message reaches the target endpoint, then callee shall invoke one of the following interfaces: * FFA_MSG_SEND_DIRECT_RESP * FFA_INTERRUPT * FFA_SUCCESS This would mean that if the callee responds with FFA_MSG_SEND_DIRECT_RESP, the callee returned error/status code shall be in w/x3-w/x7 (which I think in this case may be in SvcArgs.Arg3). [/SAMI] Regards /Ilias ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver 2021-02-02 10:40 ` Sami Mujawar @ 2021-02-02 12:33 ` Ilias Apalodimas 2021-02-02 14:49 ` Ilias Apalodimas 0 siblings, 1 reply; 14+ messages in thread From: Ilias Apalodimas @ 2021-02-02 12:33 UTC (permalink / raw) To: Sami Mujawar Cc: Sughosh Ganu, devel@edk2.groups.io, Ard Biesheuvel, Leif Lindholm, Sahil Malhotra, nd Hi Sami, On Tue, Feb 02, 2021 at 10:40:32AM +0000, Sami Mujawar wrote: > Hi Ilias, > > Please see my response inline marked [SAMI]. > > Regards, > > Sami Mujawar > > -----Original Message----- > From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > Sent: 01 February 2021 02:01 PM > To: Sami Mujawar <Sami.Mujawar@arm.com> > Cc: Sughosh Ganu <sughosh.ganu@linaro.org>; devel@edk2.groups.io; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Leif Lindholm <leif@nuviainc.com>; Sahil Malhotra <sahil.malhotra@linaro.org> > Subject: Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver > > Hi Sami, > > > [...] > > > +STATIC > > > +EFI_STATUS > > > +ReadWriteRpmb ( > > > + UINTN SvcAct, > > > + UINTN Addr, > > > + UINTN NumBytes, > > > + UINTN Offset > > > + ) > > > +{ > > > + ARM_SVC_ARGS SvcArgs; > > > + EFI_STATUS Status; > > > + > > > + ZeroMem (&SvcArgs, sizeof (SvcArgs)); > > > + > > > + SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64; > > > + SvcArgs.Arg1 = storage_id; > > > + SvcArgs.Arg2 = 0; > > > + SvcArgs.Arg3 = SvcAct; > > > + SvcArgs.Arg4 = Addr; > > > + SvcArgs.Arg5 = NumBytes; > > > + SvcArgs.Arg6 = Offset; > > > + > > > + ArmCallSvc (&SvcArgs); > > > + if (SvcArgs.Arg3) { > > > + DEBUG ((DEBUG_ERROR, "%a: Svc Call 0x%08x Addr: 0x%08x len: 0x%x Offset: 0x%x failed with 0x%x\n", > > > + __func__, SvcAct, Addr, NumBytes, Offset, SvcArgs.Arg3)); > > > + } > > > + > > > + switch (SvcArgs.Arg3) { > > > + case ARM_SVC_SPM_RET_SUCCESS: > > > + Status = EFI_SUCCESS; > > > + break; > > > + > > > + case ARM_SVC_SPM_RET_NOT_SUPPORTED: > > > + Status = EFI_UNSUPPORTED; > > > + break; > > > + > > > + case ARM_SVC_SPM_RET_INVALID_PARAMS: > > > + Status = EFI_INVALID_PARAMETER; > > > + break; > > > + > > > + case ARM_SVC_SPM_RET_DENIED: > > > + Status = EFI_ACCESS_DENIED; > > > + break; > > > + > > > + case ARM_SVC_SPM_RET_NO_MEMORY: > > > + Status = EFI_BAD_BUFFER_SIZE; > > > + break; > > > + > > > + default: > > > + Status = EFI_ACCESS_DENIED; > > > + } > > > [SAMI] Should the error handling here be updated similar to the FF-A StandaloneMmPkg patches? > > > [/SAMI] > > > > I actually picked up the error handling from the previous non-FFA code. > > I'll check what's on Sughosh latest patches and fix it if there are > > any differences. > > Looking at it again EFI_BAD_BUFFER_SIZE can change to indicate out of > > memory properly anyway. > > > > Had another look at this. This seems fine if I just change > EFI_BAD_BUFFER_SIZE -> EFI OUT_OF_RESOURCES because OP-TEE is only > using these errors from FFA. Eventually the OP-TEE code that launches > StMM today, will move to FFA and become a separate SP, so that will > naturally be handled once that's done. I don't see a point of adding > unused error cases. > > [SAMI] Referring to the FFA specification, DEN0077A, v1.0, section 10.2 FFA_MSG_SEND_DIRECT_REQ and Table 10.8: FFA_ERROR encoding, I think the > error codes being handled above would be returned in SvcArgs.Arg2. Hmm why ? > The message flow would be as follows: > - Caller sends FFA_MSG_SEND_DIRECT_REQ to the target endpoint. > - if the message does not reach the target endpoint, an error code from Table 10.8 may be returned in w2 (i.e. SvcArgs.Arg2) That would be in the case you have a working TF-A implementation and the message is never dispatched to the endpoint right? The current driver is not implementing the whole range of that spec. The communication between secure/non secure world is still based on the OP-TEE messaging mechanism. The only part that complies to the FFA spec is the communication between the driver itself and OP-TEE. > - If the message reaches the target endpoint, then callee shall invoke one of the following interfaces: > * FFA_MSG_SEND_DIRECT_RESP So what's happening here, is that we send an SVC with ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64. The op-tee relevant code is located at ./core/arch/arm/kernel/stmm_sp.c There's 2 things we handle right now on OP-TEE: 1. set the page permissions, after relocating the executable. 2. Read/Write data on our RPMB. In both cases service_compose_direct_resp() is used to construct the response and that set the return value on x3. Regards /Ilias > * FFA_INTERRUPT > * FFA_SUCCESS > This would mean that if the callee responds with FFA_MSG_SEND_DIRECT_RESP, the callee returned error/status code shall be in w/x3-w/x7 (which I think in this case may be in SvcArgs.Arg3). > [/SAMI] > > Regards > /Ilias ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver 2021-02-02 12:33 ` Ilias Apalodimas @ 2021-02-02 14:49 ` Ilias Apalodimas 2021-02-02 15:13 ` Sami Mujawar 0 siblings, 1 reply; 14+ messages in thread From: Ilias Apalodimas @ 2021-02-02 14:49 UTC (permalink / raw) To: Sami Mujawar Cc: Sughosh Ganu, devel@edk2.groups.io, Ard Biesheuvel, Leif Lindholm, Sahil Malhotra, nd Hi Sami, Inlining some additional info on my explanation. > > > [...] > > > I actually picked up the error handling from the previous non-FFA code. > > > I'll check what's on Sughosh latest patches and fix it if there are > > > any differences. > > > Looking at it again EFI_BAD_BUFFER_SIZE can change to indicate out of > > > memory properly anyway. > > > > > > > Had another look at this. This seems fine if I just change > > EFI_BAD_BUFFER_SIZE -> EFI OUT_OF_RESOURCES because OP-TEE is only > > using these errors from FFA. Eventually the OP-TEE code that launches > > StMM today, will move to FFA and become a separate SP, so that will > > naturally be handled once that's done. I don't see a point of adding > > unused error cases. > > > > [SAMI] Referring to the FFA specification, DEN0077A, v1.0, section 10.2 FFA_MSG_SEND_DIRECT_REQ and Table 10.8: FFA_ERROR encoding, I think the > > error codes being handled above would be returned in SvcArgs.Arg2. > > Hmm why ? > > > The message flow would be as follows: > > - Caller sends FFA_MSG_SEND_DIRECT_REQ to the target endpoint. > > - if the message does not reach the target endpoint, an error code from Table 10.8 may be returned in w2 (i.e. SvcArgs.Arg2) > > That would be in the case you have a working TF-A implementation and the > message is never dispatched to the endpoint right? > > The current driver is not implementing the whole range of that spec. The > communication between secure/non secure world is still based on the OP-TEE > messaging mechanism. > The only part that complies to the FFA spec is the communication between the > driver itself and OP-TEE. > > - If the message reaches the target endpoint, then callee shall invoke one of the following interfaces: > > * FFA_MSG_SEND_DIRECT_RESP > > So what's happening here, is that we send an SVC with ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64. > The op-tee relevant code is located at ./core/arch/arm/kernel/stmm_sp.c > There's 2 things we handle right now on OP-TEE: > 1. set the page permissions, after relocating the executable. > 2. Read/Write data on our RPMB. > > In both cases service_compose_direct_resp() is used to construct the response > and that set the return value on x3. What you mention and looking for is the discovery mechanism that FFA implements. This is not part of the patches (yet) and that's the reason the driver hardcodes mMemMgrId = 3U and mStorageId = 4U. In OP-TEE side the only reason we have to fill in x2 with the error code is if the request that comes in doesn't match any of these values. But that's never the case from this driver yet, since there's no SP discovery mechanism implemented to begin with. Hope that clears it up now. Regards /Ilias > > > Regards > /Ilias > > > * FFA_INTERRUPT > > * FFA_SUCCESS > > This would mean that if the callee responds with FFA_MSG_SEND_DIRECT_RESP, the callee returned error/status code shall be in w/x3-w/x7 (which I think in this case may be in SvcArgs.Arg3). > > [/SAMI] > > > > Regards > > /Ilias ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver 2021-02-02 14:49 ` Ilias Apalodimas @ 2021-02-02 15:13 ` Sami Mujawar 2021-02-02 16:27 ` Ilias Apalodimas 0 siblings, 1 reply; 14+ messages in thread From: Sami Mujawar @ 2021-02-02 15:13 UTC (permalink / raw) To: Ilias Apalodimas Cc: Sughosh Ganu, devel@edk2.groups.io, Ard Biesheuvel, Leif Lindholm, Sahil Malhotra, nd Hi Ilias, Please see my response inline marked [SAMI]. Regards, Sami Mujawar -----Original Message----- From: Ilias Apalodimas <ilias.apalodimas@linaro.org> Sent: 02 February 2021 02:50 PM To: Sami Mujawar <Sami.Mujawar@arm.com> Cc: Sughosh Ganu <sughosh.ganu@linaro.org>; devel@edk2.groups.io; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Leif Lindholm <leif@nuviainc.com>; Sahil Malhotra <sahil.malhotra@linaro.org>; nd <nd@arm.com> Subject: Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver Hi Sami, Inlining some additional info on my explanation. > > > [...] > > > I actually picked up the error handling from the previous non-FFA code. > > > I'll check what's on Sughosh latest patches and fix it if there are > > > any differences. > > > Looking at it again EFI_BAD_BUFFER_SIZE can change to indicate out of > > > memory properly anyway. > > > > > > > Had another look at this. This seems fine if I just change > > EFI_BAD_BUFFER_SIZE -> EFI OUT_OF_RESOURCES because OP-TEE is only > > using these errors from FFA. Eventually the OP-TEE code that launches > > StMM today, will move to FFA and become a separate SP, so that will > > naturally be handled once that's done. I don't see a point of adding > > unused error cases. > > > > [SAMI] Referring to the FFA specification, DEN0077A, v1.0, section 10.2 FFA_MSG_SEND_DIRECT_REQ and Table 10.8: FFA_ERROR encoding, I think the > > error codes being handled above would be returned in SvcArgs.Arg2. > > Hmm why ? [SAMI] This is for the case where the FFA_MSG_SEND_DIRECT_REQ does not reach the target endpoint. [/SAMI] > > > The message flow would be as follows: > > - Caller sends FFA_MSG_SEND_DIRECT_REQ to the target endpoint. > > - if the message does not reach the target endpoint, an error code from Table 10.8 may be returned in w2 (i.e. SvcArgs.Arg2) > > That would be in the case you have a working TF-A implementation and the > message is never dispatched to the endpoint right? [SAMI] Yes [/SAMI] > > The current driver is not implementing the whole range of that spec. The > communication between secure/non secure world is still based on the OP-TEE > messaging mechanism. > The only part that complies to the FFA spec is the communication between the > driver itself and OP-TEE. > > - If the message reaches the target endpoint, then callee shall invoke one of the following interfaces: > > * FFA_MSG_SEND_DIRECT_RESP > > So what's happening here, is that we send an SVC with ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64. > The op-tee relevant code is located at ./core/arch/arm/kernel/stmm_sp.c > There's 2 things we handle right now on OP-TEE: > 1. set the page permissions, after relocating the executable. > 2. Read/Write data on our RPMB. > > In both cases service_compose_direct_resp() is used to construct the response > and that set the return value on x3. What you mention and looking for is the discovery mechanism that FFA implements. This is not part of the patches (yet) and that's the reason the driver hardcodes mMemMgrId = 3U and mStorageId = 4U. In OP-TEE side the only reason we have to fill in x2 with the error code is if the request that comes in doesn't match any of these values. But that's never the case from this driver yet, since there's no SP discovery mechanism implemented to begin with. Hope that clears it up now. [SAMI] Thank you for the explanation. It makes sense. I think a comment describing this rationale would be helpful for someone trying to understand the code. [/SAMI] Regards /Ilias > > > Regards > /Ilias > > > * FFA_INTERRUPT > > * FFA_SUCCESS > > This would mean that if the callee responds with FFA_MSG_SEND_DIRECT_RESP, the callee returned error/status code shall be in w/x3-w/x7 (which I think in this case may be in SvcArgs.Arg3). > > [/SAMI] > > > > Regards > > /Ilias ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver 2021-02-02 15:13 ` Sami Mujawar @ 2021-02-02 16:27 ` Ilias Apalodimas 0 siblings, 0 replies; 14+ messages in thread From: Ilias Apalodimas @ 2021-02-02 16:27 UTC (permalink / raw) To: Sami Mujawar Cc: Sughosh Ganu, devel@edk2.groups.io, Ard Biesheuvel, Leif Lindholm, Sahil Malhotra, nd Hi Sami, On Tue, 2 Feb 2021 at 17:13, Sami Mujawar <Sami.Mujawar@arm.com> wrote: > > Hi Ilias, > > Please see my response inline marked [SAMI]. > > Regards, > > Sami Mujawar > > -----Original Message----- > From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > Sent: 02 February 2021 02:50 PM > To: Sami Mujawar <Sami.Mujawar@arm.com> > Cc: Sughosh Ganu <sughosh.ganu@linaro.org>; devel@edk2.groups.io; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Leif Lindholm <leif@nuviainc.com>; Sahil Malhotra <sahil.malhotra@linaro.org>; nd <nd@arm.com> > Subject: Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver > > Hi Sami, > > Inlining some additional info on my explanation. > > > > > > [...] > > > > I actually picked up the error handling from the previous non-FFA code. > > > > I'll check what's on Sughosh latest patches and fix it if there are > > > > any differences. > > > > Looking at it again EFI_BAD_BUFFER_SIZE can change to indicate out of > > > > memory properly anyway. > > > > > > > > > > Had another look at this. This seems fine if I just change > > > EFI_BAD_BUFFER_SIZE -> EFI OUT_OF_RESOURCES because OP-TEE is only > > > using these errors from FFA. Eventually the OP-TEE code that launches > > > StMM today, will move to FFA and become a separate SP, so that will > > > naturally be handled once that's done. I don't see a point of adding > > > unused error cases. > > > > > > [SAMI] Referring to the FFA specification, DEN0077A, v1.0, section 10.2 FFA_MSG_SEND_DIRECT_REQ and Table 10.8: FFA_ERROR encoding, I think the > > > error codes being handled above would be returned in SvcArgs.Arg2. > > > > Hmm why ? > [SAMI] This is for the case where the FFA_MSG_SEND_DIRECT_REQ does not reach the target endpoint. > [/SAMI] > > > > > The message flow would be as follows: > > > - Caller sends FFA_MSG_SEND_DIRECT_REQ to the target endpoint. > > > - if the message does not reach the target endpoint, an error code from Table 10.8 may be returned in w2 (i.e. SvcArgs.Arg2) > > > > That would be in the case you have a working TF-A implementation and the > > message is never dispatched to the endpoint right? > [SAMI] Yes [/SAMI] > > > > The current driver is not implementing the whole range of that spec. The > > communication between secure/non secure world is still based on the OP-TEE > > messaging mechanism. > > The only part that complies to the FFA spec is the communication between the > > driver itself and OP-TEE. > > > - If the message reaches the target endpoint, then callee shall invoke one of the following interfaces: > > > * FFA_MSG_SEND_DIRECT_RESP > > > > So what's happening here, is that we send an SVC with ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64. > > The op-tee relevant code is located at ./core/arch/arm/kernel/stmm_sp.c > > There's 2 things we handle right now on OP-TEE: > > 1. set the page permissions, after relocating the executable. > > 2. Read/Write data on our RPMB. > > > > In both cases service_compose_direct_resp() is used to construct the response > > and that set the return value on x3. > > What you mention and looking for is the discovery mechanism that FFA > implements. This is not part of the patches (yet) and that's the reason the > driver hardcodes mMemMgrId = 3U and mStorageId = 4U. > In OP-TEE side the only reason we have to fill in x2 with the error code is if > the request that comes in doesn't match any of these values. But that's never > the case from this driver yet, since there's no SP discovery mechanism > implemented to begin with. > > Hope that clears it up now. > > [SAMI] Thank you for the explanation. It makes sense. I think a comment describing this rationale would be helpful for someone trying to understand the code. > [/SAMI] That's a good idea. I'll add it just before the hardcoded values for storage and memory IDs. We need to keep in mind that this is evolving work and some things will change once the majority of FFA has been implemented. The architecture will be similar, we'll just have to account for dynamic discovery and memory sharing between secure partitions. Unfortunately this has many moving pieces at the moment, so we might as well make the reading easier. > > Regards > /Ilias > > > > > > Regards > > /Ilias > > > > > * FFA_INTERRUPT > > > * FFA_SUCCESS > > > This would mean that if the callee responds with FFA_MSG_SEND_DIRECT_RESP, the callee returned error/status code shall be in w/x3-w/x7 (which I think in this case may be in SvcArgs.Arg3). > > > [/SAMI] > > > > > > Regards > > > /Ilias ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH edk2-platforms v3 2/2] StMMRpmb: Add support for building StandaloneMm image for OP-TEE 2020-12-16 11:09 [PATCH edk2-platforms v3 0/2] Add support for running StandaloneMm as OP-TEE TA Sughosh Ganu 2020-12-16 11:09 ` [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver Sughosh Ganu @ 2020-12-16 11:09 ` Sughosh Ganu 2021-01-29 10:29 ` Sami Mujawar 1 sibling, 1 reply; 14+ messages in thread From: Sughosh Ganu @ 2020-12-16 11:09 UTC (permalink / raw) To: devel Cc: Sami Mujawar, Ard Biesheuvel, Leif Lindholm, Sahil Malhotra, Ilias Apalodimas From: Ilias Apalodimas <ilias.apalodimas@linaro.org> With some recent changes in OP-TEE [1] and U-Boot [2] we can compile StMM and launch it from an OP-TEE secure partition which is mimicking SPM. There's a number of advantages in this approach. In Arm world SPM, currently used for dispatching StMM, and SPD used for OP-TEE, are mutually exclusive. Since there's no application in OP-TEE for managing EFI variables, this means that one can have a secure OS or secure variable storage. By re-using StMM we have EDK2s approved application controlling variable storage and the ability to run a secure world OS. This also allows various firmware implementations to adopt EDK2 way of storing variables (including the FTW implementation), as long as OP-TEE is available on that given platform (or any other secure OS that can launch StMM and has a supplicant for handling the RPMB partition). Another advantage is that OP-TEE has the ability to access an eMMC RPMB partition to store those variables. This requires a normal world supplicant, which is implemented in U-Boot currently. The supplicant picks up the encrypted buffer from OP-TEE and wires it to the eMMC driver(s). Similar functionality can be added in EDK2 by porting the supplicant and adapt it to using the native eMMC drivers. There's is one drawback in using OP-TEE. The current SPM calls need to run to completion. This contradicts the current OP-TEE RPC call requirements, used to access the RPMB storage. Thats leads to two different SMC calls for entering secure world to access StMM. So let's add support for a platform that compiles StMM and an RPMB driver that communicates with OP-TEE to read/write the variables. For anyone interested in testing this there's repo that builds all the sources and works on QEMU [3]. [1] https://github.com/OP-TEE/optee_os/pull/3973 [2] http://u-boot.10912.n7.nabble.com/PATCH-0-7-v4-EFI-variable-support-via-OP-TEE-td412499.html [3] https://git.linaro.org/people/ilias.apalodimas/efi_optee_variables.git/ Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- Changes since V2: None Platform/StMMRpmb/PlatformStandaloneMm.dsc | 168 ++++++++++++++++++++ Platform/StMMRpmb/PlatformStandaloneMm.fdf | 111 +++++++++++++ 2 files changed, 279 insertions(+) diff --git a/Platform/StMMRpmb/PlatformStandaloneMm.dsc b/Platform/StMMRpmb/PlatformStandaloneMm.dsc new file mode 100644 index 0000000000..93596c0630 --- /dev/null +++ b/Platform/StMMRpmb/PlatformStandaloneMm.dsc @@ -0,0 +1,168 @@ +# +# Copyright (c) 2018, ARM Limited. All rights reserved. +# Copyright (c) 2020, Linaro Ltd. All rights reserved. +# +# SPDX-License-Identifier: BSD-2-Clause-Patent +# + +################################################################################ +# +# Defines Section - statements that will be processed to create a Makefile. +# +################################################################################ +[Defines] + PLATFORM_NAME = MmStandaloneRpmb + PLATFORM_GUID = A27A486E-D7B9-4D70-9F37-FED9ABE041A2 + PLATFORM_VERSION = 1.0 + DSC_SPECIFICATION = 0x00010011 + OUTPUT_DIRECTORY = Build/$(PLATFORM_NAME) + SUPPORTED_ARCHITECTURES = AARCH64 + BUILD_TARGETS = DEBUG|RELEASE|NOOPT + SKUID_IDENTIFIER = DEFAULT + FLASH_DEFINITION = Platform/StMMRpmb/PlatformStandaloneMm.fdf + DEFINE DEBUG_MESSAGE = TRUE + + # LzmaF86 + DEFINE COMPRESSION_TOOL_GUID = D42AE6BD-1352-4bfb-909A-CA72A6EAE889 + +################################################################################ +# +# Library Class section - list of all Library Classes needed by this Platform. +# +################################################################################ +[LibraryClasses] + ArmSvcLib|ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf + ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf + BaseLib|MdePkg/Library/BaseLib/BaseLib.inf + SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf + VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf + BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf + DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf + DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf + ExtractGuidedSectionLib|EmbeddedPkg/Library/PrePiExtractGuidedSectionLib/PrePiExtractGuidedSectionLib.inf + FvLib|StandaloneMmPkg/Library/FvLib/FvLib.inf + HobLib|StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf + IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf + MemLib|StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf + MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmCoreMemoryAllocationLib/StandaloneMmCoreMemoryAllocationLib.inf + PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf + PeCoffLib|MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf + PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf + VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf + ReportStatusCodeLib|MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf + + # + # Entry point + # + #StandaloneMmCoreEntryPoint|StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf + StandaloneMmCoreEntryPoint|StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf + StandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntryPoint/StandaloneMmDriverEntryPoint.inf + + StandaloneMmMmuLib|ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf + #CacheMaintenanceLib|ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.inf + CacheMaintenanceLib|MdePkg/Library/BaseCacheMaintenanceLibNull/BaseCacheMaintenanceLibNull.inf + PeCoffExtraActionLib|StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf + RngLib|MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf + + SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf + DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf + + # + # It is not possible to prevent the ARM compiler for generic intrinsic functions. + # This library provides the intrinsic functions generate by a given compiler. + # NULL means link this library into all ARM images. + # + NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf + +[LibraryClasses.common.MM_STANDALONE] + HobLib|StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf + MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf + MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.inf + + IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf + OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf + PlatformSecureLib|SecurityPkg/Library/PlatformSecureLibNull/PlatformSecureLibNull.inf + SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf + TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf +################################################################################ +# +# Pcd Section - list of all EDK II PCD Entries defined by this Platform +# +################################################################################ + +[PcdsFeatureFlag.common] + gArmTokenSpaceGuid.PcdFfaEnable|TRUE + +[PcdsFixedAtBuild] + gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x800000CF + gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0xff + gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x0f + + gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x2 + # Secure Storage + gEfiSecurityPkgTokenSpaceGuid.PcdUserPhysicalPresence|TRUE + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800 + + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00004000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00004000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00004000 + gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x00004000 + +[PcdsPatchableInModule] + # Allocated memory for EDK2 uppers layers + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x0 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x0 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x0 + +################################################################################################### +# +# Components Section - list of the modules and components that will be processed by compilation +# tools and the EDK II tools to generate PE32/PE32+/Coff image files. +# +# Note: The EDK II DSC file is not used to specify how compiled binary images get placed +# into firmware volume images. This section is just a list of modules to compile from +# source into UEFI-compliant binaries. +# It is the FDF file that contains information on combining binary files into firmware +# volume images, whose concept is beyond UEFI and is described in PI specification. +# Binary modules do not need to be listed in this section, as they should be +# specified in the FDF file. For example: Shell binary (Shell_Full.efi), FAT binary (Fat.efi), +# Logo (Logo.bmp), and etc. +# There may also be modules listed in this section that are not required in the FDF file, +# When a module listed here is excluded from FDF file, then UEFI-compliant binary will be +# generated for it, but the binary will not be put into any firmware volume. +# +################################################################################################### +[Components.common] + # + # Standalone MM components + # + Drivers/OpTeeRpmb/OpTeeRpmbFv.inf + StandaloneMmPkg/Core/StandaloneMmCore.inf + StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf + MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf { + <LibraryClasses> + NULL|Drivers/OpTeeRpmb/FixupPcd.inf + } + MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf { + <LibraryClasses> + AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf + BaseCryptLib|CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf + DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf + VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf + NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf + NULL|Drivers/OpTeeRpmb/FixupPcd.inf + } + +################################################################################################### +# +# BuildOptions Section - Define the module specific tool chain flags that should be used as +# the default flags for a module. These flags are appended to any +# standard flags that are defined by the build process. They can be +# applied for any modules or only those modules with the specific +# module style (EDK or EDKII) specified in [Components] section. +# +################################################################################################### +[BuildOptions.AARCH64] +GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000 -march=armv8-a+nofp +GCC:*_*_*_CC_FLAGS = -mstrict-align diff --git a/Platform/StMMRpmb/PlatformStandaloneMm.fdf b/Platform/StMMRpmb/PlatformStandaloneMm.fdf new file mode 100644 index 0000000000..febc6d0d95 --- /dev/null +++ b/Platform/StMMRpmb/PlatformStandaloneMm.fdf @@ -0,0 +1,111 @@ +# +# Copyright (c) 2018, ARM Limited. All rights reserved. +# Copyright (c) 2020, Linaro Ltd. All rights reserved. +# +# SPDX-License-Identifier: BSD-2-Clause-Patent +# + +################################################################################ +# +# FD Section +# The [FD] Section is made up of the definition statements and a +# description of what goes into the Flash Device Image. Each FD section +# defines one flash "device" image. A flash device image may be one of +# the following: Removable media bootable image (like a boot floppy +# image,) an Option ROM image (that would be "flashed" into an add-in +# card,) a System "Flash" image (that would be burned into a system's +# flash) or an Update ("Capsule") image that will be used to update and +# existing system flash. +# +################################################################################ + +[FD.BL32_AP_MM] +BaseAddress = 0x1000 # any address apart from 0x0 +Size = 0x00300000 +ErasePolarity = 1 + +BlockSize = 0x00001000 +NumBlocks = 0x0300 + +################################################################################ +# +# Following are lists of FD Region layout which correspond to the locations of different +# images within the flash device. +# +# Regions must be defined in ascending order and may not overlap. +# +# A Layout Region start with a eight digit hex offset (leading "0x" required) followed by +# the pipe "|" character, followed by the size of the region, also in hex with the leading +# "0x" characters. Like: +# Offset|Size +# PcdOffsetCName|PcdSizeCName +# RegionType <FV, DATA, or FILE> +# +################################################################################ + +0x00000000|0x00280000 +FV = FVMAIN_COMPACT + +[FV.FVMAIN_COMPACT] +FvAlignment = 8 +ERASE_POLARITY = 1 +MEMORY_MAPPED = TRUE +STICKY_WRITE = TRUE +LOCK_CAP = TRUE +LOCK_STATUS = TRUE +WRITE_DISABLED_CAP = TRUE +WRITE_ENABLED_CAP = TRUE +WRITE_STATUS = TRUE +WRITE_LOCK_CAP = TRUE +WRITE_LOCK_STATUS = TRUE +READ_DISABLED_CAP = TRUE +READ_ENABLED_CAP = TRUE +READ_STATUS = TRUE +READ_LOCK_CAP = TRUE +READ_LOCK_STATUS = TRUE + + INF StandaloneMmPkg/Core/StandaloneMmCore.inf + INF Drivers/OpTeeRpmb/OpTeeRpmbFv.inf + INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf + INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf + INF StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf +################################################################################ +# +# Rules are use with the [FV] section's module INF type to define +# how an FFS file is created for a given INF file. The following Rule are the default +# rules for the different module type. User can add the customized rules to define the +# content of the FFS file. +# +################################################################################ + + +############################################################################ +# Example of a DXE_DRIVER FFS file with a Checksum encapsulation section # +############################################################################ +# +#[Rule.Common.DXE_DRIVER] +# FILE DRIVER = $(NAMED_GUID) { +# DXE_DEPEX DXE_DEPEX Optional $(INF_OUTPUT)/$(MODULE_NAME).depex +# COMPRESS PI_STD { +# GUIDED { +# PE32 PE32 $(INF_OUTPUT)/$(MODULE_NAME).efi +# UI STRING="$(MODULE_NAME)" Optional +# VERSION STRING="$(INF_VERSION)" Optional BUILD_NUM=$(BUILD_NUMBER) +# } +# } +# } +# +############################################################################ + +[Rule.Common.MM_CORE_STANDALONE] + FILE SEC = $(NAMED_GUID) FIXED { + PE32 PE32 Align = Auto $(INF_OUTPUT)/$(MODULE_NAME).efi + } + +[Rule.Common.MM_STANDALONE] + FILE MM_STANDALONE = $(NAMED_GUID) { + SMM_DEPEX SMM_DEPEX Optional $(INF_OUTPUT)/$(MODULE_NAME).depex + PE32 PE32 $(INF_OUTPUT)/$(MODULE_NAME).efi + UI STRING="$(MODULE_NAME)" Optional + VERSION STRING="$(INF_VERSION)" Optional BUILD_NUM=$(BUILD_NUMBER) + } -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH edk2-platforms v3 2/2] StMMRpmb: Add support for building StandaloneMm image for OP-TEE 2020-12-16 11:09 ` [PATCH edk2-platforms v3 2/2] StMMRpmb: Add support for building StandaloneMm image for OP-TEE Sughosh Ganu @ 2021-01-29 10:29 ` Sami Mujawar 2021-01-29 11:47 ` Ilias Apalodimas 0 siblings, 1 reply; 14+ messages in thread From: Sami Mujawar @ 2021-01-29 10:29 UTC (permalink / raw) To: Sughosh Ganu, devel@edk2.groups.io Cc: ardb+tianocore@kernel.org, Leif Lindholm, Sahil Malhotra, Ilias Apalodimas, nd Hi Sughosh, Please find my response inline marked [SAMI]. There are a few minor suggestions, otherwise this patch looks good to me. With that changed. Reviewed-by: Sami Mujawar <sami.mujawar@arm.com> Regards, Sami Mujawar -----Original Message----- From: Sughosh Ganu <sughosh.ganu@linaro.org> Sent: 16 December 2020 11:09 AM To: devel@edk2.groups.io Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Leif Lindholm <leif@nuviainc.com>; Sahil Malhotra <sahil.malhotra@linaro.org>; Ilias Apalodimas <ilias.apalodimas@linaro.org> Subject: [PATCH edk2-platforms v3 2/2] StMMRpmb: Add support for building StandaloneMm image for OP-TEE From: Ilias Apalodimas <ilias.apalodimas@linaro.org> With some recent changes in OP-TEE [1] and U-Boot [2] we can compile StMM and launch it from an OP-TEE secure partition which is mimicking SPM. There's a number of advantages in this approach. In Arm world SPM, currently used for dispatching StMM, and SPD used for OP-TEE, are mutually exclusive. Since there's no application in OP-TEE for managing EFI variables, this means that one can have a secure OS or secure variable storage. By re-using StMM we have EDK2s approved application controlling variable storage and the ability to run a secure world OS. This also allows various firmware implementations to adopt EDK2 way of storing variables (including the FTW implementation), as long as OP-TEE is available on that given platform (or any other secure OS that can launch StMM and has a supplicant for handling the RPMB partition). Another advantage is that OP-TEE has the ability to access an eMMC RPMB partition to store those variables. This requires a normal world supplicant, which is implemented in U-Boot currently. The supplicant picks up the encrypted buffer from OP-TEE and wires it to the eMMC driver(s). Similar functionality can be added in EDK2 by porting the supplicant and adapt it to using the native eMMC drivers. There's is one drawback in using OP-TEE. The current SPM calls need to run to completion. This contradicts the current OP-TEE RPC call requirements, used to access the RPMB storage. Thats leads to two different SMC calls for entering secure world to access StMM. So let's add support for a platform that compiles StMM and an RPMB driver that communicates with OP-TEE to read/write the variables. For anyone interested in testing this there's repo that builds all the sources and works on QEMU [3]. [1] https://github.com/OP-TEE/optee_os/pull/3973 [2] http://u-boot.10912.n7.nabble.com/PATCH-0-7-v4-EFI-variable-support-via-OP-TEE-td412499.html [3] https://git.linaro.org/people/ilias.apalodimas/efi_optee_variables.git/ Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- Changes since V2: None Platform/StMMRpmb/PlatformStandaloneMm.dsc | 168 ++++++++++++++++++++ Platform/StMMRpmb/PlatformStandaloneMm.fdf | 111 +++++++++++++ 2 files changed, 279 insertions(+) diff --git a/Platform/StMMRpmb/PlatformStandaloneMm.dsc b/Platform/StMMRpmb/PlatformStandaloneMm.dsc new file mode 100644 index 0000000000..93596c0630 --- /dev/null +++ b/Platform/StMMRpmb/PlatformStandaloneMm.dsc @@ -0,0 +1,168 @@ +# +# Copyright (c) 2018, ARM Limited. All rights reserved. +# Copyright (c) 2020, Linaro Ltd. All rights reserved. +# +# SPDX-License-Identifier: BSD-2-Clause-Patent +# + +################################################################################ +# +# Defines Section - statements that will be processed to create a Makefile. +# +################################################################################ +[Defines] + PLATFORM_NAME = MmStandaloneRpmb + PLATFORM_GUID = A27A486E-D7B9-4D70-9F37-FED9ABE041A2 + PLATFORM_VERSION = 1.0 + DSC_SPECIFICATION = 0x00010011 + OUTPUT_DIRECTORY = Build/$(PLATFORM_NAME) + SUPPORTED_ARCHITECTURES = AARCH64 + BUILD_TARGETS = DEBUG|RELEASE|NOOPT + SKUID_IDENTIFIER = DEFAULT + FLASH_DEFINITION = Platform/StMMRpmb/PlatformStandaloneMm.fdf + DEFINE DEBUG_MESSAGE = TRUE + + # LzmaF86 + DEFINE COMPRESSION_TOOL_GUID = D42AE6BD-1352-4bfb-909A-CA72A6EAE889 + +################################################################################ +# +# Library Class section - list of all Library Classes needed by this Platform. +# +################################################################################ +[LibraryClasses] + ArmSvcLib|ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf + ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf + BaseLib|MdePkg/Library/BaseLib/BaseLib.inf + SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf + VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf + BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf + DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf + DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf + ExtractGuidedSectionLib|EmbeddedPkg/Library/PrePiExtractGuidedSectionLib/PrePiExtractGuidedSectionLib.inf + FvLib|StandaloneMmPkg/Library/FvLib/FvLib.inf + HobLib|StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf + IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf + MemLib|StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf + MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmCoreMemoryAllocationLib/StandaloneMmCoreMemoryAllocationLib.inf + PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf + PeCoffLib|MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf + PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf + VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf + ReportStatusCodeLib|MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf + + # + # Entry point + # + #StandaloneMmCoreEntryPoint|StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf [SAMI] This line can be removed. [/SAMI] + StandaloneMmCoreEntryPoint|StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf + StandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntryPoint/StandaloneMmDriverEntryPoint.inf + + StandaloneMmMmuLib|ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf + #CacheMaintenanceLib|ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.inf [SAMI] remove? [/SAMI] + CacheMaintenanceLib|MdePkg/Library/BaseCacheMaintenanceLibNull/BaseCacheMaintenanceLibNull.inf + PeCoffExtraActionLib|StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf + RngLib|MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf + + SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf + DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf [SAMI] This appears twice. Can the previous instance be removed? [/SAMI] + + # + # It is not possible to prevent the ARM compiler for generic intrinsic functions. + # This library provides the intrinsic functions generate by a given compiler. + # NULL means link this library into all ARM images. + # + NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf + +[LibraryClasses.common.MM_STANDALONE] + HobLib|StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf + MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf + MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.inf + + IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf + OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf + PlatformSecureLib|SecurityPkg/Library/PlatformSecureLibNull/PlatformSecureLibNull.inf + SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf + TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf +################################################################################ +# +# Pcd Section - list of all EDK II PCD Entries defined by this Platform +# +################################################################################ + +[PcdsFeatureFlag.common] + gArmTokenSpaceGuid.PcdFfaEnable|TRUE + +[PcdsFixedAtBuild] + gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x800000CF + gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0xff + gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x0f + + gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x2 + # Secure Storage + gEfiSecurityPkgTokenSpaceGuid.PcdUserPhysicalPresence|TRUE + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800 + + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00004000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00004000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00004000 + gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x00004000 + +[PcdsPatchableInModule] + # Allocated memory for EDK2 uppers layers + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x0 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x0 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x0 + +################################################################################################### +# +# Components Section - list of the modules and components that will be processed by compilation +# tools and the EDK II tools to generate PE32/PE32+/Coff image files. +# +# Note: The EDK II DSC file is not used to specify how compiled binary images get placed +# into firmware volume images. This section is just a list of modules to compile from +# source into UEFI-compliant binaries. +# It is the FDF file that contains information on combining binary files into firmware +# volume images, whose concept is beyond UEFI and is described in PI specification. +# Binary modules do not need to be listed in this section, as they should be +# specified in the FDF file. For example: Shell binary (Shell_Full.efi), FAT binary (Fat.efi), +# Logo (Logo.bmp), and etc. +# There may also be modules listed in this section that are not required in the FDF file, +# When a module listed here is excluded from FDF file, then UEFI-compliant binary will be +# generated for it, but the binary will not be put into any firmware volume. +# +################################################################################################### +[Components.common] + # + # Standalone MM components + # + Drivers/OpTeeRpmb/OpTeeRpmbFv.inf + StandaloneMmPkg/Core/StandaloneMmCore.inf + StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf + MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf { + <LibraryClasses> + NULL|Drivers/OpTeeRpmb/FixupPcd.inf + } + MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf { + <LibraryClasses> + AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf + BaseCryptLib|CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf + DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf + VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf + NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf + NULL|Drivers/OpTeeRpmb/FixupPcd.inf + } + +################################################################################################### +# +# BuildOptions Section - Define the module specific tool chain flags that should be used as +# the default flags for a module. These flags are appended to any +# standard flags that are defined by the build process. They can be +# applied for any modules or only those modules with the specific +# module style (EDK or EDKII) specified in [Components] section. +# +################################################################################################### +[BuildOptions.AARCH64] +GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000 -march=armv8-a+nofp +GCC:*_*_*_CC_FLAGS = -mstrict-align diff --git a/Platform/StMMRpmb/PlatformStandaloneMm.fdf b/Platform/StMMRpmb/PlatformStandaloneMm.fdf new file mode 100644 index 0000000000..febc6d0d95 --- /dev/null +++ b/Platform/StMMRpmb/PlatformStandaloneMm.fdf @@ -0,0 +1,111 @@ +# +# Copyright (c) 2018, ARM Limited. All rights reserved. +# Copyright (c) 2020, Linaro Ltd. All rights reserved. +# +# SPDX-License-Identifier: BSD-2-Clause-Patent +# + +################################################################################ +# +# FD Section +# The [FD] Section is made up of the definition statements and a +# description of what goes into the Flash Device Image. Each FD section +# defines one flash "device" image. A flash device image may be one of +# the following: Removable media bootable image (like a boot floppy +# image,) an Option ROM image (that would be "flashed" into an add-in +# card,) a System "Flash" image (that would be burned into a system's +# flash) or an Update ("Capsule") image that will be used to update and +# existing system flash. +# +################################################################################ + +[FD.BL32_AP_MM] +BaseAddress = 0x1000 # any address apart from 0x0 +Size = 0x00300000 +ErasePolarity = 1 + +BlockSize = 0x00001000 +NumBlocks = 0x0300 + +################################################################################ +# +# Following are lists of FD Region layout which correspond to the locations of different +# images within the flash device. +# +# Regions must be defined in ascending order and may not overlap. +# +# A Layout Region start with a eight digit hex offset (leading "0x" required) followed by +# the pipe "|" character, followed by the size of the region, also in hex with the leading +# "0x" characters. Like: +# Offset|Size +# PcdOffsetCName|PcdSizeCName +# RegionType <FV, DATA, or FILE> +# +################################################################################ + +0x00000000|0x00280000 +FV = FVMAIN_COMPACT + +[FV.FVMAIN_COMPACT] +FvAlignment = 8 +ERASE_POLARITY = 1 +MEMORY_MAPPED = TRUE +STICKY_WRITE = TRUE +LOCK_CAP = TRUE +LOCK_STATUS = TRUE +WRITE_DISABLED_CAP = TRUE +WRITE_ENABLED_CAP = TRUE +WRITE_STATUS = TRUE +WRITE_LOCK_CAP = TRUE +WRITE_LOCK_STATUS = TRUE +READ_DISABLED_CAP = TRUE +READ_ENABLED_CAP = TRUE +READ_STATUS = TRUE +READ_LOCK_CAP = TRUE +READ_LOCK_STATUS = TRUE + + INF StandaloneMmPkg/Core/StandaloneMmCore.inf + INF Drivers/OpTeeRpmb/OpTeeRpmbFv.inf + INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf + INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf + INF StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf +################################################################################ +# +# Rules are use with the [FV] section's module INF type to define +# how an FFS file is created for a given INF file. The following Rule are the default +# rules for the different module type. User can add the customized rules to define the +# content of the FFS file. +# +################################################################################ + + +############################################################################ +# Example of a DXE_DRIVER FFS file with a Checksum encapsulation section # +############################################################################ +# +#[Rule.Common.DXE_DRIVER] +# FILE DRIVER = $(NAMED_GUID) { +# DXE_DEPEX DXE_DEPEX Optional $(INF_OUTPUT)/$(MODULE_NAME).depex +# COMPRESS PI_STD { +# GUIDED { +# PE32 PE32 $(INF_OUTPUT)/$(MODULE_NAME).efi +# UI STRING="$(MODULE_NAME)" Optional +# VERSION STRING="$(INF_VERSION)" Optional BUILD_NUM=$(BUILD_NUMBER) +# } +# } +# } +# +############################################################################ + +[Rule.Common.MM_CORE_STANDALONE] + FILE SEC = $(NAMED_GUID) FIXED { + PE32 PE32 Align = Auto $(INF_OUTPUT)/$(MODULE_NAME).efi + } + +[Rule.Common.MM_STANDALONE] + FILE MM_STANDALONE = $(NAMED_GUID) { + SMM_DEPEX SMM_DEPEX Optional $(INF_OUTPUT)/$(MODULE_NAME).depex + PE32 PE32 $(INF_OUTPUT)/$(MODULE_NAME).efi + UI STRING="$(MODULE_NAME)" Optional + VERSION STRING="$(INF_VERSION)" Optional BUILD_NUM=$(BUILD_NUMBER) + } -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH edk2-platforms v3 2/2] StMMRpmb: Add support for building StandaloneMm image for OP-TEE 2021-01-29 10:29 ` Sami Mujawar @ 2021-01-29 11:47 ` Ilias Apalodimas 0 siblings, 0 replies; 14+ messages in thread From: Ilias Apalodimas @ 2021-01-29 11:47 UTC (permalink / raw) To: Sami Mujawar Cc: Sughosh Ganu, devel@edk2.groups.io, ardb+tianocore@kernel.org, Leif Lindholm, Sahil Malhotra, nd Thanks Sami, I'll fix the remarks and resend On Fri, 29 Jan 2021 at 12:29, Sami Mujawar <Sami.Mujawar@arm.com> wrote: > > Hi Sughosh, > > Please find my response inline marked [SAMI]. > > There are a few minor suggestions, otherwise this patch looks good to me. > With that changed. > > Reviewed-by: Sami Mujawar <sami.mujawar@arm.com> > > Regards, > > Sami Mujawar > > -----Original Message----- > From: Sughosh Ganu <sughosh.ganu@linaro.org> > Sent: 16 December 2020 11:09 AM > To: devel@edk2.groups.io > Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Leif Lindholm <leif@nuviainc.com>; Sahil Malhotra <sahil.malhotra@linaro.org>; Ilias Apalodimas <ilias.apalodimas@linaro.org> > Subject: [PATCH edk2-platforms v3 2/2] StMMRpmb: Add support for building StandaloneMm image for OP-TEE > > From: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > With some recent changes in OP-TEE [1] and U-Boot [2] we can compile StMM > and launch it from an OP-TEE secure partition which is mimicking SPM. > > There's a number of advantages in this approach. In Arm world SPM, > currently used for dispatching StMM, and SPD used for OP-TEE, are > mutually exclusive. Since there's no application in OP-TEE for managing > EFI variables, this means that one can have a secure OS or secure > variable storage. > > By re-using StMM we have EDK2s approved application controlling > variable storage and the ability to run a secure world OS. This also > allows various firmware implementations to adopt EDK2 way of storing > variables (including the FTW implementation), as long as OP-TEE is > available on that given platform (or any other secure OS that can launch > StMM and has a supplicant for handling the RPMB partition). > Another advantage is that OP-TEE has the ability to access an eMMC RPMB > partition to store those variables. This requires a normal world > supplicant, which is implemented in U-Boot currently. The supplicant > picks up the encrypted buffer from OP-TEE and wires it to the eMMC > driver(s). Similar functionality can be added in EDK2 by porting the > supplicant and adapt it to using the native eMMC drivers. > > There's is one drawback in using OP-TEE. The current SPM calls need to run > to completion. This contradicts the current OP-TEE RPC call requirements, > used to access the RPMB storage. Thats leads to two different SMC calls for > entering secure world to access StMM. > > So let's add support for a platform that compiles StMM and an RPMB > driver that communicates with OP-TEE to read/write the variables. > For anyone interested in testing this there's repo that builds all the > sources and works on QEMU [3]. > > [1] https://github.com/OP-TEE/optee_os/pull/3973 > [2] http://u-boot.10912.n7.nabble.com/PATCH-0-7-v4-EFI-variable-support-via-OP-TEE-td412499.html > [3] https://git.linaro.org/people/ilias.apalodimas/efi_optee_variables.git/ > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > > Changes since V2: None > > Platform/StMMRpmb/PlatformStandaloneMm.dsc | 168 ++++++++++++++++++++ > Platform/StMMRpmb/PlatformStandaloneMm.fdf | 111 +++++++++++++ > 2 files changed, 279 insertions(+) > > diff --git a/Platform/StMMRpmb/PlatformStandaloneMm.dsc b/Platform/StMMRpmb/PlatformStandaloneMm.dsc > new file mode 100644 > index 0000000000..93596c0630 > --- /dev/null > +++ b/Platform/StMMRpmb/PlatformStandaloneMm.dsc > @@ -0,0 +1,168 @@ > +# > +# Copyright (c) 2018, ARM Limited. All rights reserved. > +# Copyright (c) 2020, Linaro Ltd. All rights reserved. > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > + > +################################################################################ > +# > +# Defines Section - statements that will be processed to create a Makefile. > +# > +################################################################################ > +[Defines] > + PLATFORM_NAME = MmStandaloneRpmb > + PLATFORM_GUID = A27A486E-D7B9-4D70-9F37-FED9ABE041A2 > + PLATFORM_VERSION = 1.0 > + DSC_SPECIFICATION = 0x00010011 > + OUTPUT_DIRECTORY = Build/$(PLATFORM_NAME) > + SUPPORTED_ARCHITECTURES = AARCH64 > + BUILD_TARGETS = DEBUG|RELEASE|NOOPT > + SKUID_IDENTIFIER = DEFAULT > + FLASH_DEFINITION = Platform/StMMRpmb/PlatformStandaloneMm.fdf > + DEFINE DEBUG_MESSAGE = TRUE > + > + # LzmaF86 > + DEFINE COMPRESSION_TOOL_GUID = D42AE6BD-1352-4bfb-909A-CA72A6EAE889 > + > +################################################################################ > +# > +# Library Class section - list of all Library Classes needed by this Platform. > +# > +################################################################################ > +[LibraryClasses] > + ArmSvcLib|ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf > + ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf > + BaseLib|MdePkg/Library/BaseLib/BaseLib.inf > + SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf > + VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf > + BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf > + DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf > + DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf > + ExtractGuidedSectionLib|EmbeddedPkg/Library/PrePiExtractGuidedSectionLib/PrePiExtractGuidedSectionLib.inf > + FvLib|StandaloneMmPkg/Library/FvLib/FvLib.inf > + HobLib|StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf > + IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf > + MemLib|StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf > + MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmCoreMemoryAllocationLib/StandaloneMmCoreMemoryAllocationLib.inf > + PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf > + PeCoffLib|MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf > + PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf > + VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf > + ReportStatusCodeLib|MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf > + > + # > + # Entry point > + # > + #StandaloneMmCoreEntryPoint|StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf > [SAMI] This line can be removed. > [/SAMI] > + StandaloneMmCoreEntryPoint|StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf > + StandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntryPoint/StandaloneMmDriverEntryPoint.inf > + > + StandaloneMmMmuLib|ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf > + #CacheMaintenanceLib|ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.inf > [SAMI] remove? > [/SAMI] > + CacheMaintenanceLib|MdePkg/Library/BaseCacheMaintenanceLibNull/BaseCacheMaintenanceLibNull.inf > + PeCoffExtraActionLib|StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf > + RngLib|MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf > + > + SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf > + DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf > [SAMI] This appears twice. Can the previous instance be removed? > [/SAMI] > + > + # > + # It is not possible to prevent the ARM compiler for generic intrinsic functions. > + # This library provides the intrinsic functions generate by a given compiler. > + # NULL means link this library into all ARM images. > + # > + NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf > + > +[LibraryClasses.common.MM_STANDALONE] > + HobLib|StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf > + MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf > + MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.inf > + > + IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf > + OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf > + PlatformSecureLib|SecurityPkg/Library/PlatformSecureLibNull/PlatformSecureLibNull.inf > + SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf > + TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf > +################################################################################ > +# > +# Pcd Section - list of all EDK II PCD Entries defined by this Platform > +# > +################################################################################ > + > +[PcdsFeatureFlag.common] > + gArmTokenSpaceGuid.PcdFfaEnable|TRUE > + > +[PcdsFixedAtBuild] > + gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x800000CF > + gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0xff > + gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x0f > + > + gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x2 > + # Secure Storage > + gEfiSecurityPkgTokenSpaceGuid.PcdUserPhysicalPresence|TRUE > + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 > + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800 > + > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00004000 > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00004000 > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00004000 > + gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x00004000 > + > +[PcdsPatchableInModule] > + # Allocated memory for EDK2 uppers layers > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x0 > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x0 > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x0 > + > +################################################################################################### > +# > +# Components Section - list of the modules and components that will be processed by compilation > +# tools and the EDK II tools to generate PE32/PE32+/Coff image files. > +# > +# Note: The EDK II DSC file is not used to specify how compiled binary images get placed > +# into firmware volume images. This section is just a list of modules to compile from > +# source into UEFI-compliant binaries. > +# It is the FDF file that contains information on combining binary files into firmware > +# volume images, whose concept is beyond UEFI and is described in PI specification. > +# Binary modules do not need to be listed in this section, as they should be > +# specified in the FDF file. For example: Shell binary (Shell_Full.efi), FAT binary (Fat.efi), > +# Logo (Logo.bmp), and etc. > +# There may also be modules listed in this section that are not required in the FDF file, > +# When a module listed here is excluded from FDF file, then UEFI-compliant binary will be > +# generated for it, but the binary will not be put into any firmware volume. > +# > +################################################################################################### > +[Components.common] > + # > + # Standalone MM components > + # > + Drivers/OpTeeRpmb/OpTeeRpmbFv.inf > + StandaloneMmPkg/Core/StandaloneMmCore.inf > + StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf > + MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf { > + <LibraryClasses> > + NULL|Drivers/OpTeeRpmb/FixupPcd.inf > + } > + MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf { > + <LibraryClasses> > + AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf > + BaseCryptLib|CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf > + DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf > + VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf > + NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf > + NULL|Drivers/OpTeeRpmb/FixupPcd.inf > + } > + > +################################################################################################### > +# > +# BuildOptions Section - Define the module specific tool chain flags that should be used as > +# the default flags for a module. These flags are appended to any > +# standard flags that are defined by the build process. They can be > +# applied for any modules or only those modules with the specific > +# module style (EDK or EDKII) specified in [Components] section. > +# > +################################################################################################### > +[BuildOptions.AARCH64] > +GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000 -march=armv8-a+nofp > +GCC:*_*_*_CC_FLAGS = -mstrict-align > diff --git a/Platform/StMMRpmb/PlatformStandaloneMm.fdf b/Platform/StMMRpmb/PlatformStandaloneMm.fdf > new file mode 100644 > index 0000000000..febc6d0d95 > --- /dev/null > +++ b/Platform/StMMRpmb/PlatformStandaloneMm.fdf > @@ -0,0 +1,111 @@ > +# > +# Copyright (c) 2018, ARM Limited. All rights reserved. > +# Copyright (c) 2020, Linaro Ltd. All rights reserved. > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > + > +################################################################################ > +# > +# FD Section > +# The [FD] Section is made up of the definition statements and a > +# description of what goes into the Flash Device Image. Each FD section > +# defines one flash "device" image. A flash device image may be one of > +# the following: Removable media bootable image (like a boot floppy > +# image,) an Option ROM image (that would be "flashed" into an add-in > +# card,) a System "Flash" image (that would be burned into a system's > +# flash) or an Update ("Capsule") image that will be used to update and > +# existing system flash. > +# > +################################################################################ > + > +[FD.BL32_AP_MM] > +BaseAddress = 0x1000 # any address apart from 0x0 > +Size = 0x00300000 > +ErasePolarity = 1 > + > +BlockSize = 0x00001000 > +NumBlocks = 0x0300 > + > +################################################################################ > +# > +# Following are lists of FD Region layout which correspond to the locations of different > +# images within the flash device. > +# > +# Regions must be defined in ascending order and may not overlap. > +# > +# A Layout Region start with a eight digit hex offset (leading "0x" required) followed by > +# the pipe "|" character, followed by the size of the region, also in hex with the leading > +# "0x" characters. Like: > +# Offset|Size > +# PcdOffsetCName|PcdSizeCName > +# RegionType <FV, DATA, or FILE> > +# > +################################################################################ > + > +0x00000000|0x00280000 > +FV = FVMAIN_COMPACT > + > +[FV.FVMAIN_COMPACT] > +FvAlignment = 8 > +ERASE_POLARITY = 1 > +MEMORY_MAPPED = TRUE > +STICKY_WRITE = TRUE > +LOCK_CAP = TRUE > +LOCK_STATUS = TRUE > +WRITE_DISABLED_CAP = TRUE > +WRITE_ENABLED_CAP = TRUE > +WRITE_STATUS = TRUE > +WRITE_LOCK_CAP = TRUE > +WRITE_LOCK_STATUS = TRUE > +READ_DISABLED_CAP = TRUE > +READ_ENABLED_CAP = TRUE > +READ_STATUS = TRUE > +READ_LOCK_CAP = TRUE > +READ_LOCK_STATUS = TRUE > + > + INF StandaloneMmPkg/Core/StandaloneMmCore.inf > + INF Drivers/OpTeeRpmb/OpTeeRpmbFv.inf > + INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf > + INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf > + INF StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/StandaloneMmCpu.inf > +################################################################################ > +# > +# Rules are use with the [FV] section's module INF type to define > +# how an FFS file is created for a given INF file. The following Rule are the default > +# rules for the different module type. User can add the customized rules to define the > +# content of the FFS file. > +# > +################################################################################ > + > + > +############################################################################ > +# Example of a DXE_DRIVER FFS file with a Checksum encapsulation section # > +############################################################################ > +# > +#[Rule.Common.DXE_DRIVER] > +# FILE DRIVER = $(NAMED_GUID) { > +# DXE_DEPEX DXE_DEPEX Optional $(INF_OUTPUT)/$(MODULE_NAME).depex > +# COMPRESS PI_STD { > +# GUIDED { > +# PE32 PE32 $(INF_OUTPUT)/$(MODULE_NAME).efi > +# UI STRING="$(MODULE_NAME)" Optional > +# VERSION STRING="$(INF_VERSION)" Optional BUILD_NUM=$(BUILD_NUMBER) > +# } > +# } > +# } > +# > +############################################################################ > + > +[Rule.Common.MM_CORE_STANDALONE] > + FILE SEC = $(NAMED_GUID) FIXED { > + PE32 PE32 Align = Auto $(INF_OUTPUT)/$(MODULE_NAME).efi > + } > + > +[Rule.Common.MM_STANDALONE] > + FILE MM_STANDALONE = $(NAMED_GUID) { > + SMM_DEPEX SMM_DEPEX Optional $(INF_OUTPUT)/$(MODULE_NAME).depex > + PE32 PE32 $(INF_OUTPUT)/$(MODULE_NAME).efi > + UI STRING="$(MODULE_NAME)" Optional > + VERSION STRING="$(INF_VERSION)" Optional BUILD_NUM=$(BUILD_NUMBER) > + } > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-02-02 16:28 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-12-16 11:09 [PATCH edk2-platforms v3 0/2] Add support for running StandaloneMm as OP-TEE TA Sughosh Ganu 2020-12-16 11:09 ` [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver Sughosh Ganu 2021-01-27 17:10 ` Sami Mujawar 2021-01-29 8:02 ` Ilias Apalodimas 2021-01-29 11:45 ` Sami Mujawar 2021-02-01 14:00 ` Ilias Apalodimas 2021-02-02 10:40 ` Sami Mujawar 2021-02-02 12:33 ` Ilias Apalodimas 2021-02-02 14:49 ` Ilias Apalodimas 2021-02-02 15:13 ` Sami Mujawar 2021-02-02 16:27 ` Ilias Apalodimas 2020-12-16 11:09 ` [PATCH edk2-platforms v3 2/2] StMMRpmb: Add support for building StandaloneMm image for OP-TEE Sughosh Ganu 2021-01-29 10:29 ` Sami Mujawar 2021-01-29 11:47 ` Ilias Apalodimas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox