* [PATCH 0/2 v5] Add support for running StandaloneMm as OP-TEE TA
@ 2021-02-12 17:34 Ilias Apalodimas
2021-02-12 17:34 ` [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver Ilias Apalodimas
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Ilias Apalodimas @ 2021-02-12 17:34 UTC (permalink / raw)
To: devel, sami.mujawar; +Cc: ardb+tianocore, sughosh.ganu, leif, Ilias Apalodimas
Hi,
This is v5 of [1]
Changes since V4:
- More coding stule fixes proposed by Sami, which Ecc or Patchcheck didn't
report.
- Adding missing error handling in InitializeFvAndVariableStoreHeaders().
An allocation wasn't properly checked for success
Changes since V3:
- Coding style fixes proposed by Sami
- Fixed all reported PatchCheck errors
- Added overflow checks on the base aaddress allocated for EFI variables.
The size of the partition is user defined (via Pcd's) and the memory layout
and allocation address depends on OP-TEE. So let's make sure we won't overflow
when calculating the 3 partitions needed for FTW
- Switched some PcdGet/Set32 to 64 to accomodate 64-bit addressing
- Removed some duplicate entries in Platform/StMMRpmb/PlatformStandaloneMm.dsc
- Added reviewed-by tags on patch 2/2
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
[1] https://edk2.groups.io/g/devel/message/66483?p=,,,20,0,0,0::Created,,ilias+apalodimas,20,2,0,77703661
Ilias Apalodimas (2):
Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver
StMMRpmb: Add support for building StandaloneMm image for OP-TEE
Drivers/OpTeeRpmb/FixupPcd.c | 89 ++
Drivers/OpTeeRpmb/FixupPcd.inf | 43 +
Drivers/OpTeeRpmb/OpTeeRpmbFv.inf | 58 ++
Drivers/OpTeeRpmb/OpTeeRpmbFvb.c | 920 +++++++++++++++++++++
Drivers/OpTeeRpmb/OpTeeRpmbFvb.h | 52 ++
Platform/StMMRpmb/PlatformStandaloneMm.dsc | 165 ++++
Platform/StMMRpmb/PlatformStandaloneMm.fdf | 111 +++
7 files changed, 1438 insertions(+)
create mode 100644 Drivers/OpTeeRpmb/FixupPcd.c
create mode 100644 Drivers/OpTeeRpmb/FixupPcd.inf
create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFv.inf
create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFvb.c
create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFvb.h
create mode 100644 Platform/StMMRpmb/PlatformStandaloneMm.dsc
create mode 100644 Platform/StMMRpmb/PlatformStandaloneMm.fdf
--
2.30.0
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver 2021-02-12 17:34 [PATCH 0/2 v5] Add support for running StandaloneMm as OP-TEE TA Ilias Apalodimas @ 2021-02-12 17:34 ` Ilias Apalodimas 2021-03-03 10:20 ` [edk2-devel] " PierreGondois 2021-03-05 17:58 ` PierreGondois 2021-02-12 17:34 ` [PATCH 2/2 v5] StMMRpmb: Add support for building StandaloneMm image for OP-TEE Ilias Apalodimas 2021-03-03 11:32 ` [edk2-devel] [PATCH 0/2 v5] Add support for running StandaloneMm as OP-TEE TA Sami Mujawar 2 siblings, 2 replies; 14+ messages in thread From: Ilias Apalodimas @ 2021-02-12 17:34 UTC (permalink / raw) To: devel, sami.mujawar; +Cc: ardb+tianocore, sughosh.ganu, leif, Ilias Apalodimas 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> --- Drivers/OpTeeRpmb/FixupPcd.c | 89 +++ Drivers/OpTeeRpmb/FixupPcd.inf | 43 ++ Drivers/OpTeeRpmb/OpTeeRpmbFv.inf | 58 ++ Drivers/OpTeeRpmb/OpTeeRpmbFvb.c | 920 ++++++++++++++++++++++++++++++ Drivers/OpTeeRpmb/OpTeeRpmbFvb.h | 52 ++ 5 files changed, 1162 insertions(+) create mode 100644 Drivers/OpTeeRpmb/FixupPcd.c create mode 100644 Drivers/OpTeeRpmb/FixupPcd.inf create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFv.inf create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFvb.c create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFvb.h diff --git a/Drivers/OpTeeRpmb/FixupPcd.c b/Drivers/OpTeeRpmb/FixupPcd.c new file mode 100644 index 000000000000..6cd503b71c6d --- /dev/null +++ b/Drivers/OpTeeRpmb/FixupPcd.c @@ -0,0 +1,89 @@ +/** @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); + // The Pcd is user defined, so make sure we don't overflow + if (Instance->MemBaseAddress > MAX_UINT64 - PcdGet32 (PcdFlashNvStorageVariableSize)) { + return EFI_INVALID_PARAMETER; + } + + if (Instance->MemBaseAddress > MAX_UINT64 - PcdGet32 (PcdFlashNvStorageVariableSize) - + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)) { + return EFI_INVALID_PARAMETER; + } + + // Patch PCDs with the the correct values + PatchPcdSet64 (PcdFlashNvStorageVariableBase64, Instance->MemBaseAddress); + PatchPcdSet64 ( + PcdFlashNvStorageFtwWorkingBase64, + Instance->MemBaseAddress + PcdGet32 (PcdFlashNvStorageVariableSize) + ); + PatchPcdSet64 ( + PcdFlashNvStorageFtwSpareBase64, + Instance->MemBaseAddress + + PcdGet32 (PcdFlashNvStorageVariableSize) + + PcdGet32 (PcdFlashNvStorageFtwWorkingSize) + ); + + DEBUG ((DEBUG_INFO, "%a: Fixup PcdFlashNvStorageVariableBase64: 0x%lx\n", + __FUNCTION__, PcdGet64 (PcdFlashNvStorageVariableBase64))); + DEBUG ((DEBUG_INFO, "%a: Fixup PcdFlashNvStorageFtwWorkingBase64: 0x%lx\n", + __FUNCTION__, PcdGet64 (PcdFlashNvStorageFtwWorkingBase64))); + DEBUG ((DEBUG_INFO, "%a: Fixup PcdFlashNvStorageFtwSpareBase64: 0x%lx\n", + __FUNCTION__, PcdGet64 (PcdFlashNvStorageFtwSpareBase64))); + + return Status; +} diff --git a/Drivers/OpTeeRpmb/FixupPcd.inf b/Drivers/OpTeeRpmb/FixupPcd.inf new file mode 100644 index 000000000000..5146257993ef --- /dev/null +++ b/Drivers/OpTeeRpmb/FixupPcd.inf @@ -0,0 +1,43 @@ +## @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.PcdFlashNvStorageFtwSpareBase64 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize + +[Protocols] + gEfiSmmFirmwareVolumeBlockProtocolGuid ## CONSUMES diff --git a/Drivers/OpTeeRpmb/OpTeeRpmbFv.inf b/Drivers/OpTeeRpmb/OpTeeRpmbFv.inf new file mode 100644 index 000000000000..c3580859ffde --- /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.PcdFlashNvStorageFtwSpareBase64 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize + +[Protocols] + gEfiSmmFirmwareVolumeBlockProtocolGuid ## PRODUCES + +[Depex] + TRUE diff --git a/Drivers/OpTeeRpmb/OpTeeRpmbFvb.c b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.c new file mode 100644 index 000000000000..950082cf6df4 --- /dev/null +++ b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.c @@ -0,0 +1,920 @@ +/** @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" + +// These are what OP-TEE expects in ./core/arch/arm/kernel/stmm_sp.c +// Since the FFA autodiscovery mechanism is not yet implemented we are +// hardcoding the ID values for the two operations OP-TEE currently supports +// +// mMemMgrId is used to set the page permissions after relocating the executable +// mStorageId is used to access the RPMB partition via OP-TEE +// In both cases the return value is located in x3. Once the autodiscovery mechanism +// is in place, we'll have to account for an error value in x2 as well, handling +// the autodiscovery failed scenario +STATIC CONST UINT16 mMemMgrId = 3U; +STATIC CONST UINT16 mStorageId = 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 EFI_SUCCESS read/write ok + + @retval EFI_UNSUPPORTED SVC to op-tee not supported + + @retval EFI_INVALID_PARAMETER SVC to op-tee had an invalid param + + @retval EFI_ACCESS_DENIED SVC to op-tee was denied + + @retval EFI_OUT_OF_RESOURCES op-tee out of memory +**/ +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 = mStorageId; + 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_OUT_OF_RESOURCES; + 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_UNSUPPORTED Set attributes not supported + +**/ +STATIC +EFI_STATUS +OpTeeRpmbFvbSetAttributes ( + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, + IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes + ) +{ + DEBUG ((DEBUG_ERROR, "FvbSetAttributes(0x%X) is not supported\n", *Attributes)); + return EFI_UNSUPPORTED; +} + +/** + 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; + MEM_INSTANCE *Instance; + VOID *Base; + + Status = EFI_SUCCESS; + Instance = INSTANCE_FROM_FVB_THIS (This); + if (!Instance->Initialized) { + Status = Instance->Initialize (Instance); + if (EFI_ERROR (Status)) { + return Status; + } + } + + 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; + VOID *Base; + + Instance = INSTANCE_FROM_FVB_THIS (This); + if (!Instance->Initialized) { + Status = Instance->Initialize (Instance); + if (EFI_ERROR (Status)) { + return Status; + } + } + Base = (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize + Offset; + Status = ReadWriteRpmb ( + SP_SVC_RPMB_WRITE, + (UINTN)Buffer, + *NumBytes, + (Lba * Instance->BlockSize) + Offset + ); + if (EFI_ERROR (Status)) { + return Status; + } + + // Update the memory copy + CopyMem (Base, Buffer, *NumBytes); + + return Status; +} + +/** + 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 == NULL) { + 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 (EFI_ERROR (Status)) { + 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 Instance Address to copy flash contents to +**/ +STATIC +VOID +ReadEntireFlash ( + MEM_INSTANCE *Instance + ) +{ + UINTN ReadAddr; + UINTN StorageFtwWorkingSize; + UINTN StorageVariableSize; + UINTN StorageFtwSpareSize; + + StorageFtwWorkingSize = PcdGet32(PcdFlashNvStorageFtwWorkingSize); + StorageVariableSize = PcdGet32(PcdFlashNvStorageVariableSize); + 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 + ); +} + +/** + Validate the firmware volume header. + + @param FwVolHeader Pointer to the firmware volume header for the RPMB + + @retval EFI_SUCCESS The firmware volume header is correct + @retval EFI_NOT_FOUND No header present + @retval EFI_VOLUME_CORRUPTED The firmware volume header CRC was invalid + or either one of GUID's, Signature and header + length was invalid +**/ +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_VOLUME_CORRUPTED; + } + + // 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_VOLUME_CORRUPTED; + } + + VariableStoreHeader = (VARIABLE_STORE_HEADER*)((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_VOLUME_CORRUPTED; + } + + VariableStoreLength = PcdGet32 (PcdFlashNvStorageVariableSize) - + FwVolHeader->HeaderLength; + if (VariableStoreHeader->Size != VariableStoreLength) { + DEBUG ((DEBUG_INFO, "%a: Variable Store Length does not match\n", + __FUNCTION__)); + return EFI_VOLUME_CORRUPTED; + } + + return EFI_SUCCESS; + +} + +/** + Initialize Fvb and variable storage headers for the RPMB storage. + + @param Instance MEM_INSTANCE pointer describing the device and the + Firmware Block Protocol + + @retval EFI_SUCCESS read/write ok + + @retval EFI_UNSUPPORTED SVC to op-tee not supported + + @retval EFI_INVALID_PARAMETER SVC to op-tee had an invalid param + + @retval EFI_ACCESS_DENIED SVC to op-tee was denied + + @retval EFI_OUT_OF_RESOURCES op-tee out of memory +**/ +STATIC +EFI_STATUS +InitializeFvAndVariableStoreHeaders ( + MEM_INSTANCE *Instance + ) +{ + EFI_FIRMWARE_VOLUME_HEADER *FirmwareVolumeHeader; + VARIABLE_STORE_HEADER *VariableStoreHeader; + EFI_STATUS Status; + UINTN HeadersLength; + VOID* Headers; + + HeadersLength = sizeof (EFI_FIRMWARE_VOLUME_HEADER) + + sizeof (EFI_FV_BLOCK_MAP_ENTRY) + + sizeof (VARIABLE_STORE_HEADER); + Headers = AllocateZeroPool (HeadersLength); + if (Headers == NULL) { + return EFI_OUT_OF_RESOURCES; + } + + // + // 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 = (VARIABLE_STORE_HEADER*)((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 (EFI_ERROR (Status)) { + goto Exit; + } + // Install the combined header in memory + CopyMem ((VOID*)Instance->MemBaseAddress, Headers, HeadersLength); + +Exit: + FreePool (Headers); + return Status; +} + +/** + Initialize the firmware block protocol for the specified memory. + + @param Instance MEM_INSTANCE pointer describing the device and the + Firmware Block Protocol + + @retval EFI_SUCCESS Initialized successfully or already initialized + @retval EFI_UNSUPPORTED SVC to op-tee not supported + @retval EFI_INVALID_PARAMETER SVC to op-tee had an invalid param + @retval EFI_ACCESS_DENIED SVC to op-tee was denied + @retval EFI_OUT_OF_RESOURCES op-tee out of memory + + + +**/ +STATIC +EFI_STATUS +EFIAPI +FvbInitialize ( + MEM_INSTANCE *Instance + ) +{ + EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader; + EFI_STATUS Status; + + if (Instance->Initialized) { + return EFI_SUCCESS; + } + + // FirmwareVolumeHeader->FvLength is declared to have the Variable area + // AND the FTW working area AND the FTW Spare contiguous. + ASSERT ( + (PcdGet64 (PcdFlashNvStorageVariableBase64) + + PcdGet32 (PcdFlashNvStorageVariableSize)) == + PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) + ); + ASSERT ( + (PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) + + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)) == + PcdGet64 (PcdFlashNvStorageFtwSpareBase64)); + + // 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 ((PcdGet64 (PcdFlashNvStorageVariableBase64) % Instance->BlockSize) == 0); + ASSERT ((PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) % Instance->BlockSize) == 0); + ASSERT ((PcdGet64 (PcdFlashNvStorageFtwSpareBase64) % 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 (EFI_ERROR (Status)) { + 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 Status; +} + +/** + Since the RPMB is not byte addressable we need to allocate memory + and sync that on reads/writes. Initialize the memory and install the + Fvb protocol. + + @param ImageHandle The EFI image handle + @param SystemTable MM system table + + @retval EFI_SUCCESS Protocol installed + + @retval EFI_INVALID_PARAMETER Invalid Pcd values specified + + @retval EFI_OUT_OF_RESOURCES Can't allocate necessary memory +**/ +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); + if (Addr == NULL) { + ASSERT (0); + return EFI_OUT_OF_RESOURCES; + } + + 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; + + // The Pcd is user defined, so make sure we don't overflow + if (mInstance.MemBaseAddress > MAX_UINT64 - PcdGet32 (PcdFlashNvStorageVariableSize)) { + return EFI_INVALID_PARAMETER; + } + + if (mInstance.MemBaseAddress > MAX_UINT64 - PcdGet32 (PcdFlashNvStorageVariableSize) - + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)) { + return EFI_INVALID_PARAMETER; + } + + // Update the defined PCDs related to Variable Storage + PatchPcdSet64 (PcdFlashNvStorageVariableBase64, mInstance.MemBaseAddress); + PatchPcdSet64 ( + PcdFlashNvStorageFtwWorkingBase64, + mInstance.MemBaseAddress + PcdGet32 (PcdFlashNvStorageVariableSize) + ); + PatchPcdSet64 ( + PcdFlashNvStorageFtwSpareBase64, + 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__, PatchPcdGet64 (PcdFlashNvStorageVariableBase64))); + + return Status; +} diff --git a/Drivers/OpTeeRpmb/OpTeeRpmbFvb.h b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.h new file mode 100644 index 000000000000..8305e776dbb6 --- /dev/null +++ b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.h @@ -0,0 +1,52 @@ +/** @file + + Copyright (c) 2020, Linaro Ltd. All rights reserved.<BR> + SPDX-License-Identifier: BSD-2-Clause-Patent + +**/ + +#ifndef OPTEE_RPMB_FVB_H_ +#define OPTEE_RPMB_FVB_H_ + +/** + Those are not currently defined in any spec, it's an internal + contract between OP-TEE and EDK2. + For more details check core/arch/arm/include/kernel/stmm_sp.h in OP-TEE +**/ +#define SP_SVC_RPMB_READ 0xC4000066 +#define SP_SVC_RPMB_WRITE 0xC4000067 + +#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); + +/** + This struct is used by the RPMB driver. Since the upper EDK2 layers + expect byte addressable memory, we allocate a memory area of certain + size and sync it to the hardware on reads/writes. + + @param[in] Signature Internal signature used to discover the instance + @param[in] Initialize Function used to initialize the instance + @param[in] Initialized Set to true if initialized + @param[in] FvbProtocol FVB protocol of the instance + @param[in] Handle Handle to install + @param[in] MemBaseAddress Physical address of the beggining of the allocated memory + @param[in] BlockSize Blocksize + @param[in] NBlocks Number of allocated blocks +**/ +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 -- 2.30.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver 2021-02-12 17:34 ` [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver Ilias Apalodimas @ 2021-03-03 10:20 ` PierreGondois 2021-03-03 20:02 ` Ilias Apalodimas 2021-03-05 17:58 ` PierreGondois 1 sibling, 1 reply; 14+ messages in thread From: PierreGondois @ 2021-03-03 10:20 UTC (permalink / raw) To: ilias.apalodimas, devel, Sami.Mujawar Hello Ilias, My review is mostly about coding style, but I would have some (inlined) remarks about your patch, Regards, Pierre On 3/2/21 3:35 PM, Pierre Gondois wrote: > > ------------------------------------------------------------------------ > *From:* devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Ilias > Apalodimas via groups.io <ilias.apalodimas=linaro.org@groups.io> > *Sent:* Friday, February 12, 2021 5:34 PM > *To:* devel@edk2.groups.io <devel@edk2.groups.io>; Sami Mujawar > <Sami.Mujawar@arm.com> > *Cc:* ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; > sughosh.ganu@linaro.org <sughosh.ganu@linaro.org>; leif@nuviainc.com > <leif@nuviainc.com>; Ilias Apalodimas <ilias.apalodimas@linaro.org> > *Subject:* [edk2-devel] [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an > OP-TEE backed RPMB driver > 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> > --- > Drivers/OpTeeRpmb/FixupPcd.c | 89 +++ > Drivers/OpTeeRpmb/FixupPcd.inf | 43 ++ > Drivers/OpTeeRpmb/OpTeeRpmbFv.inf | 58 ++ > Drivers/OpTeeRpmb/OpTeeRpmbFvb.c | 920 ++++++++++++++++++++++++++++++ > Drivers/OpTeeRpmb/OpTeeRpmbFvb.h | 52 ++ > 5 files changed, 1162 insertions(+) > create mode 100644 Drivers/OpTeeRpmb/FixupPcd.c > create mode 100644 Drivers/OpTeeRpmb/FixupPcd.inf > create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFv.inf > create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFvb.c > create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFvb.h > > diff --git a/Drivers/OpTeeRpmb/FixupPcd.c b/Drivers/OpTeeRpmb/FixupPcd.c > new file mode 100644 > index 000000000000..6cd503b71c6d > --- /dev/null > +++ b/Drivers/OpTeeRpmb/FixupPcd.c > @@ -0,0 +1,89 @@ > +/** @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 I think: is -> in > > + > > + @retval EFI_SUCCESS Protocol was found and PCDs patched up > > + > > + **/ I think there should not be a space here. > > +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); > > + // The Pcd is user defined, so make sure we don't overflow > > + if (Instance->MemBaseAddress > MAX_UINT64 - PcdGet32 > (PcdFlashNvStorageVariableSize)) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + if (Instance->MemBaseAddress > MAX_UINT64 - PcdGet32 > (PcdFlashNvStorageVariableSize) - > > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + // Patch PCDs with the the correct values I think it s 'the the' -> the > > + PatchPcdSet64 (PcdFlashNvStorageVariableBase64, > Instance->MemBaseAddress); > > + PatchPcdSet64 ( > > + PcdFlashNvStorageFtwWorkingBase64, > > + Instance->MemBaseAddress + PcdGet32 (PcdFlashNvStorageVariableSize) > > + ); > > + PatchPcdSet64 ( > > + PcdFlashNvStorageFtwSpareBase64, > > + Instance->MemBaseAddress + > > + PcdGet32 (PcdFlashNvStorageVariableSize) + > > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > > + ); > > + > > + DEBUG ((DEBUG_INFO, "%a: Fixup PcdFlashNvStorageVariableBase64: > 0x%lx\n", > > + __FUNCTION__, PcdGet64 (PcdFlashNvStorageVariableBase64))); > > + DEBUG ((DEBUG_INFO, "%a: Fixup PcdFlashNvStorageFtwWorkingBase64: > 0x%lx\n", > > + __FUNCTION__, PcdGet64 (PcdFlashNvStorageFtwWorkingBase64))); > > + DEBUG ((DEBUG_INFO, "%a: Fixup PcdFlashNvStorageFtwSpareBase64: > 0x%lx\n", > > + __FUNCTION__, PcdGet64 (PcdFlashNvStorageFtwSpareBase64))); > > + > > + return Status; > > +} > > diff --git a/Drivers/OpTeeRpmb/FixupPcd.inf > b/Drivers/OpTeeRpmb/FixupPcd.inf > new file mode 100644 > index 000000000000..5146257993ef > --- /dev/null > +++ b/Drivers/OpTeeRpmb/FixupPcd.inf > @@ -0,0 +1,43 @@ > +## @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.PcdFlashNvStorageFtwSpareBase64 > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64 > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize > > + > > +[Protocols] > > + gEfiSmmFirmwareVolumeBlockProtocolGuid ## CONSUMES > > diff --git a/Drivers/OpTeeRpmb/OpTeeRpmbFv.inf > b/Drivers/OpTeeRpmb/OpTeeRpmbFv.inf > new file mode 100644 > index 000000000000..c3580859ffde > --- /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.PcdFlashNvStorageFtwSpareBase64 > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64 > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize > > + > > +[Protocols] > > + gEfiSmmFirmwareVolumeBlockProtocolGuid ## PRODUCES > > + > > +[Depex] > > + TRUE > > diff --git a/Drivers/OpTeeRpmb/OpTeeRpmbFvb.c > b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.c > new file mode 100644 > index 000000000000..950082cf6df4 > --- /dev/null > +++ b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.c > @@ -0,0 +1,920 @@ > +/** @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" > > + > > +// These are what OP-TEE expects in ./core/arch/arm/kernel/stmm_sp.c > > +// Since the FFA autodiscovery mechanism is not yet implemented we are > > +// hardcoding the ID values for the two operations OP-TEE currently > supports > > +// > > +// mMemMgrId is used to set the page permissions after relocating the > executable > > +// mStorageId is used to access the RPMB partition via OP-TEE > > +// In both cases the return value is located in x3. Once the > autodiscovery mechanism > > +// is in place, we'll have to account for an error value in x2 as > well, handling > > +// the autodiscovery failed scenario > +STATIC CONST UINT16 mMemMgrId = 3U; > > +STATIC CONST UINT16 mStorageId = 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 EFI_SUCCESS read/write ok > > + > > + @retval EFI_UNSUPPORTED SVC to op-tee not supported > > + > > + @retval EFI_INVALID_PARAMETER SVC to op-tee had an > invalid param > > + > > + @retval EFI_ACCESS_DENIED SVC to op-tee was denied > > + > > + @retval EFI_OUT_OF_RESOURCES op-tee out of memory > > +**/ > > +STATIC > > +EFI_STATUS > > +ReadWriteRpmb ( > > + UINTN SvcAct, > > + UINTN Addr, > > + UINTN NumBytes, > > + UINTN Offset > > + ) I think the parameters should have IN/OUT indications, and the function header aswell (cf https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/6_documenting_software/62_comments#6-2-1-only-use-c-style-comments-on-the-same-line-as-pre-processor-directives-and-in-doxygen-style-file-and-function-header-comment-blocks). There are other functions with missing IN/OUT parameters. > > +{ > > + ARM_SVC_ARGS SvcArgs; > > + EFI_STATUS Status; > > + > > + ZeroMem (&SvcArgs, sizeof (SvcArgs)); > > + > > + SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64; > > + SvcArgs.Arg1 = mStorageId; > > + 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_OUT_OF_RESOURCES; > > + 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_UNSUPPORTED Set attributes not supported > > + > > +**/ > > +STATIC > > +EFI_STATUS > > +OpTeeRpmbFvbSetAttributes ( > > + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, > > + IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes Parameters should be aligned. (I think this is valid at other places, like for OpTeeRpmbFvbGetPhysicalAddress()) > > + ) > > +{ > > + DEBUG ((DEBUG_ERROR, "FvbSetAttributes(0x%X) is not supported\n", > *Attributes)); > > + return EFI_UNSUPPORTED; > > +} > > + > > +/** > > + 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. > I think one new line should be enough. And would it be possible to align the return codes comments ? > + > > +**/ > > +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; > > + MEM_INSTANCE *Instance; > > + VOID *Base; > > + > > + Status = EFI_SUCCESS; > > + Instance = INSTANCE_FROM_FVB_THIS (This); > > + if (!Instance->Initialized) { > > + Status = Instance->Initialize (Instance); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + } > > + > > + 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; > > + VOID *Base; > > + > > + Instance = INSTANCE_FROM_FVB_THIS (This); > > + if (!Instance->Initialized) { > > + Status = Instance->Initialize (Instance); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + } > > + Base = (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize > + Offset; > > + Status = ReadWriteRpmb ( > > + SP_SVC_RPMB_WRITE, > > + (UINTN)Buffer, > > + *NumBytes, > > + (Lba * Instance->BlockSize) + Offset > > + ); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + > > + // Update the memory copy > > + CopyMem (Base, Buffer, *NumBytes); > > + > > + return Status; > > +} > > + > > +/** > > + 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 == NULL) { > > + 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 (EFI_ERROR (Status)) { > > + 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 Instance Address to copy flash contents to > > +**/ > > +STATIC > > +VOID > > +ReadEntireFlash ( > > + MEM_INSTANCE *Instance > > + ) > > +{ > > + UINTN ReadAddr; > > + UINTN StorageFtwWorkingSize; > > + UINTN StorageVariableSize; > > + UINTN StorageFtwSpareSize; > > + > > + StorageFtwWorkingSize = PcdGet32(PcdFlashNvStorageFtwWorkingSize); > > + StorageVariableSize = PcdGet32(PcdFlashNvStorageVariableSize); > > + 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 ( ReadWriteRpmb() returns an error code, I think we should return it aswell. > > + SP_SVC_RPMB_READ, > > + ReadAddr, > > + StorageVariableSize + StorageFtwWorkingSize + StorageFtwSpareSize, > > + 0 > > + ); > > +} > > + > > +/** > > + Validate the firmware volume header. > > + > > + @param FwVolHeader Pointer to the firmware volume header for the RPMB > > + > > + @retval EFI_SUCCESS The firmware volume header is correct > > + @retval EFI_NOT_FOUND No header present > > + @retval EFI_VOLUME_CORRUPTED The firmware volume header CRC was > invalid > > + or either one of GUID's, Signature > and header > > + length was invalid > > +**/ > > +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_VOLUME_CORRUPTED; > > + } > > + > > + // 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_VOLUME_CORRUPTED; > > + } > > + > > + VariableStoreHeader = (VARIABLE_STORE_HEADER*)((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_VOLUME_CORRUPTED; > > + } > > + > > + VariableStoreLength = PcdGet32 (PcdFlashNvStorageVariableSize) - > > + FwVolHeader->HeaderLength; > > + if (VariableStoreHeader->Size != VariableStoreLength) { > > + DEBUG ((DEBUG_INFO, "%a: Variable Store Length does not match\n", > > + __FUNCTION__)); > > + return EFI_VOLUME_CORRUPTED; > > + } > > + > > + return EFI_SUCCESS; > > + > > +} > > + > > +/** > > + Initialize Fvb and variable storage headers for the RPMB storage. > > + > > + @param Instance MEM_INSTANCE pointer describing the device and the > > + Firmware Block Protocol > > + > > + @retval EFI_SUCCESS read/write ok > > + > > + @retval EFI_UNSUPPORTED SVC to op-tee not supported > > + > > + @retval EFI_INVALID_PARAMETER SVC to op-tee had an > invalid param > > + > > + @retval EFI_ACCESS_DENIED SVC to op-tee was denied > > + > > + @retval EFI_OUT_OF_RESOURCES op-tee out of memory > > +**/ > > +STATIC > > +EFI_STATUS > > +InitializeFvAndVariableStoreHeaders ( > > + MEM_INSTANCE *Instance > > + ) > > +{ > > + EFI_FIRMWARE_VOLUME_HEADER *FirmwareVolumeHeader; > > + VARIABLE_STORE_HEADER *VariableStoreHeader; > > + EFI_STATUS Status; > > + UINTN HeadersLength; > > + VOID* Headers; > > + > > + HeadersLength = sizeof (EFI_FIRMWARE_VOLUME_HEADER) + > > + sizeof (EFI_FV_BLOCK_MAP_ENTRY) + > > + sizeof (VARIABLE_STORE_HEADER); > > + Headers = AllocateZeroPool (HeadersLength); > > + if (Headers == NULL) { > > + return EFI_OUT_OF_RESOURCES; > > + } > > + > > + // > > + // 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 = (VARIABLE_STORE_HEADER*)((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 (EFI_ERROR (Status)) { > > + goto Exit; > > + } > > + // Install the combined header in memory > > + CopyMem ((VOID*)Instance->MemBaseAddress, Headers, HeadersLength); > > + > > +Exit: > > + FreePool (Headers); > > + return Status; > > +} > > + > > +/** > > + Initialize the firmware block protocol for the specified memory. > > + > > + @param Instance MEM_INSTANCE pointer describing the device and the > > + Firmware Block Protocol > > + > > + @retval EFI_SUCCESS Initialized successfully or > already initialized > > + @retval EFI_UNSUPPORTED SVC to op-tee not supported > > + @retval EFI_INVALID_PARAMETER SVC to op-tee had an invalid param > > + @retval EFI_ACCESS_DENIED SVC to op-tee was denied > > + @retval EFI_OUT_OF_RESOURCES op-tee out of memory > > + > > + > > + I think one new line should be enough. > > +**/ > > +STATIC > > +EFI_STATUS > > +EFIAPI > > +FvbInitialize ( > > + MEM_INSTANCE *Instance > > + ) > > +{ > > + EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader; > > + EFI_STATUS Status; > > + > > + if (Instance->Initialized) { > > + return EFI_SUCCESS; > > + } > > + > > + // FirmwareVolumeHeader->FvLength is declared to have the Variable area > > + // AND the FTW working area AND the FTW Spare contiguous. > > + ASSERT ( > > + (PcdGet64 (PcdFlashNvStorageVariableBase64) + > > + PcdGet32 (PcdFlashNvStorageVariableSize)) == > > + PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) > > + ); > > + ASSERT ( > > + (PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) + > > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)) == > > + PcdGet64 (PcdFlashNvStorageFtwSpareBase64)); > > + > > + // 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 ((PcdGet64 (PcdFlashNvStorageVariableBase64) % > Instance->BlockSize) == 0); > > + ASSERT ((PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) % > Instance->BlockSize) == 0); > > + ASSERT ((PcdGet64 (PcdFlashNvStorageFtwSpareBase64) % > 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 (EFI_ERROR (Status)) { > > + 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 Status; > > +} > > + > > +/** > > + Since the RPMB is not byte addressable we need to allocate memory > > + and sync that on reads/writes. Initialize the memory and install the > > + Fvb protocol. > > + > > + @param ImageHandle The EFI image handle > > + @param SystemTable MM system table > > + > > + @retval EFI_SUCCESS Protocol installed > > + > > + @retval EFI_INVALID_PARAMETER Invalid Pcd values specified > > + > > + @retval EFI_OUT_OF_RESOURCES Can't allocate necessary memory > > +**/ > > +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); > > + if (Addr == NULL) { > > + ASSERT (0); > > + return EFI_OUT_OF_RESOURCES; > > + } > > + > > + 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; > > + > > + // The Pcd is user defined, so make sure we don't overflow > > + if (mInstance.MemBaseAddress > MAX_UINT64 - PcdGet32 > (PcdFlashNvStorageVariableSize)) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + if (mInstance.MemBaseAddress > MAX_UINT64 - PcdGet32 > (PcdFlashNvStorageVariableSize) - > > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + // Update the defined PCDs related to Variable Storage > > + PatchPcdSet64 (PcdFlashNvStorageVariableBase64, > mInstance.MemBaseAddress); > > + PatchPcdSet64 ( > > + PcdFlashNvStorageFtwWorkingBase64, > > + mInstance.MemBaseAddress + PcdGet32 (PcdFlashNvStorageVariableSize) > > + ); > > + PatchPcdSet64 ( > > + PcdFlashNvStorageFtwSpareBase64, > > + mInstance.MemBaseAddress + > > + PcdGet32 (PcdFlashNvStorageVariableSize) + > > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > > + ); It seems this part is really similar to what is FixupPcd.c:FixPcdMemory(), maybe this would be worth re-using it. > > + > > + 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__, PatchPcdGet64 (PcdFlashNvStorageVariableBase64))); > > + > > + return Status; > > +} > > diff --git a/Drivers/OpTeeRpmb/OpTeeRpmbFvb.h > b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.h > new file mode 100644 > index 000000000000..8305e776dbb6 > --- /dev/null > +++ b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.h > @@ -0,0 +1,52 @@ > +/** @file > > + > > + Copyright (c) 2020, Linaro Ltd. All rights reserved.<BR> > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > + > > +**/ > > + > > +#ifndef OPTEE_RPMB_FVB_H_ > > +#define OPTEE_RPMB_FVB_H_ > > + > > +/** > > + Those are not currently defined in any spec, it's an internal > > + contract between OP-TEE and EDK2. > > + For more details check core/arch/arm/include/kernel/stmm_sp.h in OP-TEE > > +**/ > > +#define SP_SVC_RPMB_READ 0xC4000066 > > +#define SP_SVC_RPMB_WRITE 0xC4000067 > > + > > +#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); > > + > > +/** > > + This struct is used by the RPMB driver. Since the upper EDK2 layers > > + expect byte addressable memory, we allocate a memory area of certain > > + size and sync it to the hardware on reads/writes. > > + > > + @param[in] Signature Internal signature used to discover the > instance > > + @param[in] Initialize Function used to initialize the instance > > + @param[in] Initialized Set to true if initialized > > + @param[in] FvbProtocol FVB protocol of the instance > > + @param[in] Handle Handle to install > > + @param[in] MemBaseAddress Physical address of the beggining of > the allocated memory > > + @param[in] BlockSize Blocksize > > + @param[in] NBlocks Number of allocated blocks I think that for doxygen, '@param[in]' is more for functions declarations. For structures, it should be as: /** * This struct is used by the RPMB driver. Since the upper EDK2 layers * expect byte addressable memory, we allocate a memory area of certain * size and sync it to the hardware on reads/writes. **/ struct _MEM_INSTANCE { /// Signature Internal signature used to discover the instance UINT32 Signature; /// Initialize Function used to initialize the instance MEM_INITIALIZE Initialize; [...] }; Cf: 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 > > +**/ > > +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 > > -- > 2.30.0 > > > > -=-=-=-=-=-= > Groups.io Links: You receive all messages sent to this group. > View/Reply Online (#71651): > https://edk2.groups.io/g/devel/message/71651 > <https://edk2.groups.io/g/devel/message/71651> > Mute This Topic: https://groups.io/mt/80588994/1821310 > <https://groups.io/mt/80588994/1821310> > Group Owner: devel+owner@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub > <https://edk2.groups.io/g/devel/unsub> [pierre.gondois@arm.com] > -=-=-=-=-=-= > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver 2021-03-03 10:20 ` [edk2-devel] " PierreGondois @ 2021-03-03 20:02 ` Ilias Apalodimas 2021-03-05 16:08 ` PierreGondois 0 siblings, 1 reply; 14+ messages in thread From: Ilias Apalodimas @ 2021-03-03 20:02 UTC (permalink / raw) To: Pierre; +Cc: devel, Sami.Mujawar Hi Pierre, On Wed, Mar 03, 2021 at 10:20:17AM +0000, Pierre wrote: > Hello Ilias, > > My review is mostly about coding style, but I would have some (inlined) > remarks about your patch, > > Regards, > > Pierre > > On 3/2/21 3:35 PM, Pierre Gondois wrote: > > > > > [...] > > + 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 > I think: is -> in Ok > > > > + > > > > + @retval EFI_SUCCESS Protocol was found and PCDs patched up > > > > + > > > > + **/ > > I think there should not be a space here. Ok > > > > > +EFI_STATUS > > > > +EFIAPI > > > > +FixPcdMemory ( > > [...] > > + > > > > + // Patch PCDs with the the correct values > I think it s > 'the the' -> the Yea > > > > + PatchPcdSet64 (PcdFlashNvStorageVariableBase64, > > Instance->MemBaseAddress); [...] > > + @retval EFI_OUT_OF_RESOURCES op-tee out of memory > > > > +**/ > > > > +STATIC > > > > +EFI_STATUS > > > > +ReadWriteRpmb ( > > > > + UINTN SvcAct, > > > > + UINTN Addr, > > > > + UINTN NumBytes, > > > > + UINTN Offset > > > > + ) > > I think the parameters should have IN/OUT indications, and the function > header aswell (cf https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/6_documenting_software/62_comments#6-2-1-only-use-c-style-comments-on-the-same-line-as-pre-processor-directives-and-in-doxygen-style-file-and-function-header-comment-blocks). > There are other functions with missing IN/OUT parameters. Sure, (did you c/p the wrong link?) > > > > > +{ > > [...] > > +OpTeeRpmbFvbSetAttributes ( > > > > + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, > > > > + IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes > Parameters should be aligned. (I think this is valid at other places, like > for OpTeeRpmbFvbGetPhysicalAddress()) Ok > > > > + ) [...] > > + 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. > > > I think one new line should be enough. And would it be possible to align the > return codes comments ? What's misaligned here? the parameters and the retval comments? This was pretty much copied from a different driver, so I assumed that was the 'right way'. I can of course align them. > > + > > > > +**/ > > > > +STATIC > > > > +EFI_STATUS > > > > > > + 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 ( > ReadWriteRpmb() returns an error code, I think we should return it aswell. There's a reason the return code is not checked here. This is only called in FvbInitialize(). That function will always run, even if the RPMB, the files and filesystem that OP-TEE creates haven't been created yet. In that case the OP-TEE API will return an 'error' which is 'file not found' though. You want to initialize properly with the FVB headers etc and continue with those contents. The code that follows does that, so if we checked the return code and exited early we would never be able to build the flash contents correctly to begin with. In any case after the read the headers are checked for validity/correctness and if an error is found the contents will be rebuilt. So we don't really care about the return here. Worst case scenario the flash is broken and we need to rebuild it. That's what the comment is trying to explain. Is there a better way to initialize the flash and the contents? Is there a callback in EDK2 to explicitly rebuild the flash if an error is found during the Fvbinitiaze phase? > > > > + SP_SVC_RPMB_READ, > > + @retval EFI_OUT_OF_RESOURCES op-tee out of memory [...] > > > > + > > > > + > > > > + > I think one new line should be enough. Ok > > > > [...] > > + mInstance.MemBaseAddress + > > > > + PcdGet32 (PcdFlashNvStorageVariableSize) + > > > > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > > > > + ); > It seems this part is really similar to what is FixupPcd.c:FixPcdMemory(), > maybe this would be worth re-using it. This is calculating the offsets of dynamic memory allocation that 'maps' to our RPMB and it is indeed identical. But since they run into completely different contexts I tried to avoid introducing dependencies between the 2 files. Is that a big problem? > > > > + > > [...] > > +/** > > > > + This struct is used by the RPMB driver. Since the upper EDK2 layers > > > > + expect byte addressable memory, we allocate a memory area of certain > > > > + size and sync it to the hardware on reads/writes. > > > > + > > > > + @param[in] Signature Internal signature used to discover the > > instance > > > > + @param[in] Initialize Function used to initialize the instance > > > > + @param[in] Initialized Set to true if initialized > > > > + @param[in] FvbProtocol FVB protocol of the instance > > > > + @param[in] Handle Handle to install > > > > + @param[in] MemBaseAddress Physical address of the beggining of the > > allocated memory > > > > + @param[in] BlockSize Blocksize > > > > + @param[in] NBlocks Number of allocated blocks > > I think that for doxygen, '@param[in]' is more for functions declarations. > For structures, it should be as: > > /** > * This struct is used by the RPMB driver. Since the upper EDK2 layers > * expect byte addressable memory, we allocate a memory area of certain > * size and sync it to the hardware on reads/writes. > **/ > struct _MEM_INSTANCE { > /// Signature Internal signature used to discover the instance > UINT32 Signature; > > /// Initialize Function used to initialize the instance > MEM_INITIALIZE Initialize; > > [...] > }; > > Cf: 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 > Ok Thanks /Ilias ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver 2021-03-03 20:02 ` Ilias Apalodimas @ 2021-03-05 16:08 ` PierreGondois 0 siblings, 0 replies; 14+ messages in thread From: PierreGondois @ 2021-03-05 16:08 UTC (permalink / raw) To: Ilias Apalodimas; +Cc: devel, Sami.Mujawar Hi Ilias, Thanks for the answers, I am re-reviewing the patch because I think I missed some things. I answered to your comments (inlined), Regards, Pierre On 3/3/21 8:02 PM, Ilias Apalodimas wrote: > Hi Pierre, > > On Wed, Mar 03, 2021 at 10:20:17AM +0000, Pierre wrote: >> Hello Ilias, >> >> My review is mostly about coding style, but I would have some (inlined) >> remarks about your patch, >> >> Regards, >> >> Pierre >> >> On 3/2/21 3:35 PM, Pierre Gondois wrote: >> >>> > [...] > >>> +� 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 >> I think: is -> in > Ok > >>> + >>> >>> +� @retval EFI_SUCCESS Protocol was found and PCDs patched up >>> >>> + >>> >>> + **/ >> I think there should not be a space here. > Ok > >>> +EFI_STATUS >>> >>> +EFIAPI >>> >>> +FixPcdMemory ( >>> > [...] > >>> + >>> >>> +� // Patch PCDs with the the correct values >> I think it s >> 'the the' -> the > Yea > >>> +� PatchPcdSet64 (PcdFlashNvStorageVariableBase64, >>> Instance->MemBaseAddress); > [...] > >>> +� @retval���������� EFI_OUT_OF_RESOURCES op-tee out of memory >>> >>> +**/ >>> >>> +STATIC >>> >>> +EFI_STATUS >>> >>> +ReadWriteRpmb ( >>> >>> +� UINTN� SvcAct, >>> >>> +� UINTN� Addr, >>> >>> +� UINTN� NumBytes, >>> >>> +� UINTN� Offset >>> >>> +� ) >> I think the parameters should have IN/OUT indications, and the function >> header aswell (cf https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/6_documenting_software/62_comments#6-2-1-only-use-c-style-comments-on-the-same-line-as-pre-processor-directives-and-in-doxygen-style-file-and-function-header-comment-blocks). >> There are other functions with missing IN/OUT parameters. > Sure, (did you c/p the wrong link?) Yes it seems so ... > >>> +{ >>> > [...] > >>> +OpTeeRpmbFvbSetAttributes ( >>> >>> +� IN CONST� EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, >>> >>> +� IN OUT��� EFI_FVB_ATTRIBUTES_2 *Attributes >> Parameters should be aligned. (I think this is valid at other places, like >> for OpTeeRpmbFvbGetPhysicalAddress()) > Ok > >>> +� ) > [...] > >>> +� 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. >>> >> I think one new line should be enough. And would it be possible to align the >> return codes comments ? > What's misaligned here? the parameters and the retval comments? > This was pretty much copied from a different driver, so I assumed that was the > 'right way'. I can of course align them. My comment did not belong to the right place I believe, I was referring to the '@retval' of OpTeeRpmbFvbErase(). The parameters of OpTeeRpmbFvbRead() do not appear as aligned though. > >>> + >>> >>> +**/ >>> >>> +STATIC >>> >>> +EFI_STATUS >>> >>> >>> +� 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 ( >> ReadWriteRpmb() returns an error code, I think we should return it aswell. > There's a reason the return code is not checked here. This is only called in > FvbInitialize(). That function will always run, even if the RPMB, the files > and filesystem that OP-TEE creates haven't been created yet. In that case > the OP-TEE API will return an 'error' which is 'file not found' though. > You want to initialize properly with the FVB headers etc and continue with > those contents. > > The code that follows does that, so if we checked the return code and exited > early we would never be able to build the flash contents correctly to begin > with. > In any case after the read the headers are checked for validity/correctness > and if an error is found the contents will be rebuilt. So we don't really care > about the return here. Worst case scenario the flash is broken and we need > to rebuild it. That's what the comment is trying to explain. > > Is there a better way to initialize the flash and the contents? > Is there a callback in EDK2 to explicitly rebuild the flash if an error is > found during the Fvbinitiaze phase? Thanks for the explanation. I am not aware of such mechanism. > >>> +��� SP_SVC_RPMB_READ, >>> +� @retval EFI_OUT_OF_RESOURCES����� op-tee out of memory > [...] > >>> + >>> >>> + >>> >>> + >> I think one new line should be enough. > Ok > >>> > [...] > >>> +��� mInstance.MemBaseAddress + >>> >>> +��� PcdGet32 (PcdFlashNvStorageVariableSize) + >>> >>> +��� PcdGet32 (PcdFlashNvStorageFtwWorkingSize) >>> >>> +��� ); >> It seems this part is really similar to what is FixupPcd.c:FixPcdMemory(), >> maybe this would be worth re-using it. > This is calculating the offsets of dynamic memory allocation that 'maps' to > our RPMB and it is indeed identical. But since they run into completely > different contexts I tried to avoid introducing dependencies between the 2 > files. Is that a big problem? This is not a big problem, but it seems the 'FixupPcd' and 'OpTeeRpmbFvb' modules are currently tied up by the definitions in 'OpTeeRpmbFvb.h'. If there is a chance for them to be separated in the future, dependencies should effectively avoided. Otherwise Iit might be better to factorize the code. > >>> + >>> > [...] > >>> +/** >>> >>> +� This struct is used by the RPMB driver. Since the upper EDK2 layers >>> >>> +� expect byte addressable memory, we allocate a memory area of certain >>> >>> +� size and sync it to the hardware on reads/writes. >>> >>> + >>> >>> +� @param[in] Signature������� Internal signature used to discover the >>> instance >>> >>> +� @param[in] Initialize������ Function used to initialize the instance >>> >>> +� @param[in] Initialized����� Set to true if initialized >>> >>> +� @param[in] FvbProtocol����� FVB protocol of the instance >>> >>> +� @param[in] Handle���������� Handle to install >>> >>> +� @param[in] MemBaseAddress�� Physical address of the beggining of the >>> allocated memory >>> >>> +� @param[in] BlockSize������� Blocksize >>> >>> +� @param[in] NBlocks��������� Number of allocated blocks >> I think that for doxygen, '@param[in]' is more for functions declarations. >> For structures, it should be as: >> >> /** >> � * This struct is used by the RPMB driver. Since the upper EDK2 layers >> � * expect byte addressable memory, we allocate a memory area of certain >> � * size and sync it to the hardware on reads/writes. >> **/ >> struct _MEM_INSTANCE { >> ��� ///� Signature������� Internal signature used to discover the instance >> ��� UINT32����������������������������� Signature; >> >> /// Initialize������ Function used to initialize the instance >> ��� MEM_INITIALIZE��������������������� Initialize; >> >> [...] >> }; >> >> Cf: 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 >> > Ok > > > Thanks > /Ilias ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver 2021-02-12 17:34 ` [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver Ilias Apalodimas 2021-03-03 10:20 ` [edk2-devel] " PierreGondois @ 2021-03-05 17:58 ` PierreGondois 2021-03-08 23:33 ` Ilias Apalodimas 1 sibling, 1 reply; 14+ messages in thread From: PierreGondois @ 2021-03-05 17:58 UTC (permalink / raw) To: devel, Ilias Apalodimas, Sami.Mujawar Hi Ilias, Here is the rest of the review. Sorry to do it in 2 times. Regards, Pierre > > +/** > > + 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 The error codes are missing. > > + > > + **/ > > +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); > > + // The Pcd is user defined, so make sure we don't overflow > > + if (Instance->MemBaseAddress > MAX_UINT64 - PcdGet32 > (PcdFlashNvStorageVariableSize)) { I think this can be removed since the next condition is more strict. > > + return EFI_INVALID_PARAMETER; > > + } > > + > > [...] > +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; If this is an FFA call, is it possible to: - put a reference in the header to the spec (it should be similar to the one at edk2/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c) - check the return status of the SVC call against the ones available at edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h - if possible, remove the dependency to <IndustryStandard/ArmMmSvc.h> > > + SvcArgs.Arg1 = mStorageId; > > + 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_OUT_OF_RESOURCES; > > + break; > > + > > + default: > > + Status = EFI_ACCESS_DENIED; > > + } > > + > > + return Status; > > +} > [...] > > +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) > > + ) could be on the same line -> ') {' > > + { > > + 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_VOLUME_CORRUPTED; > > + } > > + > > + // 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_VOLUME_CORRUPTED; > > + } > > + > > + VariableStoreHeader = (VARIABLE_STORE_HEADER*)((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_VOLUME_CORRUPTED; > > + } > > + > > + VariableStoreLength = PcdGet32 (PcdFlashNvStorageVariableSize) - > > + FwVolHeader->HeaderLength; > > + if (VariableStoreHeader->Size != VariableStoreLength) { > > + DEBUG ((DEBUG_INFO, "%a: Variable Store Length does not match\n", > > + __FUNCTION__)); > > + return EFI_VOLUME_CORRUPTED; > > + } > > + > > + return EFI_SUCCESS; > empty line, could be removed > + > > +} > > + > > [...] > > +**/ > > +STATIC > > +EFI_STATUS > > +EFIAPI > > +FvbInitialize ( > > + MEM_INSTANCE *Instance > > + ) > > +{ > > + EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader; > > + EFI_STATUS Status; > > + > > + if (Instance->Initialized) { > > + return EFI_SUCCESS; > > + } > > + > > + // FirmwareVolumeHeader->FvLength is declared to have the Variable area > > + // AND the FTW working area AND the FTW Spare contiguous. > > + ASSERT ( > > + (PcdGet64 (PcdFlashNvStorageVariableBase64) + > > + PcdGet32 (PcdFlashNvStorageVariableSize)) == > > + PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) > > + ); > > + ASSERT ( > > + (PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) + > > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)) == > > + PcdGet64 (PcdFlashNvStorageFtwSpareBase64)); > > + > > + // Check if the size of the area is at least one block size > > + ASSERT ( > > + (PcdGet32 (PcdFlashNvStorageVariableSize) > 0) && I think the first check (Size > 0) is redundant with the second one (Size > BlockSize). > > + (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 ((PcdGet64 (PcdFlashNvStorageVariableBase64) % > Instance->BlockSize) == 0); > > + ASSERT ((PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) % > Instance->BlockSize) == 0); > > + ASSERT ((PcdGet64 (PcdFlashNvStorageFtwSpareBase64) % > 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 (EFI_ERROR (Status)) { > > + 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 Status; > > +} > > + > > +/** > > + Since the RPMB is not byte addressable we need to allocate memory > > + and sync that on reads/writes. Initialize the memory and install the > > + Fvb protocol. > > + > > + @param ImageHandle The EFI image handle > > + @param SystemTable MM system table > > + > > + @retval EFI_SUCCESS Protocol installed > > + > > + @retval EFI_INVALID_PARAMETER Invalid Pcd values specified > > + > > + @retval EFI_OUT_OF_RESOURCES Can't allocate necessary memory > > +**/ > > +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); > > + if (Addr == NULL) { > > + ASSERT (0); > > + return EFI_OUT_OF_RESOURCES; > > + } > > + > > + SetMem (&mInstance, sizeof (mInstance), 0); NIT: you can use ZeroMem() > > + > > + 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; > > + > > + // The Pcd is user defined, so make sure we don't overflow > > + if (mInstance.MemBaseAddress > MAX_UINT64 - PcdGet32 > (PcdFlashNvStorageVariableSize)) { > I don't think this is necessary to do any check here since the memory has just been allocated with the right size. > + return EFI_INVALID_PARAMETER; > > + } > > + > > + if (mInstance.MemBaseAddress > MAX_UINT64 - PcdGet32 > (PcdFlashNvStorageVariableSize) - > > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + // Update the defined PCDs related to Variable Storage > > + PatchPcdSet64 (PcdFlashNvStorageVariableBase64, > mInstance.MemBaseAddress); > > + PatchPcdSet64 ( > > + PcdFlashNvStorageFtwWorkingBase64, > > + mInstance.MemBaseAddress + PcdGet32 (PcdFlashNvStorageVariableSize) > > + ); > > + PatchPcdSet64 ( > > + PcdFlashNvStorageFtwSpareBase64, > > + mInstance.MemBaseAddress + > > + PcdGet32 (PcdFlashNvStorageVariableSize) + > > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > > + ); Do we actually need to set these PCDs ? If the PCDs are persistent, we should not need to patch them in FixupPcd.c. FvbInitialize() is using them, but it is not called from OpTeeRpmbFvbInit(). > > + > > + 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__, PatchPcdGet64 (PcdFlashNvStorageVariableBase64))); > > + > > + return Status; > > +} ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver 2021-03-05 17:58 ` PierreGondois @ 2021-03-08 23:33 ` Ilias Apalodimas 0 siblings, 0 replies; 14+ messages in thread From: Ilias Apalodimas @ 2021-03-08 23:33 UTC (permalink / raw) To: Pierre; +Cc: devel, Sami Mujawar On Fri, Mar 05, 2021 at 05:58:49PM +0000, Pierre wrote: > Hi Ilias, > Here is the rest of the review. Sorry to do it in 2 times. No worries, I'll try to pick up all the comments. > > Regards, > > Pierre > > > > > > +/** > > > > + 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 > The error codes are missing. Yea, but I'll remove the overflow check on v6 so that should be fine as-is. > > > > + > > > > + **/ > > > > +EFI_STATUS > > > > +EFIAPI > > > > [...] > > + ASSERT_EFI_ERROR (Status); > > > > + > > > > + Instance = INSTANCE_FROM_FVB_THIS (FvbProtocol); > > > > + // The Pcd is user defined, so make sure we don't overflow > > > > + if (Instance->MemBaseAddress > MAX_UINT64 - PcdGet32 > > (PcdFlashNvStorageVariableSize)) { > I think this can be removed since the next condition is more strict. ditto > > > > + return EFI_INVALID_PARAMETER; > > > > + } > > > > + > > > > > [...] > > +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; > > If this is an FFA call, is it possible to: > - put a reference in the header to the spec (it should be similar to the > one at > edk2/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c) > - check the return status of the SVC call against the ones available at > edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h > - if possible, remove the dependency to <IndustryStandard/ArmMmSvc.h> > The call is technically an FFA one but at the moment OP-TEE returns the StMM return code which is defined in the last header you mention. The relevant code is in ./core/arch/arm/kernel/stmm_sp.c function tee2stmm_ret_val(). So unless we redefine that in OP-TEE or (better imho), wait for a full FFA mechanism to be in place, I'd prefer leaving it as is. Keep in mind that adding the full FFA will also get rid of the hardcoded IDs on the beginning of the file. > > > > + SvcArgs.Arg1 = mStorageId; > > + // [...] > > > > + if ( (FwVolHeader->Revision != EFI_FVH_REVISION) > > > > + || (FwVolHeader->Signature != EFI_FVH_SIGNATURE) > > > > + || (FwVolHeader->FvLength != FvLength) > > > > + ) > could be on the same line -> ') {' ok > > > > + { > > > > > > + if (VariableStoreHeader->Size != VariableStoreLength) { > > > > + DEBUG ((DEBUG_INFO, "%a: Variable Store Length does not match\n", > > > > + __FUNCTION__)); > > > > + return EFI_VOLUME_CORRUPTED; > > > > + } > > > > + > > > > + return EFI_SUCCESS; > > > empty line, could be removed ok > > + > > > > + (PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) + > > > > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)) == > > > > + PcdGet64 (PcdFlashNvStorageFtwSpareBase64)); > > > > + > > > > + // Check if the size of the area is at least one block size > > > > + ASSERT ( > > > > + (PcdGet32 (PcdFlashNvStorageVariableSize) > 0) && > I think the first check (Size > 0) is redundant with the second one (Size > > BlockSize). Yea it seems so. This was again a c/p from other drivers handling the PCD, but we can start clean here. > > > > + (PcdGet32 (PcdFlashNvStorageVariableSize) / Instance->BlockSize > 0) > > > > + ); > > > > + ASSERT ( > > > > + (PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > 0) && [...] > > + > > > > + SetMem (&mInstance, sizeof (mInstance), 0); > NIT: you can use ZeroMem() Sure > > > > + > > > > + 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; > > > > + > > > > + // The Pcd is user defined, so make sure we don't overflow > > > > + if (mInstance.MemBaseAddress > MAX_UINT64 - PcdGet32 > > (PcdFlashNvStorageVariableSize)) { > > > I don't think this is necessary to do any check here since the memory has > just been allocated with the right size. Yea it's not. I'll probably remove the same piece of code from the FixupPcd.c as well. > > > > + ); > Do we actually need to set these PCDs ? If the PCDs are persistent, we > should not need to patch them in FixupPcd.c. FvbInitialize() is using them, > but it is not called from OpTeeRpmbFvbInit(). > > I think it is. There's something 'special' about this driver. The upper EDK2 lays expect a byte-addressable interface. So the PCDs are definitely not persistent, they are patchable in module PCDs. Since RPMB isn't one we are allocating memory and handing that memory over to EDK2, while syncing the background on the hardware. Again, it's been a while since I wrote this and memory might be failing, but I think what's happening here is: OpTeeRpmbFvbInit is the entry point, which calls in EDK2 code, so you need to patch them in here, before calling anything. After that the fixup code runs before calling into the module and fixes up the values for us. In any case I'll double check before posting a v6 Thanks /Ilias [...] ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2 v5] StMMRpmb: Add support for building StandaloneMm image for OP-TEE 2021-02-12 17:34 [PATCH 0/2 v5] Add support for running StandaloneMm as OP-TEE TA Ilias Apalodimas 2021-02-12 17:34 ` [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver Ilias Apalodimas @ 2021-02-12 17:34 ` Ilias Apalodimas 2021-03-03 10:18 ` [edk2-devel] " PierreGondois 2021-03-03 11:32 ` [edk2-devel] [PATCH 0/2 v5] Add support for running StandaloneMm as OP-TEE TA Sami Mujawar 2 siblings, 1 reply; 14+ messages in thread From: Ilias Apalodimas @ 2021-02-12 17:34 UTC (permalink / raw) To: devel, sami.mujawar; +Cc: ardb+tianocore, sughosh.ganu, leif, Ilias Apalodimas 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/ Reviewed-by: Sami Mujawar <sami.mujawar@arm.com> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- Platform/StMMRpmb/PlatformStandaloneMm.dsc | 165 +++++++++++++++++++++ Platform/StMMRpmb/PlatformStandaloneMm.fdf | 111 ++++++++++++++ 2 files changed, 276 insertions(+) create mode 100644 Platform/StMMRpmb/PlatformStandaloneMm.dsc create mode 100644 Platform/StMMRpmb/PlatformStandaloneMm.fdf diff --git a/Platform/StMMRpmb/PlatformStandaloneMm.dsc b/Platform/StMMRpmb/PlatformStandaloneMm.dsc new file mode 100644 index 000000000000..1f1ddf968c1f --- /dev/null +++ b/Platform/StMMRpmb/PlatformStandaloneMm.dsc @@ -0,0 +1,165 @@ +# +# 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 + 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 + StandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntryPoint/StandaloneMmDriverEntryPoint.inf + + StandaloneMmMmuLib|ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.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.PcdFlashNvStorageVariableBase64|0x0 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64|0x0 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64|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 000000000000..febc6d0d959b --- /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.30.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH 2/2 v5] StMMRpmb: Add support for building StandaloneMm image for OP-TEE 2021-02-12 17:34 ` [PATCH 2/2 v5] StMMRpmb: Add support for building StandaloneMm image for OP-TEE Ilias Apalodimas @ 2021-03-03 10:18 ` PierreGondois 2021-03-03 19:24 ` Ilias Apalodimas 0 siblings, 1 reply; 14+ messages in thread From: PierreGondois @ 2021-03-03 10:18 UTC (permalink / raw) To: ilias.apalodimas, devel, Sami.Mujawar Hello Ilias, I would have some (inlined) remarks about your patch, Regards, Pierre On 3/2/21 3:35 PM, Pierre Gondois wrote: > > ------------------------------------------------------------------------ > *From:* devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Ilias > Apalodimas via groups.io <ilias.apalodimas=linaro.org@groups.io> > *Sent:* Friday, February 12, 2021 5:34 PM > *To:* devel@edk2.groups.io <devel@edk2.groups.io>; Sami Mujawar > <Sami.Mujawar@arm.com> > *Cc:* ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; > sughosh.ganu@linaro.org <sughosh.ganu@linaro.org>; leif@nuviainc.com > <leif@nuviainc.com>; Ilias Apalodimas <ilias.apalodimas@linaro.org> > *Subject:* [edk2-devel] [PATCH 2/2 v5] StMMRpmb: Add support for > building StandaloneMm image for OP-TEE > 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 > <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 > <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/ > <https://git.linaro.org/people/ilias.apalodimas/efi_optee_variables.git/> > > Reviewed-by: Sami Mujawar <sami.mujawar@arm.com> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > Platform/StMMRpmb/PlatformStandaloneMm.dsc | 165 +++++++++++++++++++++ > Platform/StMMRpmb/PlatformStandaloneMm.fdf | 111 ++++++++++++++ > 2 files changed, 276 insertions(+) > create mode 100644 Platform/StMMRpmb/PlatformStandaloneMm.dsc > create mode 100644 Platform/StMMRpmb/PlatformStandaloneMm.fdf > > diff --git a/Platform/StMMRpmb/PlatformStandaloneMm.dsc > b/Platform/StMMRpmb/PlatformStandaloneMm.dsc > new file mode 100644 > index 000000000000..1f1ddf968c1f > --- /dev/null > +++ b/Platform/StMMRpmb/PlatformStandaloneMm.dsc > @@ -0,0 +1,165 @@ > +# > > +# 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 I think we are at the revision 0x0001001C, cf https://edk2-docs.gitbook.io/edk-ii-dsc-specification/3_edk_ii_dsc_file_format/35_-defines-_section > > + 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 > > + > 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 > > + > StandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntryPoint/StandaloneMmDriverEntryPoint.inf > > + > > + > StandaloneMmMmuLib|ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.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 seems in the previous patch you are using some 'DEBUG ()' and 'ASSERT () statements. Wouldn't using BaseDebugLibNull and BaseSerialPortLibNull make them useless for DEBUG and RELEASE build ? > > + > > + # > > + # 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 > > +# > > +################################################################################ > Since this comment is for the PCD section, I think it would be better to remove the empty line after the comment and add one at the top of this comment. > + > > +[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.PcdFlashNvStorageVariableBase64|0x0 > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64|0x0 > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64|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 asOn 3/2/21 4:43 PM, Pierre wrote: > > +# 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 000000000000..febc6d0d959b > --- /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.30.0 > > > > -=-=-=-=-=-= > Groups.io Links: You receive all messages sent to this group. > View/Reply Online (#71652): > https://edk2.groups.io/g/devel/message/71652 > <https://edk2.groups.io/g/devel/message/71652> > Mute This Topic: https://groups.io/mt/80588995/1821310 > <https://groups.io/mt/80588995/1821310> > Group Owner: devel+owner@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub > <https://edk2.groups.io/g/devel/unsub> [pierre.gondois@arm.com] > -=-=-=-=-=-= > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH 2/2 v5] StMMRpmb: Add support for building StandaloneMm image for OP-TEE 2021-03-03 10:18 ` [edk2-devel] " PierreGondois @ 2021-03-03 19:24 ` Ilias Apalodimas 2021-03-05 18:07 ` PierreGondois 0 siblings, 1 reply; 14+ messages in thread From: Ilias Apalodimas @ 2021-03-03 19:24 UTC (permalink / raw) To: Pierre; +Cc: devel, Sami.Mujawar Hi Pierre, On Wed, Mar 03, 2021 at 10:18:57AM +0000, Pierre wrote: > Hello Ilias, > > I would have some (inlined) remarks about your patch, > > Regards, > > Pierre > > On 3/2/21 3:35 PM, Pierre Gondois wrote: > > + PLATFORM_NAME = MmStandaloneRpmb [...] > > > > + PLATFORM_GUID = A27A486E-D7B9-4D70-9F37-FED9ABE041A2 > > > > + PLATFORM_VERSION = 1.0 > > > > + DSC_SPECIFICATION = 0x00010011 > I think we are at the revision 0x0001001C, cf https://edk2-docs.gitbook.io/edk-ii-dsc-specification/3_edk_ii_dsc_file_format/35_-defines-_section > Probably, the paptch has been sitting in my trees for quite some time. I'll have a look and update it. > > > > + OUTPUT_DIRECTORY = Build/$(PLATFORM_NAME) > > > > + SUPPORTED_ARCHITECTURES = AARCH64 > > + StandaloneMmCoreEntryPoint|StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf [...] > > > > + StandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntryPoint/StandaloneMmDriverEntryPoint.inf > > > > + > > > > + StandaloneMmMmuLib|ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.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 seems in the previous patch you are using some 'DEBUG ()' and 'ASSERT () > statements. Wouldn't using BaseDebugLibNull and BaseSerialPortLibNull make > them useless for DEBUG and RELEASE build ? There's a reasson for that. So what happens right now is that OP-TEE creates (and maps) the device(s) StMM can access. In the current case you are correct, but noone prevents us from mapping the console and in the future have those ASSERT/DEBUG statements visible. So I figured it's worth keeping them in there even if they won't (currently) show up, since they offer some hints to code reading as well. In fact we do have patches that map the console and intend to upstream them into OP-TEE at some point (and this was used during development as well). > > > > + > > > > + # > > > > + # 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 > > > > +# > > > > +################################################################################ > > > Since this comment is for the PCD section, I think it would be better to > remove the empty line after the comment and add one at the top of this > comment. Sure [...] Thanks /Ilias ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH 2/2 v5] StMMRpmb: Add support for building StandaloneMm image for OP-TEE 2021-03-03 19:24 ` Ilias Apalodimas @ 2021-03-05 18:07 ` PierreGondois 2021-03-08 11:16 ` Ilias Apalodimas 0 siblings, 1 reply; 14+ messages in thread From: PierreGondois @ 2021-03-05 18:07 UTC (permalink / raw) To: Ilias Apalodimas, devel [-- Attachment #1: Type: text/plain, Size: 248 bytes --] Hi Ilias, Thanks for the answer. Is it necessary to have the 'COMPRESSION_TOOL_GUID' define ? I could not find any use of it. If this is coming from StandaloneMmPkg/StandaloneMmPkg.dsc we might want to remove it there aswel. Regards, Pierre [-- Attachment #2: Type: text/html, Size: 268 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH 2/2 v5] StMMRpmb: Add support for building StandaloneMm image for OP-TEE 2021-03-05 18:07 ` PierreGondois @ 2021-03-08 11:16 ` Ilias Apalodimas 0 siblings, 0 replies; 14+ messages in thread From: Ilias Apalodimas @ 2021-03-08 11:16 UTC (permalink / raw) To: PierreGondois; +Cc: devel On Fri, 5 Mar 2021 at 20:07, PierreGondois <pierre.gondois@arm.com> wrote: > > Hi Ilias, > Thanks for the answer. > Is it necessary to have the 'COMPRESSION_TOOL_GUID' define ? I could not find any use of it. If this is coming from StandaloneMmPkg/StandaloneMmPkg.dsc we might want to remove it there aswel. > Ok, I'll have a look and remove it > Regards, > Pierre ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH 0/2 v5] Add support for running StandaloneMm as OP-TEE TA 2021-02-12 17:34 [PATCH 0/2 v5] Add support for running StandaloneMm as OP-TEE TA Ilias Apalodimas 2021-02-12 17:34 ` [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver Ilias Apalodimas 2021-02-12 17:34 ` [PATCH 2/2 v5] StMMRpmb: Add support for building StandaloneMm image for OP-TEE Ilias Apalodimas @ 2021-03-03 11:32 ` Sami Mujawar 2021-03-08 15:57 ` Leif Lindholm 2 siblings, 1 reply; 14+ messages in thread From: Sami Mujawar @ 2021-03-03 11:32 UTC (permalink / raw) To: devel@edk2.groups.io, ilias.apalodimas@linaro.org Cc: ardb+tianocore@kernel.org, sughosh.ganu@linaro.org, leif@nuviainc.com, nd Hi Ard, Leif, This patch series is creating 2 new folders Platform/StMMRpmb & Drivers/OpTeeRpmb. - Should these be in Drivers\StandaloneMmRpmbPkg similar to Drivers\OptionRomPkg ? - Also, the maintainer.txt file would need updating accordingly. Any advice/suggestions about this, please. Regards, Sami Mujawar -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ilias Apalodimas via groups.io Sent: 12 February 2021 05:35 PM To: devel@edk2.groups.io; Sami Mujawar <Sami.Mujawar@arm.com> Cc: ardb+tianocore@kernel.org; sughosh.ganu@linaro.org; leif@nuviainc.com; Ilias Apalodimas <ilias.apalodimas@linaro.org> Subject: [edk2-devel] [PATCH 0/2 v5] Add support for running StandaloneMm as OP-TEE TA Hi, This is v5 of [1] Changes since V4: - More coding stule fixes proposed by Sami, which Ecc or Patchcheck didn't report. - Adding missing error handling in InitializeFvAndVariableStoreHeaders(). An allocation wasn't properly checked for success Changes since V3: - Coding style fixes proposed by Sami - Fixed all reported PatchCheck errors - Added overflow checks on the base aaddress allocated for EFI variables. The size of the partition is user defined (via Pcd's) and the memory layout and allocation address depends on OP-TEE. So let's make sure we won't overflow when calculating the 3 partitions needed for FTW - Switched some PcdGet/Set32 to 64 to accomodate 64-bit addressing - Removed some duplicate entries in Platform/StMMRpmb/PlatformStandaloneMm.dsc - Added reviewed-by tags on patch 2/2 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 [1] https://edk2.groups.io/g/devel/message/66483?p=,,,20,0,0,0::Created,,ilias+apalodimas,20,2,0,77703661 Ilias Apalodimas (2): Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver StMMRpmb: Add support for building StandaloneMm image for OP-TEE Drivers/OpTeeRpmb/FixupPcd.c | 89 ++ Drivers/OpTeeRpmb/FixupPcd.inf | 43 + Drivers/OpTeeRpmb/OpTeeRpmbFv.inf | 58 ++ Drivers/OpTeeRpmb/OpTeeRpmbFvb.c | 920 +++++++++++++++++++++ Drivers/OpTeeRpmb/OpTeeRpmbFvb.h | 52 ++ Platform/StMMRpmb/PlatformStandaloneMm.dsc | 165 ++++ Platform/StMMRpmb/PlatformStandaloneMm.fdf | 111 +++ 7 files changed, 1438 insertions(+) create mode 100644 Drivers/OpTeeRpmb/FixupPcd.c create mode 100644 Drivers/OpTeeRpmb/FixupPcd.inf create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFv.inf create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFvb.c create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFvb.h create mode 100644 Platform/StMMRpmb/PlatformStandaloneMm.dsc create mode 100644 Platform/StMMRpmb/PlatformStandaloneMm.fdf -- 2.30.0 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH 0/2 v5] Add support for running StandaloneMm as OP-TEE TA 2021-03-03 11:32 ` [edk2-devel] [PATCH 0/2 v5] Add support for running StandaloneMm as OP-TEE TA Sami Mujawar @ 2021-03-08 15:57 ` Leif Lindholm 0 siblings, 0 replies; 14+ messages in thread From: Leif Lindholm @ 2021-03-08 15:57 UTC (permalink / raw) To: Sami Mujawar Cc: devel@edk2.groups.io, ilias.apalodimas@linaro.org, ardb+tianocore@kernel.org, sughosh.ganu@linaro.org, nd Hi Sami, My £0.05 would be something like: - Drivers/OpTee/OpteeRpmbPkg - Platform/StandaloneMm/PlatformStandaloneMmPkg I think until we have more generic STMM solutions, it may be tricky to figure out the optimal layout, so if the yneed to change in future, that's fine. / Leif On Wed, Mar 03, 2021 at 11:32:53 +0000, Sami Mujawar wrote: > Hi Ard, Leif, > > This patch series is creating 2 new folders Platform/StMMRpmb & Drivers/OpTeeRpmb. > - Should these be in Drivers\StandaloneMmRpmbPkg similar to Drivers\OptionRomPkg ? > - Also, the maintainer.txt file would need updating accordingly. > > Any advice/suggestions about this, please. > > Regards, > > Sami Mujawar > > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ilias Apalodimas via groups.io > Sent: 12 February 2021 05:35 PM > To: devel@edk2.groups.io; Sami Mujawar <Sami.Mujawar@arm.com> > Cc: ardb+tianocore@kernel.org; sughosh.ganu@linaro.org; leif@nuviainc.com; Ilias Apalodimas <ilias.apalodimas@linaro.org> > Subject: [edk2-devel] [PATCH 0/2 v5] Add support for running StandaloneMm as OP-TEE TA > > Hi, > > This is v5 of [1] > > Changes since V4: > - More coding stule fixes proposed by Sami, which Ecc or Patchcheck didn't > report. > - Adding missing error handling in InitializeFvAndVariableStoreHeaders(). > An allocation wasn't properly checked for success > > Changes since V3: > - Coding style fixes proposed by Sami > - Fixed all reported PatchCheck errors > - Added overflow checks on the base aaddress allocated for EFI variables. > The size of the partition is user defined (via Pcd's) and the memory layout > and allocation address depends on OP-TEE. So let's make sure we won't overflow > when calculating the 3 partitions needed for FTW > - Switched some PcdGet/Set32 to 64 to accomodate 64-bit addressing > - Removed some duplicate entries in Platform/StMMRpmb/PlatformStandaloneMm.dsc > - Added reviewed-by tags on patch 2/2 > > 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 > > [1] https://edk2.groups.io/g/devel/message/66483?p=,,,20,0,0,0::Created,,ilias+apalodimas,20,2,0,77703661 > > Ilias Apalodimas (2): > Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver > StMMRpmb: Add support for building StandaloneMm image for OP-TEE > > Drivers/OpTeeRpmb/FixupPcd.c | 89 ++ > Drivers/OpTeeRpmb/FixupPcd.inf | 43 + > Drivers/OpTeeRpmb/OpTeeRpmbFv.inf | 58 ++ > Drivers/OpTeeRpmb/OpTeeRpmbFvb.c | 920 +++++++++++++++++++++ > Drivers/OpTeeRpmb/OpTeeRpmbFvb.h | 52 ++ > Platform/StMMRpmb/PlatformStandaloneMm.dsc | 165 ++++ > Platform/StMMRpmb/PlatformStandaloneMm.fdf | 111 +++ > 7 files changed, 1438 insertions(+) > create mode 100644 Drivers/OpTeeRpmb/FixupPcd.c > create mode 100644 Drivers/OpTeeRpmb/FixupPcd.inf > create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFv.inf > create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFvb.c > create mode 100644 Drivers/OpTeeRpmb/OpTeeRpmbFvb.h > create mode 100644 Platform/StMMRpmb/PlatformStandaloneMm.dsc > create mode 100644 Platform/StMMRpmb/PlatformStandaloneMm.fdf > > -- > 2.30.0 > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-03-08 23:34 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-02-12 17:34 [PATCH 0/2 v5] Add support for running StandaloneMm as OP-TEE TA Ilias Apalodimas 2021-02-12 17:34 ` [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver Ilias Apalodimas 2021-03-03 10:20 ` [edk2-devel] " PierreGondois 2021-03-03 20:02 ` Ilias Apalodimas 2021-03-05 16:08 ` PierreGondois 2021-03-05 17:58 ` PierreGondois 2021-03-08 23:33 ` Ilias Apalodimas 2021-02-12 17:34 ` [PATCH 2/2 v5] StMMRpmb: Add support for building StandaloneMm image for OP-TEE Ilias Apalodimas 2021-03-03 10:18 ` [edk2-devel] " PierreGondois 2021-03-03 19:24 ` Ilias Apalodimas 2021-03-05 18:07 ` PierreGondois 2021-03-08 11:16 ` Ilias Apalodimas 2021-03-03 11:32 ` [edk2-devel] [PATCH 0/2 v5] Add support for running StandaloneMm as OP-TEE TA Sami Mujawar 2021-03-08 15:57 ` Leif Lindholm
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox