public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "PierreGondois" <pierre.gondois@arm.com>
To: devel@edk2.groups.io,
	Ilias Apalodimas <ilias.apalodimas@linaro.org>,
	Sami.Mujawar@arm.com
Subject: Re: [edk2-devel] [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver
Date: Fri, 5 Mar 2021 17:58:49 +0000	[thread overview]
Message-ID: <ade96a28-d4c9-c1f2-0ff1-77b425189015@arm.com> (raw)
In-Reply-To: <20210212173459.508205-2-ilias.apalodimas@linaro.org>

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;
>
> +}


  parent reply	other threads:[~2021-03-05 17:59 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-12 17:34 [PATCH 0/2 v5] Add support for running StandaloneMm as OP-TEE TA Ilias Apalodimas
2021-02-12 17:34 ` [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver Ilias Apalodimas
2021-03-03 10:20   ` [edk2-devel] " PierreGondois
2021-03-03 20:02     ` Ilias Apalodimas
2021-03-05 16:08       ` PierreGondois
2021-03-05 17:58   ` PierreGondois [this message]
2021-03-08 23:33     ` Ilias Apalodimas
2021-02-12 17:34 ` [PATCH 2/2 v5] StMMRpmb: Add support for building StandaloneMm image for OP-TEE Ilias Apalodimas
2021-03-03 10:18   ` [edk2-devel] " PierreGondois
2021-03-03 19:24     ` Ilias Apalodimas
2021-03-05 18:07       ` PierreGondois
2021-03-08 11:16         ` Ilias Apalodimas
2021-03-03 11:32 ` [edk2-devel] [PATCH 0/2 v5] Add support for running StandaloneMm as OP-TEE TA Sami Mujawar
2021-03-08 15:57   ` Leif Lindholm

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ade96a28-d4c9-c1f2-0ff1-77b425189015@arm.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox