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

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

On Fri, Mar 05, 2021 at 05:58:49PM +0000, Pierre wrote:
> Hi Ilias,
> Here is the rest of the review. Sorry to do it in 2 times.

No worries, I'll try to pick up all the comments.

>
> 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.

Yea, but I'll remove the overflow check on v6 so that should be fine as-is.

> >
> > +
> >
> > + **/
> >
> > +EFI_STATUS
> >
> > +EFIAPI
> >
> >

[...]

> > +  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.

ditto

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

The call is technically an FFA one but at the moment OP-TEE returns the StMM
return code which is defined in the last header you mention.
The relevant code is in ./core/arch/arm/kernel/stmm_sp.c function
tee2stmm_ret_val().
So unless we redefine that in OP-TEE or (better imho), wait for a full FFA
mechanism to be in place, I'd prefer leaving it as is.
Keep in mind that adding the full FFA will also get rid of the hardcoded IDs
on the beginning of the file.

> >
> > +  SvcArgs.Arg1 = mStorageId;
> > +  //

[...]

> >
> > +  if (   (FwVolHeader->Revision  != EFI_FVH_REVISION)
> >
> > +      || (FwVolHeader->Signature != EFI_FVH_SIGNATURE)
> >
> > +      || (FwVolHeader->FvLength  != FvLength)
> >
> > +      )
> could be on the same line -> ') {'

ok

> >
> > +  {
> >
> >
> > +  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

ok

> > +

> >
> > +    (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).

Yea it seems so. This was again a c/p from other drivers handling the
PCD, but we can start
clean here.

> >
> > +    (PcdGet32 (PcdFlashNvStorageVariableSize) / Instance->BlockSize > 0)
> >
> > +    );
> >
> > +  ASSERT (
> >
> > +    (PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > 0) &&


[...]

> > +
> >
> > +  SetMem (&mInstance, sizeof (mInstance), 0);
> NIT: you can use ZeroMem()

Sure

> >
> > +
> >
> > +  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.

Yea it's not. I'll probably remove the same piece of code from the
FixupPcd.c as well.

> >
> > +    );
> 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().
> >

I think it is.  There's something 'special' about this driver. The
upper EDK2 lays expect a byte-addressable interface.
So the PCDs are definitely not persistent, they are patchable in module PCDs.
Since RPMB isn't one we are allocating memory and handing that memory
over to EDK2, while syncing the background on the hardware.
Again, it's been a while since I wrote this and memory might be
failing, but I think what's happening here is:

OpTeeRpmbFvbInit is the entry point, which calls in EDK2 code, so you
need to patch them in here, before calling anything. After that the
fixup code runs before calling into the module and fixes up the values
for us.
In any case I'll double check before posting a v6

Thanks
/Ilias

[...]