Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver

["Ilias Apalodimas" <ilias.apalodimas@linaro.org>]

Hi Sami,

Thanks for taking the time

On Wed, 27 Jan 2021 at 19:10, Sami Mujawar <Sami.Mujawar@arm.com> wrote:
>
> Hi Sughosh,
>
> There are some edk2 coding standard incompatibilities (like space between function name and opening bracket, function parameter alignment etc.) and documentation for some functions is missing in the patch. Can you fix these, please?
> Please also run the ECC tool in Drivers/OpTeeRpmb folder and fix any reported issues. The ECC errors 10002, 10006 and 10022 can be ignored for now.
>
> Other than that, please find my response inline marked [SAMI].
>
> Regards,
>
> Sami Mujawar
>
> -----Original Message-----
> From: Sughosh Ganu <sughosh.ganu@linaro.org>
> Sent: 16 December 2020 11:09 AM
> To: devel@edk2.groups.io
> Cc: Sami Mujawar <Sami.Mujawar@arm.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Leif Lindholm <leif@nuviainc.com>; Sahil Malhotra <sahil.malhotra@linaro.org>; Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Subject: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver
>
> From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>
> A following patch is adding support for building StMM in order to run it
> from OP-TEE.
> OP-TEE in combination with a NS-world supplicant can use the RPMB
> partition of an eMMC to store EFI variables. The supplicant
> functionality is currently available in U-Boot only but can be ported
> into EDK2. Assuming similar functionality is added in EDK2, this will
> allow any hardware with an RPMB partition to store EFI variables
> securely.
>
> So let's add a driver that enables access of the RPMB partition through
> OP-TEE. Since the upper layers expect a byte addressable interface,
> the driver allocates memory and patches the PCDs, while syncing the
> memory/hardware on read/write callbacks.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[...]
> --- /dev/null
> +++ b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.h
> @@ -0,0 +1,35 @@
> +/** @file
> +
> +  Copyright (c) 2020, Linaro Ltd. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef __OPTEE_RPMB_FV_
> +#define __OPTEE_RPMB_FV_
> [SAMI] This does not follow the edk2 coding standard.  See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/53_include_files#5-3-5-all-include-file-contents-must-be-protected-by-a-include-guard
> [/SAMI]

Ok

> +
> +/* SVC Args */
> +#define SP_SVC_RPMB_READ                0xC4000066
> +#define SP_SVC_RPMB_WRITE               0xC4000067
> [SAMI] Is there a specification reference for this? If so, please add to the file header.
> See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-3-1-every-new-file-shall-begin-with-a-file-header-comment-block.
> [/SAMI]

No there isn't. Since those are FFA calls it's something we define
internally in OP-TEE.
We had a discussion with Arm about standardizing this in the future,
but it can wait until we get full FFA and SP support in OP-TEE.
The rpmb 'driver' is essentially a bunch of OP-TEE SVC calls that end
up using the OP-TEE APIs for accessing an RPMB partition.
I could add a reference to the OP-TEE code if that makes sense?


> +
> +#define FILENAME "EFI_VARS"
> +
> +#define FLASH_SIGNATURE            SIGNATURE_32('r', 'p', 'm', 'b')
> +#define INSTANCE_FROM_FVB_THIS(a)  CR(a, MEM_INSTANCE, FvbProtocol, \
> +                                      FLASH_SIGNATURE)
> +
> +typedef struct _MEM_INSTANCE         MEM_INSTANCE;
> +typedef EFI_STATUS (*MEM_INITIALIZE) (MEM_INSTANCE* Instance);
> +struct _MEM_INSTANCE
> +{
> +    UINT32                              Signature;
> +    MEM_INITIALIZE                      Initialize;
> +    BOOLEAN                             Initialized;
> +    EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL  FvbProtocol;
> +    EFI_HANDLE                          Handle;
> +    EFI_PHYSICAL_ADDRESS                MemBaseAddress;
> +    UINT16                              BlockSize;
> +    UINT16                              NBlocks;
> +};
> [SAMI] Please add documentation for structure. See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/56_declarations_and_types#5-6-3-2-structure-declaration-with-forward-reference-or-self-reference
> [/SAMI]

Ok

> +
> +#endif
> diff --git a/Drivers/OpTeeRpmb/FixupPcd.c b/Drivers/OpTeeRpmb/FixupPcd.c
> new file mode 100644

