[edk2-devel] [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver
Sami Mujawar
sami.mujawar at arm.com
Fri Jan 29 11:45:31 UTC 2021
Hi Ilias,
Please find my response inline marked [SAMI].
Regards,
Sami Mujawar
-----Original Message-----
From: Ilias Apalodimas <ilias.apalodimas at linaro.org>
Sent: 29 January 2021 08:02 AM
To: Sami Mujawar <Sami.Mujawar at arm.com>
Cc: Sughosh Ganu <sughosh.ganu at linaro.org>; devel at edk2.groups.io; Ard Biesheuvel <Ard.Biesheuvel at arm.com>; Leif Lindholm <leif at nuviainc.com>; Sahil Malhotra <sahil.malhotra at linaro.org>
Subject: Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver
Hi Sami,
Thanks for taking the time
On Wed, 27 Jan 2021 at 19:10, Sami Mujawar <Sami.Mujawar at arm.com> wrote:
>
> Hi Sughosh,
>
> There are some edk2 coding standard incompatibilities (like space between function name and opening bracket, function parameter alignment etc.) and documentation for some functions is missing in the patch. Can you fix these, please?
> Please also run the ECC tool in Drivers/OpTeeRpmb folder and fix any reported issues. The ECC errors 10002, 10006 and 10022 can be ignored for now.
>
> Other than that, please find my response inline marked [SAMI].
>
> Regards,
>
> Sami Mujawar
>
> -----Original Message-----
> From: Sughosh Ganu <sughosh.ganu at linaro.org>
> Sent: 16 December 2020 11:09 AM
> To: devel at edk2.groups.io
> Cc: Sami Mujawar <Sami.Mujawar at arm.com>; Ard Biesheuvel <Ard.Biesheuvel at arm.com>; Leif Lindholm <leif at nuviainc.com>; Sahil Malhotra <sahil.malhotra at linaro.org>; Ilias Apalodimas <ilias.apalodimas at linaro.org>
> Subject: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver
>
> From: Ilias Apalodimas <ilias.apalodimas at linaro.org>
>
> A following patch is adding support for building StMM in order to run it
> from OP-TEE.
> OP-TEE in combination with a NS-world supplicant can use the RPMB
> partition of an eMMC to store EFI variables. The supplicant
> functionality is currently available in U-Boot only but can be ported
> into EDK2. Assuming similar functionality is added in EDK2, this will
> allow any hardware with an RPMB partition to store EFI variables
> securely.
>
> So let's add a driver that enables access of the RPMB partition through
> OP-TEE. Since the upper layers expect a byte addressable interface,
> the driver allocates memory and patches the PCDs, while syncing the
> memory/hardware on read/write callbacks.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
[...]
> --- /dev/null
> +++ b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.h
> @@ -0,0 +1,35 @@
> +/** @file
> +
> + Copyright (c) 2020, Linaro Ltd. All rights reserved.<BR>
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef __OPTEE_RPMB_FV_
> +#define __OPTEE_RPMB_FV_
> [SAMI] This does not follow the edk2 coding standard. See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/53_include_files#5-3-5-all-include-file-contents-must-be-protected-by-a-include-guard
> [/SAMI]
Ok
> +
> +/* SVC Args */
> +#define SP_SVC_RPMB_READ 0xC4000066
> +#define SP_SVC_RPMB_WRITE 0xC4000067
> [SAMI] Is there a specification reference for this? If so, please add to the file header.
> See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-3-1-every-new-file-shall-begin-with-a-file-header-comment-block.
> [/SAMI]
No there isn't. Since those are FFA calls it's something we define
internally in OP-TEE.
We had a discussion with Arm about standardizing this in the future,
but it can wait until we get full FFA and SP support in OP-TEE.
The rpmb 'driver' is essentially a bunch of OP-TEE SVC calls that end
up using the OP-TEE APIs for accessing an RPMB partition.
I could add a reference to the OP-TEE code if that makes sense?
[SAMI] I think that may be a good reference point for now.
[/SAMI]
> +
> +#define FILENAME "EFI_VARS"
> +
> +#define FLASH_SIGNATURE SIGNATURE_32('r', 'p', 'm', 'b')
> +#define INSTANCE_FROM_FVB_THIS(a) CR(a, MEM_INSTANCE, FvbProtocol, \
> + FLASH_SIGNATURE)
> +
> +typedef struct _MEM_INSTANCE MEM_INSTANCE;
> +typedef EFI_STATUS (*MEM_INITIALIZE) (MEM_INSTANCE* Instance);
> +struct _MEM_INSTANCE
> +{
> + UINT32 Signature;
> + MEM_INITIALIZE Initialize;
> + BOOLEAN Initialized;
> + EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL FvbProtocol;
> + EFI_HANDLE Handle;
> + EFI_PHYSICAL_ADDRESS MemBaseAddress;
> + UINT16 BlockSize;
> + UINT16 NBlocks;
> +};
> [SAMI] Please add documentation for structure. See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/56_declarations_and_types#5-6-3-2-structure-declaration-with-forward-reference-or-self-reference
> [/SAMI]
Ok
> +
> +#endif
> diff --git a/Drivers/OpTeeRpmb/FixupPcd.c b/Drivers/OpTeeRpmb/FixupPcd.c
> new file mode 100644
[...]
> +
> + Instance = INSTANCE_FROM_FVB_THIS(FvbProtocol);
> + // Patch PCDs with the the correct values
> + PatchPcdSet32 (PcdFlashNvStorageVariableBase, Instance->MemBaseAddress);
> + PatchPcdSet32 (PcdFlashNvStorageFtwWorkingBase, Instance->MemBaseAddress +
> + PcdGet32 (PcdFlashNvStorageVariableSize));
> + PatchPcdSet32 (PcdFlashNvStorageFtwSpareBase, Instance->MemBaseAddress +
> + PcdGet32 (PcdFlashNvStorageVariableSize) +
> + PcdGet32 (PcdFlashNvStorageFtwWorkingSize));NorFlashDxe
> [SAMI] Should the 64-bit versions of the PCDs be used here.
> A recent change added similar support to the ArmPlatformPkg\Drivers\NorFlashDxe.
> [/SAMI]
Looking at the NorFlashDxe commit, this is needed for NOR flash
devices connected in a 64-bit address space.
In our case OP-TEE maps the memory StMM can use, so that depends on
the platform and how OP-TEE is layed out in memory.
I'll have a look on the current OP-TEE supported platforms and if
that's a case I'll change it.
> +
> + DEBUG ((DEBUG_INFO, "%a: Fixup PcdFlashNvStorageVariableBase: 0x%lx\n",
[...]
> +
> +#include "OpTeeRpmbFvb.h"
> +
> +static const UINT16 mem_mgr_id = 3U;
> +static const UINT16 storage_id = 4U;
> [SAMI] Please change to STATIC CONST and also update the variable names according to the edk2 coding standard. See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/4_naming_conventions/43_identifiers#4-3-3-2-any-variable-with-file-scope-or-better-shall-be-prefixed-by-an-m-or-g
> [/SAMI]
Ok
> +
> +STATIC MEM_INSTANCE mInstance;
> +
> +/**
> + Sends an SVC call to OP-TEE for reading/writing an RPMB partition
> +
> + @param SvcAct SVC ID for read/write
> + @param Addr Base address of the buffer. When reading contents will be
> + copied to that buffer after reading them from the device.
> + When writing, the buffer holds the contents we want to
> + write cwtoin the device
> + @param NumBytes Number of bytes to read/write
> + @param Offset Offset into the RPMB file
> +
> + @retval OP-TEE return code
> +**/
> +
> [SAMI] Remove blank line. Same at other places as well.
> [/SAMI]
Ok
> +STATIC
> +EFI_STATUS
> +ReadWriteRpmb (
> + UINTN SvcAct,
> + UINTN Addr,
> + UINTN NumBytes,
> + UINTN Offset
> + )
> +{
> + ARM_SVC_ARGS SvcArgs;
> + EFI_STATUS Status;
> +
> + ZeroMem (&SvcArgs, sizeof (SvcArgs));
> +
> + SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
> + SvcArgs.Arg1 = storage_id;
> + SvcArgs.Arg2 = 0;
> + SvcArgs.Arg3 = SvcAct;
> + SvcArgs.Arg4 = Addr;
> + SvcArgs.Arg5 = NumBytes;
> + SvcArgs.Arg6 = Offset;
> +
> + ArmCallSvc (&SvcArgs);
> + if (SvcArgs.Arg3) {
> + DEBUG ((DEBUG_ERROR, "%a: Svc Call 0x%08x Addr: 0x%08x len: 0x%x Offset: 0x%x failed with 0x%x\n",
> + __func__, SvcAct, Addr, NumBytes, Offset, SvcArgs.Arg3));
> + }
> +
> + switch (SvcArgs.Arg3) {
> + case ARM_SVC_SPM_RET_SUCCESS:
> + Status = EFI_SUCCESS;
> + break;
> +
> + case ARM_SVC_SPM_RET_NOT_SUPPORTED:
> + Status = EFI_UNSUPPORTED;
> + break;
> +
> + case ARM_SVC_SPM_RET_INVALID_PARAMS:
> + Status = EFI_INVALID_PARAMETER;
> + break;
> +
> + case ARM_SVC_SPM_RET_DENIED:
> + Status = EFI_ACCESS_DENIED;
> + break;
> +
> + case ARM_SVC_SPM_RET_NO_MEMORY:
> + Status = EFI_BAD_BUFFER_SIZE;
> + break;
> +
> + default:
> + Status = EFI_ACCESS_DENIED;
> + }
> [SAMI] Should the error handling here be updated similar to the FF-A StandaloneMmPkg patches?
> [/SAMI]
I actually picked up the error handling from the previous non-FFA code.
I'll check what's on Sughosh latest patches and fix it if there are
any differences.
Looking at it again EFI_BAD_BUFFER_SIZE can change to indicate out of
memory properly anyway.
> +
> + return Status;
> +}
[...]
> +STATIC
> +EFI_STATUS
> +OpTeeRpmbFvbRead (
> + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
> + IN EFI_LBA Lba,
> + IN UINTN Offset,
> + IN OUT UINTN *NumBytes,
> + IN OUT UINT8 *Buffer
> + )
> +{
> + EFI_STATUS Status = EFI_SUCCESS;
> + MEM_INSTANCE *Instance;
> + VOID *Base;
> +
> + Instance = INSTANCE_FROM_FVB_THIS(This);
> + if (Instance->Initialized == FALSE) {
> [SAMI] if (!Instance->Initialized) ?
> See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false
> [/SAMI]
Ok
> + Instance->Initialize (Instance);
> + }
> +
> + Base = (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize + Offset;
[...]
> + MEM_INSTANCE *Instance;
> + EFI_STATUS Status = EFI_SUCCESS;
> + VOID *Base;
> +
> + Instance = INSTANCE_FROM_FVB_THIS(This);
> + if (Instance->Initialized == FALSE) {
> [SAMI] if (!Instance->Initialized) ?
> See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false
> [/SAMI]
sure
> + Instance->Initialize (Instance);
> + }
> + Base = (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize + Offset;
> + Status = ReadWriteRpmb (SP_SVC_RPMB_WRITE, (UINTN) Buffer, *NumBytes,
> + Lba * Instance->BlockSize + Offset);
> + if (Status != EFI_SUCCESS) {
> [SAMI] if (EFI_ERROR (Status)) ?
> [/SAMI]
Ok
> + return Status;
> + }
[...]
> + if (!Buf) {
> + return EFI_DEVICE_ERROR;
> + }
> + SetMem64 (Buf, NumLba * Instance->BlockSize, ~0UL);
> + // Write the device
> + Status = ReadWriteRpmb (SP_SVC_RPMB_WRITE, (UINTN) Buf, NumBytes,
> + Start * Instance->BlockSize);
> + if (Status != EFI_SUCCESS) {
> [SAMI] if (EFI_ERROR (Status)) ?
> [/SAMI]
> + return Status;
Ok
> + }
> + // Update the in memory copy
> + SetMem64 (Base, NumLba * Instance->BlockSize, ~0UL);
> + FreePool (Buf);
> + }
> +
> + VA_END (Args);
> +
> + return EFI_SUCCESS;
> +}
> +
> +/**
> + Since we use a memory backed storage we need to restore the RPMB contents
> + into memory before we register the Fvb protocol.
> +
> + @param Instace Address to copy flash contents to
> +
> + @retval 0 on success, OP-TEE error on failure
> +**/
> +STATIC
> +VOID
> +ReadEntireFlash (
> + MEM_INSTANCE *Instance
> + )
> +{
> + UINTN ReadAddr;
> +
> + UINTN StorageFtwWorkingSize = PcdGet32(PcdFlashNvStorageFtwWorkingSize);
> + UINTN StorageVariableSize = PcdGet32(PcdFlashNvStorageVariableSize);
> + UINTN StorageFtwSpareSize = PcdGet32(PcdFlashNvStorageFtwSpareSize);
> +
> + ReadAddr = Instance->MemBaseAddress;
> + // There's no need to check if the read failed here. The upper EDK2 layers
> + // will initialize the flash correctly if the in-memory copy is wrong
> + ReadWriteRpmb(SP_SVC_RPMB_READ, ReadAddr, StorageVariableSize +
> + StorageFtwWorkingSize + StorageFtwSpareSize , 0);
> +}
> +
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +ValidateFvHeader (
> + IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader
> + )
> +{
> + UINT16 Checksum;
> + VARIABLE_STORE_HEADER *VariableStoreHeader;
> + UINTN VariableStoreLength;
> + UINTN FvLength;
> +
> + FvLength = PcdGet32(PcdFlashNvStorageVariableSize) +
> + PcdGet32(PcdFlashNvStorageFtwWorkingSize) +
> + PcdGet32(PcdFlashNvStorageFtwSpareSize);
> +
> + // Verify the header revision, header signature, length
> + // Length of FvBlock cannot be 2**64-1
> + // HeaderLength cannot be an odd number
> + //
> + if ( (FwVolHeader->Revision != EFI_FVH_REVISION)
> + || (FwVolHeader->Signature != EFI_FVH_SIGNATURE)
> + || (FwVolHeader->FvLength != FvLength)
> + )
> + {
> + DEBUG ((DEBUG_INFO, "%a: No Firmware Volume header present\n",
> + __FUNCTION__));
> + return EFI_NOT_FOUND;
> + }
> +
> + // Check the Firmware Volume Guid
> + if (!CompareGuid (&FwVolHeader->FileSystemGuid, &gEfiSystemNvDataFvGuid)) {
> [SAMI] Not a comment for this patch but I noticed that BaseTools has an implementation of CompareGuid() that returns INTN.
> I wonder if that is intentional. Moreover, it also appears that the CompareGuid() usage in BaseTools does not consistently follow the explicit comparison rule (https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false).
> [/SAMI]
> + DEBUG ((DEBUG_INFO, "%a: Firmware Volume Guid non-compatible\n",
> + __FUNCTION__));
> + return EFI_NOT_FOUND;
> + }
> +
> + // Verify the header checksum
> + Checksum = CalculateSum16((UINT16*)FwVolHeader, FwVolHeader->HeaderLength);
> + if (Checksum != 0) {
> + DEBUG ((DEBUG_INFO, "%a: FV checksum is invalid (Checksum:0x%X)\n",
> + __FUNCTION__, Checksum));
> + return EFI_NOT_FOUND;
> [SAMI] Minor: By this point we should be fairly certain that this is a Firmware Volume header.
> So, should the error code returned here be EFI_VOLUME_CORRUPTED? Would the same apply to the remaining checks below?
> [/SAMI]
I think that makes sense, but I'll have to check if there's a
difference in behavior of the upper layers depending on the return
value.
The relevant Volume Header checks is a c/p from
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c and I kept it for
consistency.
Should we change the other driver as well at some point?
[SAMI] Looking at the NorFlashDxe code, there may be more needed. But I think it will be a good improvement.
[/SAMI]
> + }
> +
> + VariableStoreHeader = (VOID *)((UINTN)FwVolHeader +
> [SAMI] Should the typecast be (VARIABLE_STORE_HEADER*) instead of (VOID *)?
> [/SAMI]
Yes
> + FwVolHeader->HeaderLength);
> +
> + // Check the Variable Store Guid
[...]
> + FirmwareVolumeHeader->Revision = EFI_FVH_REVISION;
> + FirmwareVolumeHeader->BlockMap[0].NumBlocks = Instance->NBlocks;
> + FirmwareVolumeHeader->BlockMap[0].Length = Instance->BlockSize;
> + FirmwareVolumeHeader->BlockMap[1].NumBlocks = 0;
> + FirmwareVolumeHeader->BlockMap[1].Length = 0;
> + FirmwareVolumeHeader->Checksum = CalculateCheckSum16 (
> + (UINT16*)FirmwareVolumeHeader,
> + FirmwareVolumeHeader->HeaderLength);
> +
> + //
> + // VARIABLE_STORE_HEADER
> + //
> + VariableStoreHeader = (VOID *)((UINTN)Headers +
> [SAMI] Should the typecase be (VARIABLE_STORE_HEADER*) instead of (VOID *)?
> [/SAMI]
Yes
> + FirmwareVolumeHeader->HeaderLength);
> + CopyGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid);
> + VariableStoreHeader->Size = PcdGet32(PcdFlashNvStorageVariableSize) -
> + FirmwareVolumeHeader->HeaderLength;
> + VariableStoreHeader->Format = VARIABLE_STORE_FORMATTED;
> + VariableStoreHeader->State = VARIABLE_STORE_HEALTHY;
> +
> + Status = ReadWriteRpmb(SP_SVC_RPMB_WRITE, (UINTN) Headers, HeadersLength, 0);
> + if (Status != EFI_SUCCESS) {
> [SAMI] if (EFI_ERROR (Status)) ?
> [/SAMI]
Ok
> + goto Exit;
> + }
> + // Install the combined header in memory
> + CopyMem ((VOID*) Instance->MemBaseAddress, Headers, HeadersLength);
> +
> +Exit:
> + FreePool (Headers);
> + return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +FvbInitialize (
> + MEM_INSTANCE *Instance
> + )
> +{
> + EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader;
> + EFI_STATUS Status;
> +
> + if (Instance->Initialized == TRUE) {
> [SAMI] if (Instance->Initialized) ?
> See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false
> [/SAMI]
Sure
> + return EFI_SUCCESS;
> + }
> +
> + // FirmwareVolumeHeader->FvLength is declared to have the Variable area
> + // AND the FTW working area AND the FTW Spare contiguous.
> + ASSERT (PcdGet32 (PcdFlashNvStorageVariableBase) +
> + PcdGet32 (PcdFlashNvStorageVariableSize) ==
> + PcdGet32 (PcdFlashNvStorageFtwWorkingBase));
> + ASSERT (PcdGet32 (PcdFlashNvStorageFtwWorkingBase) +
> + PcdGet32 (PcdFlashNvStorageFtwWorkingSize) ==
> + PcdGet32 (PcdFlashNvStorageFtwSpareBase));
> +
> + // Check if the size of the area is at least one block size
> + ASSERT ((PcdGet32 (PcdFlashNvStorageVariableSize) > 0) &&
> + (PcdGet32 (PcdFlashNvStorageVariableSize) / Instance->BlockSize > 0));
> + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > 0) &&
> + (PcdGet32 (PcdFlashNvStorageFtwWorkingSize) / Instance->BlockSize > 0));
> + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwSpareSize) > 0) &&
> + (PcdGet32 (PcdFlashNvStorageFtwSpareSize) / Instance->BlockSize > 0));
> +
> + // Ensure the Variable areas are aligned on block size boundaries
> + ASSERT ((PcdGet32 (PcdFlashNvStorageVariableBase) % Instance->BlockSize) == 0);
> + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwWorkingBase) % Instance->BlockSize) == 0);
> + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwSpareBase) % Instance->BlockSize) == 0);
> [SAMI] These asserts may need to be adjusted if 64-bit versions of the PCDs are used.
> [/SAMI]
Noted
> +
> + // Read the file from disk and copy it to memory
> + ReadEntireFlash (Instance);
> +
> + FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *) Instance->MemBaseAddress;
> + Status = ValidateFvHeader(FwVolHeader);
> + if (EFI_ERROR (Status)) {
> + // There is no valid header, so time to install one.
> + DEBUG ((DEBUG_INFO, "%a: The FVB Header is not valid.\n", __FUNCTION__));
> +
> + // Reset memory
> + SetMem64 ((VOID *)Instance->MemBaseAddress, Instance->NBlocks * Instance->BlockSize, ~0UL);
> + DEBUG ((DEBUG_INFO, "%a: Erasing Flash.\n", __FUNCTION__));
> + Status = ReadWriteRpmb(SP_SVC_RPMB_WRITE, Instance->MemBaseAddress,
> + PcdGet32(PcdFlashNvStorageVariableSize) +
> + PcdGet32(PcdFlashNvStorageFtwWorkingSize) +
> + PcdGet32(PcdFlashNvStorageFtwSpareSize), 0);
> + if (Status != EFI_SUCCESS) {
> [SAMI] if (EFI_ERROR (Status)) ?
> [/SAMI]
Sure
> + return Status;
> + }
> + // Install all appropriate headers
> + DEBUG ((DEBUG_INFO, "%a: Installing a correct one for this volume.\n",
> + __FUNCTION__));
> + Status = InitializeFvAndVariableStoreHeaders (Instance);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + } else {
> + DEBUG ((DEBUG_INFO, "%a: Found valid FVB Header.\n", __FUNCTION__));
> + }
> + Instance->Initialized = TRUE;
> +
> + return EFI_SUCCESS;
> [SAMI] return Status; ?
> [/SAMI]
> +}
Sure
> +
> +EFI_STATUS
> +EFIAPI
> +OpTeeRpmbFvbInit (
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_MM_SYSTEM_TABLE *SystemTable
> + )
> +{
> + EFI_STATUS Status;
> + VOID *Addr;
> + UINTN FvLength;
> + UINTN NBlocks;
> +
> + FvLength = PcdGet32(PcdFlashNvStorageVariableSize) +
> + PcdGet32(PcdFlashNvStorageFtwWorkingSize) +
> + PcdGet32(PcdFlashNvStorageFtwSpareSize);
> +
> + NBlocks = EFI_SIZE_TO_PAGES(ALIGN_VARIABLE(FvLength));
> + Addr = AllocatePages(NBlocks);
> + ASSERT (Addr != NULL);
> [SAMI] Asserts will vanish in release builds and a failure to allocate memory would result in incorrect results.
> Please handle this error and return EFI_OUT_OF_RESOURCES.
> [/SAMI]
Fair enough
> +
> + SetMem (&mInstance, sizeof (mInstance), 0);
> +
> + mInstance.FvbProtocol.GetPhysicalAddress = OpTeeRpmbFvbGetPhysicalAddress;
> + mInstance.FvbProtocol.GetAttributes = OpTeeRpmbFvbGetAttributes;
> + mInstance.FvbProtocol.SetAttributes = OpTeeRpmbFvbSetAttributes;
> + mInstance.FvbProtocol.GetBlockSize = OpTeeRpmbFvbGetBlockSize;
> + mInstance.FvbProtocol.EraseBlocks = OpTeeRpmbFvbErase;
> + mInstance.FvbProtocol.Write = OpTeeRpmbFvbWrite;
> + mInstance.FvbProtocol.Read = OpTeeRpmbFvbRead;
> +
> + mInstance.MemBaseAddress = (EFI_PHYSICAL_ADDRESS) Addr;
> + mInstance.Signature = FLASH_SIGNATURE;
> + mInstance.Initialize = FvbInitialize;
> + mInstance.BlockSize = EFI_PAGE_SIZE;
> + mInstance.NBlocks = NBlocks;
> +
> + // Update the defined PCDs related to Variable Storage
> + PatchPcdSet32 (PcdFlashNvStorageVariableBase, mInstance.MemBaseAddress);
> [SAMI] Should the 64-bit versions of the PCDs be used here.
> A recent change added similar support to the ArmPlatformPkg\Drivers\NorFlashDxe.
> [/SAMI]
Noted
> + PatchPcdSet32 (PcdFlashNvStorageFtwWorkingBase, mInstance.MemBaseAddress +
> + PcdGet32 (PcdFlashNvStorageVariableSize));
> + PatchPcdSet32 (PcdFlashNvStorageFtwSpareBase, mInstance.MemBaseAddress +
> + PcdGet32 (PcdFlashNvStorageVariableSize) +
> + PcdGet32 (PcdFlashNvStorageFtwWorkingSize));
> +
> + Status = gMmst->MmInstallProtocolInterface (
> + &mInstance.Handle,
> + &gEfiSmmFirmwareVolumeBlockProtocolGuid,
> + EFI_NATIVE_INTERFACE,
> + &mInstance.FvbProtocol
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> + DEBUG ((DEBUG_INFO, "%a: Register OP-TEE RPMB Fvb\n", __FUNCTION__));
> + DEBUG ((DEBUG_INFO, "%a: Using NV store FV in-memory copy at 0x%lx\n",
> + __FUNCTION__, PatchPcdGet32 (PcdFlashNvStorageVariableBase)));
> +
> + return Status;
> +}
> --
> 2.17.1
>
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70908): https://edk2.groups.io/g/devel/message/70908
Mute This Topic: https://groups.io/mt/78998101/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list