From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yb1-f178.google.com (mail-yb1-f178.google.com [209.85.219.178]) by mx.groups.io with SMTP id smtpd.web12.6694.1611907357452600889 for ; Fri, 29 Jan 2021 00:02:37 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=c/Ph86Sz; spf=pass (domain: linaro.org, ip: 209.85.219.178, mailfrom: ilias.apalodimas@linaro.org) Received: by mail-yb1-f178.google.com with SMTP id m76so1384930ybf.0 for ; Fri, 29 Jan 2021 00:02:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=1BGpYk3dieePsDWRpjGwSPwvZVEXVSrqFlmMLZXj4UY=; b=c/Ph86SzACY+TgpIoAcxA9h3hP4u8HgljmLbqOL3vIGMefgFYPrCZXdy8Dqk/OSMA7 RQ3kKSMb96GurVr1woIOf1JWRHVR9xnl4vFxnz2gWcB1Cz8jWINYzDDI5cdT/Fx9l+d9 mvoOgT7X0+T9KRNzeaqpQDcxnrlutjyv5Wc0K8iyjUHU8OEAn5Fxt0OE84yB8qQM9w2o L0YNXq94CIfY1LDB2WA+zAmp6aKOaQsgGay3oNxgPzhHNvoXckcMnlGpjM6GCargib1p CzGrQqNAnTKb392AUMBafNezVhKckDDrSySYGUbOqdeAgYLvUVj+Medby9NmL/aXYLEc pXaw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=1BGpYk3dieePsDWRpjGwSPwvZVEXVSrqFlmMLZXj4UY=; b=uBdJu7G2n8k1O1ilT2if7u83FBHsFKrHyDE23VTSBmF7OEgquYMIHnx7YcBSr84cnK 8LrlbsSY8DtuvgZ446C5s3w0StTDeuWGMmHs1iFbi21XQjSheOrK9IAfcycJOs+0CsbX juMjigYhxHtNdvbny6yQu+5N4Ut1Q0vtL0kaEYLxXrrXfVV+CQHfHv2JO9thH5829vjQ zpfnyXwNZT/1R/HC+MpHTkVrAE39dPebLwnZkL3HIouyCkp0yRJyIn7/FgUbzwU74yFS KrTb5YeByfjAY9gBVgF/r2Yn8VxrOxm6sYL2NVf1gaE9+BUViX5ZsMFdQTTw9zkECqgC I5VQ== X-Gm-Message-State: AOAM531y6hc6BAlHcERpJQMs+isjrkizposSbs8GOvkEjN6bJHXDvdFg f15KStrhHgV6hqZLkWTCpDmynO6/yaw3Ej1CefJGyQ== X-Google-Smtp-Source: ABdhPJxdaM9LxnU6v5Rz3tqrUZoEWRYCqSpTnT+mYOAEIIO69lMeDUOTd83f9Gigx+kK1vNyCNbcz2QYs0IRqzPin/4= X-Received: by 2002:a25:2fc1:: with SMTP id v184mr104336ybv.51.1611907356550; Fri, 29 Jan 2021 00:02:36 -0800 (PST) MIME-Version: 1.0 References: <20201216110903.17995-1-sughosh.ganu@linaro.org> <20201216110903.17995-2-sughosh.ganu@linaro.org> In-Reply-To: From: "Ilias Apalodimas" Date: Fri, 29 Jan 2021 10:02:00 +0200 Message-ID: Subject: Re: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver To: Sami Mujawar Cc: Sughosh Ganu , "devel@edk2.groups.io" , Ard Biesheuvel , Leif Lindholm , Sahil Malhotra Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Sami, Thanks for taking the time On Wed, 27 Jan 2021 at 19:10, Sami Mujawar 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 repo= rted 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 > Sent: 16 December 2020 11:09 AM > To: devel@edk2.groups.io > Cc: Sami Mujawar ; Ard Biesheuvel ; Leif Lindholm ; Sahil Malhotra ; Ilias Apalodimas > Subject: [PATCH edk2-platforms v3 1/2] Drivers/OpTeeRpmb: Add an OP-TEE b= acked RPMB driver > > From: 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 [...] > --- /dev/null > +++ b/Drivers/OpTeeRpmb/OpTeeRpmbFvb.h > @@ -0,0 +1,35 @@ > +/** @file > + > + Copyright (c) 2020, Linaro Ltd. All rights reserved.
> + 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-d= ocs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/53_in= clude_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-he= ader-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.gitb= ook.io/edk-ii-c-coding-standards-specification/5_source_files/56_declaratio= ns_and_types#5-6-3-2-structure-declaration-with-forward-reference-or-self-r= eference > [/SAMI] Ok > + > +#endif > diff --git a/Drivers/OpTeeRpmb/FixupPcd.c b/Drivers/OpTeeRpmb/FixupPcd.c > new file mode 100644 [...] > + > + Instance =3D INSTANCE_FROM_FVB_THIS(FvbProtocol); > + // Patch PCDs with the the correct values > + PatchPcdSet32 (PcdFlashNvStorageVariableBase, Instance->MemBaseAddress= ); > + PatchPcdSet32 (PcdFlashNvStorageFtwWorkingBase, Instance->MemBaseAddre= ss + > + 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\NorFl= ashDxe. > [/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 =3D 3U; > +static const UINT16 storage_id =3D 4U; > [SAMI] Please change to STATIC CONST and also update the variable names a= ccording 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 wi= ll be > + copied to that buffer after reading them from the de= vice. > + 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 =3D ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64; > + SvcArgs.Arg1 =3D storage_id; > + SvcArgs.Arg2 =3D 0; > + SvcArgs.Arg3 =3D SvcAct; > + SvcArgs.Arg4 =3D Addr; > + SvcArgs.Arg5 =3D NumBytes; > + SvcArgs.Arg6 =3D Offset; > + > + ArmCallSvc (&SvcArgs); > + if (SvcArgs.Arg3) { > + DEBUG ((DEBUG_ERROR, "%a: Svc Call 0x%08x Addr: 0x%08x len: 0x%x Off= set: 0x%x failed with 0x%x\n", > + __func__, SvcAct, Addr, NumBytes, Offset, SvcArgs.Arg3)); > + } > + > + switch (SvcArgs.Arg3) { > + case ARM_SVC_SPM_RET_SUCCESS: > + Status =3D EFI_SUCCESS; > + break; > + > + case ARM_SVC_SPM_RET_NOT_SUPPORTED: > + Status =3D EFI_UNSUPPORTED; > + break; > + > + case ARM_SVC_SPM_RET_INVALID_PARAMS: > + Status =3D EFI_INVALID_PARAMETER; > + break; > + > + case ARM_SVC_SPM_RET_DENIED: > + Status =3D EFI_ACCESS_DENIED; > + break; > + > + case ARM_SVC_SPM_RET_NO_MEMORY: > + Status =3D EFI_BAD_BUFFER_SIZE; > + break; > + > + default: > + Status =3D EFI_ACCESS_DENIED; > + } > [SAMI] Should the error handling here be updated similar to the FF-A Stan= daloneMmPkg 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 =3D EFI_SUCCESS; > + MEM_INSTANCE *Instance; > + VOID *Base; > + > + Instance =3D INSTANCE_FROM_FVB_THIS(This); > + if (Instance->Initialized =3D=3D 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-boolea= n-do-not-require-explicit-comparisons-to-true-or-false > [/SAMI] Ok > + Instance->Initialize (Instance); > + } > + > + Base =3D (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize = + Offset; [...] > + MEM_INSTANCE *Instance; > + EFI_STATUS Status =3D EFI_SUCCESS; > + VOID *Base; > + > + Instance =3D INSTANCE_FROM_FVB_THIS(This); > + if (Instance->Initialized =3D=3D 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-boolea= n-do-not-require-explicit-comparisons-to-true-or-false > [/SAMI] sure > + Instance->Initialize (Instance); > + } > + Base =3D (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize = + Offset; > + Status =3D ReadWriteRpmb (SP_SVC_RPMB_WRITE, (UINTN) Buffer, *NumBytes= , > + Lba * Instance->BlockSize + Offset); > + if (Status !=3D 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 =3D ReadWriteRpmb (SP_SVC_RPMB_WRITE, (UINTN) Buf, NumBytes, > + Start * Instance->BlockSize); > + if (Status !=3D 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 conte= nts > + 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 =3D PcdGet32(PcdFlashNvStorageFtwWorkingSi= ze); > + UINTN StorageVariableSize =3D PcdGet32(PcdFlashNvStorageVariableSize= ); > + UINTN StorageFtwSpareSize =3D PcdGet32(PcdFlashNvStorageFtwSpareSize= ); > + > + ReadAddr =3D Instance->MemBaseAddress; > + // There's no need to check if the read failed here. The upper EDK2 la= yers > + // 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 =3D 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 !=3D EFI_FVH_REVISION) > + || (FwVolHeader->Signature !=3D EFI_FVH_SIGNATURE) > + || (FwVolHeader->FvLength !=3D 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, &gEfiSystemNvDataFvGui= d)) { > [SAMI] Not a comment for this patch but I noticed that BaseTools has an i= mplementation of CompareGuid() that returns INTN. > I wonder if that is intentional. Moreover, it also appears that the Compa= reGuid() usage in BaseTools does not consistently follow the explicit compa= rison rule (https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specific= ation/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 =3D CalculateSum16((UINT16*)FwVolHeader, FwVolHeader->HeaderL= ength); > + if (Checksum !=3D 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 Fi= rmware Volume header. > So, should the error code returned here be EFI_VOLUME_CORRUPTED? Would th= e 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 =3D (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 =3D EFI_FVH_REVISION; > + FirmwareVolumeHeader->BlockMap[0].NumBlocks =3D Instance->NBlocks; > + FirmwareVolumeHeader->BlockMap[0].Length =3D Instance->BlockSize; > + FirmwareVolumeHeader->BlockMap[1].NumBlocks =3D 0; > + FirmwareVolumeHeader->BlockMap[1].Length =3D 0; > + FirmwareVolumeHeader->Checksum =3D CalculateCheckSum16 ( > + (UINT16*)FirmwareVolumeHeader, > + FirmwareVolumeHeader->HeaderLength)= ; > + > + // > + // VARIABLE_STORE_HEADER > + // > + VariableStoreHeader =3D (VOID *)((UINTN)Headers + > [SAMI] Should the typecase be (VARIABLE_STORE_HEADER*) instead of (VOID *= )? > [/SAMI] Yes > + FirmwareVolumeHeader->HeaderLength); > + CopyGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableG= uid); > + VariableStoreHeader->Size =3D PcdGet32(PcdFlashNvStorageVariableSize) = - > + FirmwareVolumeHeader->HeaderLength; > + VariableStoreHeader->Format =3D VARIABLE_STORE_FORMATTED; > + VariableStoreHeader->State =3D VARIABLE_STORE_HEALTHY; > + > + Status =3D ReadWriteRpmb(SP_SVC_RPMB_WRITE, (UINTN) Headers, HeadersLe= ngth, 0); > + if (Status !=3D 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 =3D=3D 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-boolea= n-do-not-require-explicit-comparisons-to-true-or-false > [/SAMI] Sure > + return EFI_SUCCESS; > + } > + > + // FirmwareVolumeHeader->FvLength is declared to have the Variable are= a > + // AND the FTW working area AND the FTW Spare contiguous. > + ASSERT (PcdGet32 (PcdFlashNvStorageVariableBase) + > + PcdGet32 (PcdFlashNvStorageVariableSize) =3D=3D > + PcdGet32 (PcdFlashNvStorageFtwWorkingBase)); > + ASSERT (PcdGet32 (PcdFlashNvStorageFtwWorkingBase) + > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize) =3D=3D > + PcdGet32 (PcdFlashNvStorageFtwSpareBase)); > + > + // Check if the size of the area is at least one block size > + ASSERT ((PcdGet32 (PcdFlashNvStorageVariableSize) > 0) && > + (PcdGet32 (PcdFlashNvStorageVariableSize) / Instance->BlockSiz= e > 0)); > + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > 0) && > + (PcdGet32 (PcdFlashNvStorageFtwWorkingSize) / Instance->BlockS= ize > 0)); > + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwSpareSize) > 0) && > + (PcdGet32 (PcdFlashNvStorageFtwSpareSize) / Instance->BlockSiz= e > 0)); > + > + // Ensure the Variable areas are aligned on block size boundaries > + ASSERT ((PcdGet32 (PcdFlashNvStorageVariableBase) % Instance->BlockSiz= e) =3D=3D 0); > + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwWorkingBase) % Instance->BlockS= ize) =3D=3D 0); > + ASSERT ((PcdGet32 (PcdFlashNvStorageFtwSpareBase) % Instance->BlockSiz= e) =3D=3D 0); > [SAMI] These asserts may need to be adjusted if 64-bit versions of the PC= Ds are used. > [/SAMI] Noted > + > + // Read the file from disk and copy it to memory > + ReadEntireFlash (Instance); > + > + FwVolHeader =3D (EFI_FIRMWARE_VOLUME_HEADER *) Instance->MemBaseAddres= s; > + Status =3D 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 * Inst= ance->BlockSize, ~0UL); > + DEBUG ((DEBUG_INFO, "%a: Erasing Flash.\n", __FUNCTION__)); > + Status =3D ReadWriteRpmb(SP_SVC_RPMB_WRITE, Instance->MemBaseAddress= , > + PcdGet32(PcdFlashNvStorageVariableSize) + > + PcdGet32(PcdFlashNvStorageFtwWorkingSize) + > + PcdGet32(PcdFlashNvStorageFtwSpareSize), 0); > + if (Status !=3D 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 =3D InitializeFvAndVariableStoreHeaders (Instance); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + } else { > + DEBUG ((DEBUG_INFO, "%a: Found valid FVB Header.\n", __FUNCTION__)); > + } > + Instance->Initialized =3D 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 =3D PcdGet32(PcdFlashNvStorageVariableSize) + > + PcdGet32(PcdFlashNvStorageFtwWorkingSize) + > + PcdGet32(PcdFlashNvStorageFtwSpareSize); > + > + NBlocks =3D EFI_SIZE_TO_PAGES(ALIGN_VARIABLE(FvLength)); > + Addr =3D AllocatePages(NBlocks); > + ASSERT (Addr !=3D NULL); > [SAMI] Asserts will vanish in release builds and a failure to allocate me= mory 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 =3D OpTeeRpmbFvbGetPhysicalAd= dress; > + mInstance.FvbProtocol.GetAttributes =3D OpTeeRpmbFvbGetAttributes= ; > + mInstance.FvbProtocol.SetAttributes =3D OpTeeRpmbFvbSetAttributes= ; > + mInstance.FvbProtocol.GetBlockSize =3D OpTeeRpmbFvbGetBlockSize; > + mInstance.FvbProtocol.EraseBlocks =3D OpTeeRpmbFvbErase; > + mInstance.FvbProtocol.Write =3D OpTeeRpmbFvbWrite; > + mInstance.FvbProtocol.Read =3D OpTeeRpmbFvbRead; > + > + mInstance.MemBaseAddress =3D (EFI_PHYSICAL_ADDRESS) Addr; > + mInstance.Signature =3D FLASH_SIGNATURE; > + mInstance.Initialize =3D FvbInitialize; > + mInstance.BlockSize =3D EFI_PAGE_SIZE; > + mInstance.NBlocks =3D 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\NorFl= ashDxe. > [/SAMI] Noted > + PatchPcdSet32 (PcdFlashNvStorageFtwWorkingBase, mInstance.MemBaseAddre= ss + > + PcdGet32 (PcdFlashNvStorageVariableSize)); > + PatchPcdSet32 (PcdFlashNvStorageFtwSpareBase, mInstance.MemBaseAddress= + > + PcdGet32 (PcdFlashNvStorageVariableSize) + > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)); > + > + Status =3D 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 conf= idential and may also be privileged. If you are not the intended recipient,= please notify the sender immediately and do not disclose the contents to a= ny other person, use it for any purpose, or store or copy the information i= n any medium. Thank you.