[...]

> +
> +  Instance = INSTANCE_FROM_FVB_THIS(FvbProtocol);
> +  // Patch PCDs with the the correct values
> +  PatchPcdSet32 (PcdFlashNvStorageVariableBase, Instance->MemBaseAddress);
> +  PatchPcdSet32 (PcdFlashNvStorageFtwWorkingBase, Instance->MemBaseAddress +
> +    PcdGet32 (PcdFlashNvStorageVariableSize));
> +  PatchPcdSet32 (PcdFlashNvStorageFtwSpareBase, Instance->MemBaseAddress +
> +    PcdGet32 (PcdFlashNvStorageVariableSize) +
> +    PcdGet32 (PcdFlashNvStorageFtwWorkingSize));NorFlashDxe
> [SAMI] Should the 64-bit versions of the PCDs be used here.
> A recent change added similar support to the ArmPlatformPkg\Drivers\NorFlashDxe.
> [/SAMI]

Looking at the NorFlashDxe commit, this is needed for NOR flash
devices connected in a 64-bit address space.
In our case OP-TEE maps the memory StMM can use, so that depends on
the platform and how OP-TEE is layed out in memory.
I'll have a look on the current OP-TEE supported platforms and if
that's a case I'll change it.

> +
> +  DEBUG ((DEBUG_INFO, "%a: Fixup PcdFlashNvStorageVariableBase: 0x%lx\n",

[...]

> +
> +#include "OpTeeRpmbFvb.h"
> +
> +static const UINT16 mem_mgr_id = 3U;
> +static const UINT16 storage_id = 4U;
> [SAMI] Please change to STATIC CONST and also update the variable names according to the edk2 coding standard. See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/4_naming_conventions/43_identifiers#4-3-3-2-any-variable-with-file-scope-or-better-shall-be-prefixed-by-an-m-or-g
> [/SAMI]

Ok

> +
> +STATIC MEM_INSTANCE mInstance;
> +
> +/**
> +  Sends an SVC call to OP-TEE for reading/writing an RPMB partition
> +
> +  @param SvcAct     SVC ID for read/write
> +  @param Addr       Base address of the buffer. When reading contents will be
> +                    copied to that buffer after reading them from the device.
> +                    When writing, the buffer holds the contents we want to
> +                    write cwtoin the device
> +  @param NumBytes   Number of bytes to read/write
> +  @param Offset     Offset into the RPMB file
> +
> +  @retval           OP-TEE return code
> +**/
> +
> [SAMI] Remove blank line. Same at other places as well.
> [/SAMI]

Ok

> +STATIC
> +EFI_STATUS
> +ReadWriteRpmb (
> +  UINTN  SvcAct,
> +  UINTN  Addr,
> +  UINTN  NumBytes,
> +  UINTN  Offset
> +  )
> +{
> +  ARM_SVC_ARGS  SvcArgs;
> +  EFI_STATUS    Status;
> +
> +  ZeroMem (&SvcArgs, sizeof (SvcArgs));
> +
> +  SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
> +  SvcArgs.Arg1 = storage_id;
> +  SvcArgs.Arg2 = 0;
> +  SvcArgs.Arg3 = SvcAct;
> +  SvcArgs.Arg4 = Addr;
> +  SvcArgs.Arg5 = NumBytes;
> +  SvcArgs.Arg6 = Offset;
> +
> +  ArmCallSvc (&SvcArgs);
> +  if (SvcArgs.Arg3) {
> +    DEBUG ((DEBUG_ERROR, "%a: Svc Call 0x%08x Addr: 0x%08x len: 0x%x Offset: 0x%x failed with 0x%x\n",
> +     __func__, SvcAct, Addr, NumBytes, Offset, SvcArgs.Arg3));
> +  }
> +
> +  switch (SvcArgs.Arg3) {
> +  case ARM_SVC_SPM_RET_SUCCESS:
> +    Status = EFI_SUCCESS;
> +    break;
> +
> +  case ARM_SVC_SPM_RET_NOT_SUPPORTED:
> +    Status = EFI_UNSUPPORTED;
> +    break;
> +
> +  case ARM_SVC_SPM_RET_INVALID_PARAMS:
> +    Status = EFI_INVALID_PARAMETER;
> +    break;
> +
> +  case ARM_SVC_SPM_RET_DENIED:
> +    Status = EFI_ACCESS_DENIED;
> +    break;
> +
> +  case ARM_SVC_SPM_RET_NO_MEMORY:
> +    Status = EFI_BAD_BUFFER_SIZE;
> +    break;
> +
> +  default:
> +    Status = EFI_ACCESS_DENIED;
> +  }
> [SAMI] Should the error handling here be updated similar to the FF-A StandaloneMmPkg patches?
> [/SAMI]

I actually picked up the error handling from the previous non-FFA code.
I'll check what's on Sughosh latest patches and fix it if there are
any differences.
Looking at it again EFI_BAD_BUFFER_SIZE can change to indicate out of
memory properly anyway.

> +
> +  return Status;
> +}

[...]

> +STATIC
> +EFI_STATUS
> +OpTeeRpmbFvbRead (
> +  IN CONST  EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
> +  IN        EFI_LBA                             Lba,
> +  IN        UINTN                               Offset,
> +  IN OUT    UINTN                               *NumBytes,
> +  IN OUT    UINT8                               *Buffer
> +  )
> +{
> +  EFI_STATUS   Status = EFI_SUCCESS;
> +  MEM_INSTANCE *Instance;
> +  VOID         *Base;
> +
> +  Instance = INSTANCE_FROM_FVB_THIS(This);
> +  if (Instance->Initialized == FALSE) {
> [SAMI] if (!Instance->Initialized)  ?
> See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false
> [/SAMI]

Ok

> +    Instance->Initialize (Instance);
> +  }
> +
> +  Base = (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize + Offset;

[...]

> +  MEM_INSTANCE *Instance;
> +  EFI_STATUS   Status = EFI_SUCCESS;
> +  VOID         *Base;
> +
> +  Instance = INSTANCE_FROM_FVB_THIS(This);
> +  if (Instance->Initialized == FALSE) {
> [SAMI] if (!Instance->Initialized)  ?
> See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false
> [/SAMI]

sure

> +    Instance->Initialize (Instance);
> +  }
> +  Base = (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize + Offset;
> +  Status = ReadWriteRpmb (SP_SVC_RPMB_WRITE, (UINTN) Buffer, *NumBytes,
> +    Lba * Instance->BlockSize + Offset);
> +  if (Status != EFI_SUCCESS) {
> [SAMI] if (EFI_ERROR (Status))  ?
> [/SAMI]

Ok

> +    return Status;
> +  }

[...]

> +    if (!Buf) {
> +      return EFI_DEVICE_ERROR;
> +    }
> +    SetMem64 (Buf, NumLba * Instance->BlockSize, ~0UL);
> +    // Write the device
> +    Status = ReadWriteRpmb (SP_SVC_RPMB_WRITE, (UINTN) Buf, NumBytes,
> +     Start * Instance->BlockSize);
> +    if (Status != EFI_SUCCESS) {
> [SAMI] if (EFI_ERROR (Status))  ?
> [/SAMI]
> +      return Status;

Ok

> +    }
> +    // Update the in memory copy
> +    SetMem64 (Base, NumLba * Instance->BlockSize, ~0UL);
> +    FreePool (Buf);
> +  }
> +
> +  VA_END (Args);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Since we use a memory backed storage we need to restore the RPMB contents
> +  into memory before we register the Fvb protocol.
> +
> +  @param Instace Address to copy flash contents to
> +
> +  @retval     0 on success, OP-TEE error on failure
> +**/
> +STATIC
> +VOID
> +ReadEntireFlash (
> +  MEM_INSTANCE *Instance
> + )
> +{
> +  UINTN ReadAddr;
> +
> +  UINTN StorageFtwWorkingSize = PcdGet32(PcdFlashNvStorageFtwWorkingSize);
> +  UINTN StorageVariableSize   = PcdGet32(PcdFlashNvStorageVariableSize);
> +  UINTN StorageFtwSpareSize   = PcdGet32(PcdFlashNvStorageFtwSpareSize);
> +
> +  ReadAddr = Instance->MemBaseAddress;
> +  // There's no need to check if the read failed here. The upper EDK2 layers
> +  // will initialize the flash correctly if the in-memory copy is wrong
> +  ReadWriteRpmb(SP_SVC_RPMB_READ, ReadAddr, StorageVariableSize +
> +    StorageFtwWorkingSize + StorageFtwSpareSize , 0);
> +}
> +
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +ValidateFvHeader (
> +  IN EFI_FIRMWARE_VOLUME_HEADER            *FwVolHeader
> +  )
> +{
> +  UINT16                      Checksum;
> +  VARIABLE_STORE_HEADER       *VariableStoreHeader;
> +  UINTN                       VariableStoreLength;
> +  UINTN                       FvLength;
> +
> +  FvLength = PcdGet32(PcdFlashNvStorageVariableSize) +
> +             PcdGet32(PcdFlashNvStorageFtwWorkingSize) +
> +             PcdGet32(PcdFlashNvStorageFtwSpareSize);
> +
> +  // Verify the header revision, header signature, length
> +  // Length of FvBlock cannot be 2**64-1
> +  // HeaderLength cannot be an odd number
> +  //
> +  if (   (FwVolHeader->Revision  != EFI_FVH_REVISION)
> +      || (FwVolHeader->Signature != EFI_FVH_SIGNATURE)
> +      || (FwVolHeader->FvLength  != FvLength)
> +      )
> +  {
> +    DEBUG ((DEBUG_INFO, "%a: No Firmware Volume header present\n",
> +      __FUNCTION__));
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  // Check the Firmware Volume Guid
> +  if (!CompareGuid (&FwVolHeader->FileSystemGuid, &gEfiSystemNvDataFvGuid)) {
> [SAMI] Not a comment for this patch but I noticed that BaseTools has an implementation of CompareGuid() that returns INTN.
> I wonder if that is intentional. Moreover, it also appears that the CompareGuid() usage in BaseTools does not consistently follow the explicit comparison rule (https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false).
> [/SAMI]
> +    DEBUG ((DEBUG_INFO, "%a: Firmware Volume Guid non-compatible\n",
> +      __FUNCTION__));
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  // Verify the header checksum
> +  Checksum = CalculateSum16((UINT16*)FwVolHeader, FwVolHeader->HeaderLength);
> +  if (Checksum != 0) {
> +    DEBUG ((DEBUG_INFO, "%a: FV checksum is invalid (Checksum:0x%X)\n",
> +      __FUNCTION__, Checksum));
> +    return EFI_NOT_FOUND;
> [SAMI] Minor: By this point we should be fairly certain that this is a Firmware Volume header.
> So, should the error code returned here be EFI_VOLUME_CORRUPTED? Would the same apply to the remaining checks below?
> [/SAMI]

I think that makes sense, but I'll have to check if there's a
difference in behavior of the upper layers depending on the return
value.
The relevant Volume Header checks is a c/p from
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c and I kept it for
consistency.
Should we change the other driver as well at some point?

> +  }
> +
> +  VariableStoreHeader = (VOID *)((UINTN)FwVolHeader +
> [SAMI] Should the typecast be (VARIABLE_STORE_HEADER*) instead of (VOID *)?
> [/SAMI]

Yes

> +                                 FwVolHeader->HeaderLength);
> +
> +  // Check the Variable Store Guid

[...]

> +  FirmwareVolumeHeader->Revision = EFI_FVH_REVISION;
> +  FirmwareVolumeHeader->BlockMap[0].NumBlocks = Instance->NBlocks;
> +  FirmwareVolumeHeader->BlockMap[0].Length    = Instance->BlockSize;
> +  FirmwareVolumeHeader->BlockMap[1].NumBlocks = 0;
> +  FirmwareVolumeHeader->BlockMap[1].Length    = 0;
> +  FirmwareVolumeHeader->Checksum = CalculateCheckSum16 (
> +                                     (UINT16*)FirmwareVolumeHeader,
> +                                     FirmwareVolumeHeader->HeaderLength);
> +
> +  //
> +  // VARIABLE_STORE_HEADER
> +  //
> +  VariableStoreHeader = (VOID *)((UINTN)Headers +
> [SAMI] Should the typecase be (VARIABLE_STORE_HEADER*) instead of (VOID *)?
> [/SAMI]

Yes

> +                                 FirmwareVolumeHeader->HeaderLength);
> +  CopyGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid);
> +  VariableStoreHeader->Size = PcdGet32(PcdFlashNvStorageVariableSize) -
> +                              FirmwareVolumeHeader->HeaderLength;
> +  VariableStoreHeader->Format = VARIABLE_STORE_FORMATTED;
> +  VariableStoreHeader->State = VARIABLE_STORE_HEALTHY;
> +
> +  Status = ReadWriteRpmb(SP_SVC_RPMB_WRITE, (UINTN) Headers, HeadersLength, 0);
> +  if (Status != EFI_SUCCESS) {
> [SAMI] if (EFI_ERROR (Status))  ?
> [/SAMI]

Ok

> +    goto Exit;
> +  }
> +  // Install the combined header in memory
> +  CopyMem ((VOID*) Instance->MemBaseAddress, Headers, HeadersLength);
> +
> +Exit:
> +  FreePool (Headers);
> +  return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +FvbInitialize (
> +  MEM_INSTANCE *Instance
> +  )
> +{
> +  EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader;
> +  EFI_STATUS                  Status;
> +
> +  if (Instance->Initialized == TRUE) {
> [SAMI] if (Instance->Initialized)  ?
> See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-1-boolean-values-variable-type-boolean-do-not-require-explicit-comparisons-to-true-or-false
> [/SAMI]

Sure

> +    return EFI_SUCCESS;
> +  }
> +
> +  // FirmwareVolumeHeader->FvLength is declared to have the Variable area
> +  // AND the FTW working area AND the FTW Spare contiguous.
> +  ASSERT (PcdGet32 (PcdFlashNvStorageVariableBase) +
> +          PcdGet32 (PcdFlashNvStorageVariableSize) ==
> +          PcdGet32 (PcdFlashNvStorageFtwWorkingBase));
> +  ASSERT (PcdGet32 (PcdFlashNvStorageFtwWorkingBase) +
> +          PcdGet32 (PcdFlashNvStorageFtwWorkingSize) ==
> +          PcdGet32 (PcdFlashNvStorageFtwSpareBase));
> +
> +  // Check if the size of the area is at least one block size
> +  ASSERT ((PcdGet32 (PcdFlashNvStorageVariableSize) > 0) &&
> +          (PcdGet32 (PcdFlashNvStorageVariableSize) / Instance->BlockSize > 0));
> +  ASSERT ((PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > 0) &&
> +          (PcdGet32 (PcdFlashNvStorageFtwWorkingSize) / Instance->BlockSize > 0));
> +  ASSERT ((PcdGet32 (PcdFlashNvStorageFtwSpareSize) > 0) &&
> +          (PcdGet32 (PcdFlashNvStorageFtwSpareSize) / Instance->BlockSize > 0));
> +
> +  // Ensure the Variable areas are aligned on block size boundaries
> +  ASSERT ((PcdGet32 (PcdFlashNvStorageVariableBase) % Instance->BlockSize) == 0);
> +  ASSERT ((PcdGet32 (PcdFlashNvStorageFtwWorkingBase) % Instance->BlockSize) == 0);
> +  ASSERT ((PcdGet32 (PcdFlashNvStorageFtwSpareBase) % Instance->BlockSize) == 0);
> [SAMI] These asserts may need to be adjusted if 64-bit versions of the PCDs are used.
> [/SAMI]

Noted

> +
> +  // Read the file from disk and copy it to memory
> +  ReadEntireFlash (Instance);
> +
> +  FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *) Instance->MemBaseAddress;
> +  Status = ValidateFvHeader(FwVolHeader);
> +  if (EFI_ERROR (Status)) {
> +    // There is no valid header, so time to install one.
> +    DEBUG ((DEBUG_INFO, "%a: The FVB Header is not valid.\n", __FUNCTION__));
> +
> +    // Reset memory
> +    SetMem64 ((VOID *)Instance->MemBaseAddress, Instance->NBlocks * Instance->BlockSize, ~0UL);
> +    DEBUG ((DEBUG_INFO, "%a: Erasing Flash.\n", __FUNCTION__));
> +    Status = ReadWriteRpmb(SP_SVC_RPMB_WRITE, Instance->MemBaseAddress,
> +      PcdGet32(PcdFlashNvStorageVariableSize) +
> +      PcdGet32(PcdFlashNvStorageFtwWorkingSize) +
> +      PcdGet32(PcdFlashNvStorageFtwSpareSize), 0);
> +    if (Status != EFI_SUCCESS) {
> [SAMI] if (EFI_ERROR (Status))  ?
> [/SAMI]

Sure

> +      return Status;
> +    }
> +    // Install all appropriate headers
> +    DEBUG ((DEBUG_INFO, "%a: Installing a correct one for this volume.\n",
> +      __FUNCTION__));
> +    Status = InitializeFvAndVariableStoreHeaders (Instance);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +  } else {
> +    DEBUG ((DEBUG_INFO, "%a: Found valid FVB Header.\n", __FUNCTION__));
> +  }
> +  Instance->Initialized = TRUE;
> +
> +  return EFI_SUCCESS;
> [SAMI] return Status; ?
> [/SAMI]
> +}

Sure

> +
> +EFI_STATUS
> +EFIAPI
> +OpTeeRpmbFvbInit (
> +  IN EFI_HANDLE           ImageHandle,
> +  IN EFI_MM_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS   Status;
> +  VOID         *Addr;
> +  UINTN        FvLength;
> +  UINTN        NBlocks;
> +
> +  FvLength = PcdGet32(PcdFlashNvStorageVariableSize) +
> +             PcdGet32(PcdFlashNvStorageFtwWorkingSize) +
> +             PcdGet32(PcdFlashNvStorageFtwSpareSize);
> +
> +  NBlocks = EFI_SIZE_TO_PAGES(ALIGN_VARIABLE(FvLength));
> +  Addr = AllocatePages(NBlocks);
> +  ASSERT (Addr != NULL);
> [SAMI] Asserts will vanish in release builds and a failure to allocate memory would result in incorrect results.
> Please handle this error and return EFI_OUT_OF_RESOURCES.
> [/SAMI]

Fair enough

> +
> +  SetMem (&mInstance, sizeof (mInstance), 0);
> +
> +  mInstance.FvbProtocol.GetPhysicalAddress = OpTeeRpmbFvbGetPhysicalAddress;
> +  mInstance.FvbProtocol.GetAttributes      = OpTeeRpmbFvbGetAttributes;
> +  mInstance.FvbProtocol.SetAttributes      = OpTeeRpmbFvbSetAttributes;
> +  mInstance.FvbProtocol.GetBlockSize       = OpTeeRpmbFvbGetBlockSize;
> +  mInstance.FvbProtocol.EraseBlocks        = OpTeeRpmbFvbErase;
> +  mInstance.FvbProtocol.Write              = OpTeeRpmbFvbWrite;
> +  mInstance.FvbProtocol.Read               = OpTeeRpmbFvbRead;
> +
> +  mInstance.MemBaseAddress = (EFI_PHYSICAL_ADDRESS) Addr;
> +  mInstance.Signature      = FLASH_SIGNATURE;
> +  mInstance.Initialize     = FvbInitialize;
> +  mInstance.BlockSize      = EFI_PAGE_SIZE;
> +  mInstance.NBlocks        = NBlocks;
> +
> +  // Update the defined PCDs related to Variable Storage
> +  PatchPcdSet32 (PcdFlashNvStorageVariableBase, mInstance.MemBaseAddress);
> [SAMI] Should the 64-bit versions of the PCDs be used here.
> A recent change added similar support to the ArmPlatformPkg\Drivers\NorFlashDxe.
> [/SAMI]

Noted

> +  PatchPcdSet32 (PcdFlashNvStorageFtwWorkingBase, mInstance.MemBaseAddress +
> +    PcdGet32 (PcdFlashNvStorageVariableSize));
> +  PatchPcdSet32 (PcdFlashNvStorageFtwSpareBase, mInstance.MemBaseAddress +
> +    PcdGet32 (PcdFlashNvStorageVariableSize) +
> +    PcdGet32 (PcdFlashNvStorageFtwWorkingSize));
> +
> +  Status = gMmst->MmInstallProtocolInterface (
> +                    &mInstance.Handle,
> +                    &gEfiSmmFirmwareVolumeBlockProtocolGuid,
> +                    EFI_NATIVE_INTERFACE,
> +                    &mInstance.FvbProtocol
> +                    );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  DEBUG ((DEBUG_INFO, "%a: Register OP-TEE RPMB Fvb\n", __FUNCTION__));
> +  DEBUG ((DEBUG_INFO, "%a: Using NV store FV in-memory copy at 0x%lx\n",
> +    __FUNCTION__, PatchPcdGet32 (PcdFlashNvStorageVariableBase)));
> +
> +  return Status;
> +}
> --
> 2.17.1
>
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.