From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from foss.arm.com (foss.arm.com [217.140.110.172])
 by mx.groups.io with SMTP id smtpd.web10.397.1614967150205397729
 for <devel@edk2.groups.io>;
 Fri, 05 Mar 2021 09:59:10 -0800
Authentication-Results: mx.groups.io;
 dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com)
Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14])
	by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id AC2F431B;
	Fri,  5 Mar 2021 09:58:59 -0800 (PST)
Received: from [192.168.1.169] (unknown [172.31.20.19])
	by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id D288E3F766;
	Fri,  5 Mar 2021 09:58:58 -0800 (PST)
From: "PierreGondois" <pierre.gondois@arm.com>
Subject: Re: [edk2-devel] [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver
References: <20210212173459.508205-1-ilias.apalodimas@linaro.org>
 <20210212173459.508205-2-ilias.apalodimas@linaro.org>
To: devel@edk2.groups.io, Ilias Apalodimas <ilias.apalodimas@linaro.org>,
 Sami.Mujawar@arm.com
Message-ID: <ade96a28-d4c9-c1f2-0ff1-77b425189015@arm.com>
Date: Fri, 5 Mar 2021 17:58:49 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.0
MIME-Version: 1.0
In-Reply-To: <20210212173459.508205-2-ilias.apalodimas@linaro.org>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable

Hi Ilias,
Here is the rest of the review. Sorry to do it in 2 times.

Regards,

Pierre


>
> +/**
>
> +=A0 Fixup the Pcd values for variable storage
>
> +
>
> +=A0 Since the upper layers of EDK2 expect a memory mapped interface an=
d=20
> we can't
>
> +=A0 offer that from an RPMB, the driver allocates memory on init and=20
> passes that
>
> +=A0 on the upper layers. Since the memory is dynamically allocated and=
=20
> we can't set the
>
> +=A0 PCD is StMM context, we need to patch it correctly on each access
>
> +
>
> +=A0 @retval EFI_SUCCESS Protocol was found and PCDs patched up
The error codes are missing.
>
> +
>
> + **/
>
> +EFI_STATUS
>
> +EFIAPI
>
> +FixPcdMemory (
>
> +=A0 VOID
>
> +=A0 )
>
> +{
>
> +=A0 EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL=A0 *FvbProtocol;
>
> +=A0 MEM_INSTANCE=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0 *Instance;
>
> +=A0 EFI_STATUS=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0 Status;
>
> +
>
> +=A0 //
>
> +=A0 // Locate SmmFirmwareVolumeBlockProtocol
>
> +=A0 //
>
> +=A0 Status =3D gMmst->MmLocateProtocol (
>
> + &gEfiSmmFirmwareVolumeBlockProtocolGuid,
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 NULL,
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 (VOID **) &F=
vbProtocol
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 );
>
> +=A0 ASSERT_EFI_ERROR (Status);
>
> +
>
> +=A0 Instance =3D INSTANCE_FROM_FVB_THIS (FvbProtocol);
>
> +=A0 // The Pcd is user defined, so make sure we don't overflow
>
> +=A0 if (Instance->MemBaseAddress > MAX_UINT64 - PcdGet32=20
> (PcdFlashNvStorageVariableSize)) {
I think this can be removed since the next condition is more strict.
>
> +=A0=A0=A0 return EFI_INVALID_PARAMETER;
>
> +=A0 }
>
> +
>
>
[...]
> +STATIC
>
> +EFI_STATUS
>
> +ReadWriteRpmb (
>
> +=A0 UINTN=A0 SvcAct,
>
> +=A0 UINTN=A0 Addr,
>
> +=A0 UINTN=A0 NumBytes,
>
> +=A0 UINTN=A0 Offset
>
> +=A0 )
>
> +{
>
> +=A0 ARM_SVC_ARGS=A0 SvcArgs;
>
> +=A0 EFI_STATUS=A0=A0=A0 Status;
>
> +
>
> +=A0 ZeroMem (&SvcArgs, sizeof (SvcArgs));
>
> +
>
> +=A0 SvcArgs.Arg0 =3D ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;

If this is an FFA call, is it possible to:
 =A0- put a reference in the header to the spec (it should be similar to=20
the one at=20
edk2/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c)
 =A0- check the return status of the SVC call against the ones available=20
at edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
 =A0- if possible, remove the dependency to <IndustryStandard/ArmMmSvc.h>

>
> +=A0 SvcArgs.Arg1 =3D mStorageId;
>
> +=A0 SvcArgs.Arg2 =3D 0;
>
> +=A0 SvcArgs.Arg3 =3D SvcAct;
>
> +=A0 SvcArgs.Arg4 =3D Addr;
>
> +=A0 SvcArgs.Arg5 =3D NumBytes;
>
> +=A0 SvcArgs.Arg6 =3D Offset;
>
> +
>
> +=A0 ArmCallSvc (&SvcArgs);
>
> +=A0 if (SvcArgs.Arg3) {
>
> +=A0=A0=A0 DEBUG ((DEBUG_ERROR, "%a: Svc Call 0x%08x Addr: 0x%08x len: =
0x%x=20
> Offset: 0x%x failed with 0x%x\n",
>
> +=A0=A0=A0=A0=A0 __func__, SvcAct, Addr, NumBytes, Offset, SvcArgs.Arg3=
));
>
> +=A0 }
>
> +
>
> +=A0 switch (SvcArgs.Arg3) {
>
> +=A0 case ARM_SVC_SPM_RET_SUCCESS:
>
> +=A0=A0=A0 Status =3D EFI_SUCCESS;
>
> +=A0=A0=A0 break;
>
> +
>
> +=A0 case ARM_SVC_SPM_RET_NOT_SUPPORTED:
>
> +=A0=A0=A0 Status =3D EFI_UNSUPPORTED;
>
> +=A0=A0=A0 break;
>
> +
>
> +=A0 case ARM_SVC_SPM_RET_INVALID_PARAMS:
>
> +=A0=A0=A0 Status =3D EFI_INVALID_PARAMETER;
>
> +=A0=A0=A0 break;
>
> +
>
> +=A0 case ARM_SVC_SPM_RET_DENIED:
>
> +=A0=A0=A0 Status =3D EFI_ACCESS_DENIED;
>
> +=A0=A0=A0 break;
>
> +
>
> +=A0 case ARM_SVC_SPM_RET_NO_MEMORY:
>
> +=A0=A0=A0 Status =3D EFI_OUT_OF_RESOURCES;
>
> +=A0=A0=A0 break;
>
> +
>
> +=A0 default:
>
> +=A0=A0=A0 Status =3D EFI_ACCESS_DENIED;
>
> +=A0 }
>
> +
>
> +=A0 return Status;
>
> +}
>
[...]
>
> +STATIC
>
> +EFI_STATUS
>
> +EFIAPI
>
> +ValidateFvHeader (
>
> +=A0 IN EFI_FIRMWARE_VOLUME_HEADER=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 *Fw=
VolHeader
>
> +=A0 )
>
> +{
>
> +=A0 UINT16=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0 Checksum;
>
> +=A0 VARIABLE_STORE_HEADER=A0=A0=A0=A0=A0=A0 *VariableStoreHeader;
>
> +=A0 UINTN=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0 VariableStoreLength;
>
> +=A0 UINTN=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0 FvLength;
>
> +
>
> +=A0 FvLength =3D PcdGet32(PcdFlashNvStorageVariableSize) +
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 PcdGet32(PcdFlashNvStorageFtwWork=
ingSize) +
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 PcdGet32(PcdFlashNvStorageFtwSpar=
eSize);
>
> +
>
> +=A0 // Verify the header revision, header signature, length
>
> +=A0 // Length of FvBlock cannot be 2**64-1
>
> +=A0 // HeaderLength cannot be an odd number
>
> +=A0 //
>
> +=A0 if (=A0=A0 (FwVolHeader->Revision=A0 !=3D EFI_FVH_REVISION)
>
> +=A0=A0=A0=A0=A0 || (FwVolHeader->Signature !=3D EFI_FVH_SIGNATURE)
>
> +=A0=A0=A0=A0=A0 || (FwVolHeader->FvLength=A0 !=3D FvLength)
>
> +=A0=A0=A0=A0=A0 )
could be on the same line -> ') {'
>
> +=A0 {
>
> +=A0=A0=A0 DEBUG ((DEBUG_INFO, "%a: No Firmware Volume header present\n=
",
>
> +=A0=A0=A0=A0=A0 __FUNCTION__));
>
> +=A0=A0=A0 return EFI_NOT_FOUND;
>
> +=A0 }
>
> +
>
> +=A0 // Check the Firmware Volume Guid
>
> +=A0 if (!CompareGuid (&FwVolHeader->FileSystemGuid,=20
> &gEfiSystemNvDataFvGuid)) {
>
> +=A0=A0=A0 DEBUG ((DEBUG_INFO, "%a: Firmware Volume Guid non-compatible=
\n",
>
> +=A0=A0=A0=A0=A0 __FUNCTION__));
>
> +=A0=A0=A0 return EFI_VOLUME_CORRUPTED;
>
> +=A0 }
>
> +
>
> +=A0 // Verify the header checksum
>
> +=A0 Checksum =3D CalculateSum16((UINT16*)FwVolHeader,=20
> FwVolHeader->HeaderLength);
>
> +=A0 if (Checksum !=3D 0) {
>
> +=A0=A0=A0 DEBUG ((DEBUG_INFO, "%a: FV checksum is invalid (Checksum:0x=
%X)\n",
>
> +=A0=A0=A0=A0=A0 __FUNCTION__, Checksum));
>
> +=A0=A0=A0 return EFI_VOLUME_CORRUPTED;
>
> +=A0 }
>
> +
>
> +=A0 VariableStoreHeader =3D (VARIABLE_STORE_HEADER*)((UINTN)FwVolHeade=
r +
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
 FwVolHeader->HeaderLength);
>
> +
>
> +=A0 // Check the Variable Store Guid
>
> +=A0 if (!CompareGuid (&VariableStoreHeader->Signature,=20
> &gEfiVariableGuid) &&
>
> +=A0=A0=A0=A0=A0 !CompareGuid (&VariableStoreHeader->Signature,=20
> &gEfiAuthenticatedVariableGuid)) {
>
> +=A0=A0=A0 DEBUG ((DEBUG_INFO, "%a: Variable Store Guid non-compatible\=
n",=20
> __FUNCTION__));
>
> +=A0=A0=A0 return EFI_VOLUME_CORRUPTED;
>
> +=A0 }
>
> +
>
> +=A0 VariableStoreLength =3D PcdGet32 (PcdFlashNvStorageVariableSize) -
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =
FwVolHeader->HeaderLength;
>
> +=A0 if (VariableStoreHeader->Size !=3D VariableStoreLength) {
>
> +=A0=A0=A0 DEBUG ((DEBUG_INFO, "%a: Variable Store Length does not matc=
h\n",
>
> +=A0=A0=A0=A0=A0 __FUNCTION__));
>
> +=A0=A0=A0 return EFI_VOLUME_CORRUPTED;
>
> +=A0 }
>
> +
>
> +=A0 return EFI_SUCCESS;
>
empty line, could be removed
> +
>
> +}
>
> +
>
>
[...]
>
> +**/
>
> +STATIC
>
> +EFI_STATUS
>
> +EFIAPI
>
> +FvbInitialize (
>
> +=A0 MEM_INSTANCE *Instance
>
> +=A0 )
>
> +{
>
> +=A0 EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader;
>
> +=A0 EFI_STATUS=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 Stat=
us;
>
> +
>
> +=A0 if (Instance->Initialized) {
>
> +=A0=A0=A0 return EFI_SUCCESS;
>
> +=A0 }
>
> +
>
> +=A0 // FirmwareVolumeHeader->FvLength is declared to have the Variable=
 area
>
> +=A0 // AND the FTW working area AND the FTW Spare contiguous.
>
> +=A0 ASSERT (
>
> +=A0=A0=A0 (PcdGet64 (PcdFlashNvStorageVariableBase64) +
>
> +=A0=A0=A0 PcdGet32 (PcdFlashNvStorageVariableSize)) =3D=3D
>
> +=A0=A0=A0 PcdGet64 (PcdFlashNvStorageFtwWorkingBase64)
>
> +=A0=A0=A0 );
>
> +=A0 ASSERT (
>
> +=A0=A0=A0 (PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) +
>
> +=A0=A0=A0 PcdGet32 (PcdFlashNvStorageFtwWorkingSize)) =3D=3D
>
> +=A0=A0=A0 PcdGet64 (PcdFlashNvStorageFtwSpareBase64));
>
> +
>
> +=A0 // Check if the size of the area is at least one block size
>
> +=A0 ASSERT (
>
> +=A0=A0=A0 (PcdGet32 (PcdFlashNvStorageVariableSize) > 0) &&
I think the first check (Size > 0) is redundant with the second one=20
(Size > BlockSize).
>
> +=A0=A0=A0 (PcdGet32 (PcdFlashNvStorageVariableSize) / Instance->BlockS=
ize > 0)
>
> +=A0=A0=A0 );
>
> +=A0 ASSERT (
>
> +=A0=A0=A0 (PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > 0) &&
>
> +=A0=A0=A0 (PcdGet32 (PcdFlashNvStorageFtwWorkingSize) / Instance->Bloc=
kSize=20
> > 0)
>
> +=A0=A0=A0 );
>
> +=A0 ASSERT (
>
> +=A0=A0=A0 (PcdGet32 (PcdFlashNvStorageFtwSpareSize) > 0) &&
>
> +=A0=A0=A0 (PcdGet32 (PcdFlashNvStorageFtwSpareSize) / Instance->BlockS=
ize > 0)
>
> +=A0=A0=A0 );
>
> +
>
> +=A0 // Ensure the Variable areas are aligned on block size boundaries
>
> +=A0 ASSERT ((PcdGet64 (PcdFlashNvStorageVariableBase64) %=20
> Instance->BlockSize) =3D=3D 0);
>
> +=A0 ASSERT ((PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) %=20
> Instance->BlockSize) =3D=3D 0);
>
> +=A0 ASSERT ((PcdGet64 (PcdFlashNvStorageFtwSpareBase64) %=20
> Instance->BlockSize) =3D=3D 0);
>
> +
>
> +=A0 // Read the file from disk and copy it to memory
>
> +=A0 ReadEntireFlash (Instance);
>
> +
>
> +=A0 FwVolHeader =3D (EFI_FIRMWARE_VOLUME_HEADER *)Instance->MemBaseAdd=
ress;
>
> +=A0 Status =3D ValidateFvHeader (FwVolHeader);
>
> +=A0 if (EFI_ERROR (Status)) {
>
> +=A0=A0=A0 // There is no valid header, so time to install one.
>
> +=A0=A0=A0 DEBUG ((DEBUG_INFO, "%a: The FVB Header is not valid.\n",=20
> __FUNCTION__));
>
> +
>
> +=A0=A0=A0 // Reset memory
>
> +=A0=A0=A0 SetMem64 ((VOID *)Instance->MemBaseAddress, Instance->NBlock=
s *=20
> Instance->BlockSize, ~0UL);
>
> +=A0=A0=A0 DEBUG ((DEBUG_INFO, "%a: Erasing Flash.\n", __FUNCTION__));
>
> +=A0=A0=A0 Status =3D ReadWriteRpmb (
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 SP_SVC_RPMB_WRITE,
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 Instance->MemBaseAddress,
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 PcdGet32(PcdFlashNvStorageV=
ariableSize) +
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 PcdGet32(PcdFlashNvStorageF=
twWorkingSize) +
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 PcdGet32(PcdFlashNvStorageF=
twSpareSize),
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 0
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 );
>
> +=A0=A0=A0 if (EFI_ERROR (Status)) {
>
> +=A0=A0=A0=A0=A0 return Status;
>
> +=A0=A0=A0 }
>
> +=A0=A0=A0 // Install all appropriate headers
>
> +=A0=A0=A0 DEBUG ((DEBUG_INFO, "%a: Installing a correct one for this=20
> volume.\n",
>
> +=A0=A0=A0=A0=A0 __FUNCTION__));
>
> +=A0=A0=A0 Status =3D InitializeFvAndVariableStoreHeaders (Instance);
>
> +=A0=A0=A0 if (EFI_ERROR (Status)) {
>
> +=A0=A0=A0=A0=A0 return Status;
>
> +=A0=A0=A0 }
>
> +=A0 } else {
>
> +=A0=A0=A0 DEBUG ((DEBUG_INFO, "%a: Found valid FVB Header.\n", __FUNCT=
ION__));
>
> +=A0 }
>
> +=A0 Instance->Initialized =3D TRUE;
>
> +
>
> +=A0 return Status;
>
> +}
>
> +
>
> +/**
>
> +=A0 Since the RPMB is not byte addressable we need to allocate memory
>
> +=A0 and sync that on reads/writes. Initialize the memory and install t=
he
>
> +=A0 Fvb protocol.
>
> +
>
> +=A0 @param ImageHandle The EFI image handle
>
> +=A0 @param SystemTable MM system table
>
> +
>
> +=A0 @retval EFI_SUCCESS Protocol installed
>
> +
>
> +=A0 @retval EFI_INVALID_PARAMETER Invalid Pcd values specified
>
> +
>
> +=A0 @retval EFI_OUT_OF_RESOURCES=A0 Can't allocate necessary memory
>
> +**/
>
> +EFI_STATUS
>
> +EFIAPI
>
> +OpTeeRpmbFvbInit (
>
> +=A0 IN EFI_HANDLE=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 ImageHandle,
>
> +=A0 IN EFI_MM_SYSTEM_TABLE=A0 *SystemTable
>
> +=A0 )
>
> +{
>
> +=A0 EFI_STATUS=A0=A0 Status;
>
> +=A0 VOID=A0=A0=A0=A0=A0=A0=A0=A0 *Addr;
>
> +=A0 UINTN=A0=A0=A0=A0=A0=A0=A0 FvLength;
>
> +=A0 UINTN=A0=A0=A0=A0=A0=A0=A0 NBlocks;
>
> +
>
> +=A0 FvLength =3D PcdGet32(PcdFlashNvStorageVariableSize) +
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 PcdGet32(PcdFlashNvStorageFtwWork=
ingSize) +
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 PcdGet32(PcdFlashNvStorageFtwSpar=
eSize);
>
> +
>
> +=A0 NBlocks =3D EFI_SIZE_TO_PAGES (ALIGN_VARIABLE (FvLength));
>
> +=A0 Addr =3D AllocatePages (NBlocks);
>
> +=A0 if (Addr =3D=3D NULL) {
>
> +=A0=A0=A0 ASSERT (0);
>
> +=A0=A0=A0 return EFI_OUT_OF_RESOURCES;
>
> +=A0 }
>
> +
>
> +=A0 SetMem (&mInstance, sizeof (mInstance), 0);
NIT: you can use ZeroMem()
>
> +
>
> +=A0 mInstance.FvbProtocol.GetPhysicalAddress =3D=20
> OpTeeRpmbFvbGetPhysicalAddress;
>
> +=A0 mInstance.FvbProtocol.GetAttributes=A0=A0=A0=A0=A0 =3D OpTeeRpmbFv=
bGetAttributes;
>
> +=A0 mInstance.FvbProtocol.SetAttributes=A0=A0=A0=A0=A0 =3D OpTeeRpmbFv=
bSetAttributes;
>
> +=A0 mInstance.FvbProtocol.GetBlockSize=A0=A0=A0=A0=A0=A0 =3D OpTeeRpmb=
FvbGetBlockSize;
>
> +=A0 mInstance.FvbProtocol.EraseBlocks=A0=A0=A0=A0=A0=A0=A0 =3D OpTeeRp=
mbFvbErase;
>
> +=A0 mInstance.FvbProtocol.Write=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
 =3D OpTeeRpmbFvbWrite;
>
> +=A0 mInstance.FvbProtocol.Read=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
 =3D OpTeeRpmbFvbRead;
>
> +
>
> +=A0 mInstance.MemBaseAddress =3D (EFI_PHYSICAL_ADDRESS)Addr;
>
> +=A0 mInstance.Signature=A0=A0=A0=A0=A0 =3D FLASH_SIGNATURE;
>
> +=A0 mInstance.Initialize=A0=A0=A0=A0 =3D FvbInitialize;
>
> +=A0 mInstance.BlockSize=A0=A0=A0=A0=A0 =3D EFI_PAGE_SIZE;
>
> +=A0 mInstance.NBlocks=A0=A0=A0=A0=A0=A0=A0 =3D NBlocks;
>
> +
>
> +=A0 // The Pcd is user defined, so make sure we don't overflow
>
> +=A0 if (mInstance.MemBaseAddress > MAX_UINT64 - PcdGet32=20
> (PcdFlashNvStorageVariableSize)) {
>
I don't think this is necessary to do any check here since the memory=20
has just been allocated with the right size.
> +=A0=A0=A0 return EFI_INVALID_PARAMETER;
>
> +=A0 }
>
> +
>
> +=A0 if (mInstance.MemBaseAddress > MAX_UINT64 - PcdGet32=20
> (PcdFlashNvStorageVariableSize) -
>
> +=A0=A0=A0 PcdGet32 (PcdFlashNvStorageFtwWorkingSize)) {
>
> +=A0=A0=A0 return EFI_INVALID_PARAMETER;
>
> +=A0 }
>
> +
>
> +=A0 // Update the defined PCDs related to Variable Storage
>
> +=A0 PatchPcdSet64 (PcdFlashNvStorageVariableBase64,=20
> mInstance.MemBaseAddress);
>
> +=A0 PatchPcdSet64 (
>
> +=A0=A0=A0 PcdFlashNvStorageFtwWorkingBase64,
>
> +=A0=A0=A0 mInstance.MemBaseAddress + PcdGet32 (PcdFlashNvStorageVariab=
leSize)
>
> +=A0=A0=A0 );
>
> +=A0 PatchPcdSet64 (
>
> +=A0=A0=A0 PcdFlashNvStorageFtwSpareBase64,
>
> +=A0=A0=A0 mInstance.MemBaseAddress +
>
> +=A0=A0=A0 PcdGet32 (PcdFlashNvStorageVariableSize) +
>
> +=A0=A0=A0 PcdGet32 (PcdFlashNvStorageFtwWorkingSize)
>
> +=A0=A0=A0 );
Do we actually need to set these PCDs ? If the PCDs are persistent, we=20
should not need to patch them in FixupPcd.c. FvbInitialize() is using=20
them, but it is not called from OpTeeRpmbFvbInit().
>
> +
>
> +=A0 Status =3D gMmst->MmInstallProtocolInterface (
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 &mInstance.H=
andle,
>
> + &gEfiSmmFirmwareVolumeBlockProtocolGuid,
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 EFI_NATIVE_I=
NTERFACE,
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 &mInstance.F=
vbProtocol
>
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 );
>
> +=A0 ASSERT_EFI_ERROR (Status);
>
> +
>
> +=A0 DEBUG ((DEBUG_INFO, "%a: Register OP-TEE RPMB Fvb\n", __FUNCTION__=
));
>
> +=A0 DEBUG ((DEBUG_INFO, "%a: Using NV store FV in-memory copy at 0x%lx=
\n",
>
> +=A0=A0=A0 __FUNCTION__, PatchPcdGet64 (PcdFlashNvStorageVariableBase64=
)));
>
> +
>
> +=A0 return Status;
>
> +}