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;
+}
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72505): https://edk2.groups.io/g/devel/message/72505
Mute This Topic: https://groups.io/mt/80588994/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
