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

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

Thanks Sami,

I managed to run Ecc properly now, so it already reported that to me

On Fri, 12 Feb 2021 at 17:29, Sami Mujawar <Sami.Mujawar@arm.com> wrote:
>
> Hi Ilias,
>
> > +#ifndef OPTEE_RPMB_FVB_H
> > +#define OPTEE_RPMB_FVB_H
> Just remembered the include file guard should be 'OPTEE_RPMB_FVB_H_' in Drivers/OpTeeRpmb/OpTeeRpmbFvb.h
>
> Regards,
>
> Sami Mujawar
>
> -----Original Message-----
> From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Sent: 12 February 2021 01:38 PM
> To: Sami Mujawar <Sami.Mujawar@arm.com>
> Cc: devel@edk2.groups.io; ard+tianocore@kernel.org; sughosh.ganu@linaro.org; leif@nuviainc.com; nd <nd@arm.com>
> Subject: Re: [PATCH 1/2 v4] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver
>
> Hi Sami,
>
> On Fri, Feb 12, 2021 at 01:11:10PM +0000, Sami Mujawar wrote:
> > Hi Ilias,
> >
> > Apologies for the delay in reviewing this patch.
> >
>
> No worries, thanks for the comments
>
> > Please find my response inline marked [SAMI].
> > There are some coding standard issues remaining. I believe these are not reported by Ecc.
>
> Yea Ecc or PatchCheck didn't catch any of these.
>
> > Also, InitializeFvAndVariableStoreHeaders() would need error handling for a memory allocation failure.
> >
> > Regards,
> >
> > Sami Mujawar
> >
> >
>
> [...]
>
> > +  Instance = INSTANCE_FROM_FVB_THIS(FvbProtocol);
> > [SAMI] Space needed before opening parenthesis.
> >  See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-2-6-always-put-space-before-an-open-parenthesis
> > Please also check other places in this patch.
>
> Ok
>
> > [/SAMI]
> >
> > +  // The Pcd is user defined, so make sure we don't overflow
> >
> > +  if (Instance->MemBaseAddress > MAX_UINT64 - PcdGet32 (PcdFlashNvStorageVariableSize)) {
> >
> > +    return EFI_INVALID_PARAMETER;
> >
> > +  }
> >
> > +
> >
> > +  if (Instance->MemBaseAddress > MAX_UINT64 - PcdGet32 (PcdFlashNvStorageVariableSize) -
> >
> > +    PcdGet32 (PcdFlashNvStorageFtwWorkingSize)) {
> >
> > +    return EFI_INVALID_PARAMETER;
> >
> > +  }
> >
> > +
> >
> > +  // Patch PCDs with the the correct values
> >
> > +  PatchPcdSet64 (PcdFlashNvStorageVariableBase64, Instance->MemBaseAddress);
> >
> > +  PatchPcdSet64 (PcdFlashNvStorageFtwWorkingBase64, Instance->MemBaseAddress +
> >
> > +    PcdGet32 (PcdFlashNvStorageVariableSize));
> > [SAMI] Please see alignment guidelines described in EDKII coding standard at:
> >  https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-2-4-subsequent-lines-of-multi-line-function-calls-should-line-up-two-spaces-from-the-beginning-of-the-function-name
> > Please also fix this at other places in this patch.
>
> Ok
>
> > [/SAMI]
> >
>
> [...]
>
> > +[Pcd]
> >
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
> >
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
> >
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64
> >
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
> >
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64
> >
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
> > [SAMI] Please sort in alphabetical order.
> > [/SAMI]
>
> Ok
>
> >
> [...]
> >
> > +
> >
> > +[Pcd]
> >
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
> >
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
> >
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64
> >
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
> >
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64
> >
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
> > [SAMI] Please sort in alphabetical order.
> > [/SAMI]
>
> Ok
>
> >
> > +
> >
> > +OpTeeRpmbFvbSetAttributes (
> >
> > +  IN CONST  EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
> >
> > +  IN OUT    EFI_FVB_ATTRIBUTES_2                *Attributes
> >
> > +  )
> >
> > +{
> >
> > +  return EFI_SUCCESS;  // ignore for now
> > [SAMI] Should this be EFI_UNSUPPORTED? If not, a comment may be helpful.
>
> Some of the drivers I looked were returning EFI_SUCCESS.
> Checking again I see some returning EFI_UNSUPPORTED as well, so I'll switch to
> that.
>
> > [/SAMI]
> >
> > +}
>
> [...]
>
> >
> > +  *NumberOfBlocks = Instance->NBlocks - (UINTN) Lba;
> > [SAMI] There should be no space between (UINTN) and Lba.
> > Please see point 2 at https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/v/release%2F2.20/3_quick_reference#3-2-3-formatting-horizontal-spacing
> > [/SAMI]
>
> Ok
>
> >
> > +  *BlockSize = Instance->BlockSize;
> >
> > +
>
> [...]
>
> > +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 = EFI_SUCCESS;
> >
> > +  MEM_INSTANCE *Instance;
> >
> > +  VOID         *Base;
> >
> > +
> >
> > +  Instance = INSTANCE_FROM_FVB_THIS(This);
> >
> > +  if (!Instance->Initialized) {
> >
> > +    Instance->Initialize (Instance);
> > [SAMI] Should the status be checked here and returned?
> > Or is the assumption that Initialize will always succeed. If so,
> >      - a comment would be helpful.
> >      - the Status variable here is redundant.
> > Same comment for OpTeeRpmbFvbWrite().
>
> No you are right 'Status =' is missing, I'll add that.
>
> > [/SAMI]
> > +  }
> >
> > +
> >
> > +  Base = (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize + Offset;
> > [SAMI] Use of parentheses is encouraged. See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-2-10-use-extra-parentheses-rather-than-depending-on-in-depth-knowledge-of-the-order-of-precedence-of-c
> > [/SAMI]
>
> ok
>
> [...]
> >
> > +    }
> >
> > +    NumBytes = NumLba * Instance->BlockSize;
> >
> > +    Base = (VOID *)Instance->MemBaseAddress + Start * Instance->BlockSize;
> >
> > +    Buf = AllocatePool(NumLba * Instance->BlockSize);
> >
> > +    if (!Buf) {
> > [SAMI] if (Buf == NULL) {
> > [/SAMI]
>
> Ok
>
> >
> > +      return EFI_DEVICE_ERROR;
> >
>
> [...]
>
> > +EFI_STATUS
> >
> > +InitializeFvAndVariableStoreHeaders (
> >
> > +  MEM_INSTANCE *Instance
> >
> > +  )
> >
> > +{
> >
> > +  EFI_FIRMWARE_VOLUME_HEADER *FirmwareVolumeHeader;
> >
> > +  VARIABLE_STORE_HEADER      *VariableStoreHeader;
> >
> > +  EFI_STATUS                 Status = EFI_SUCCESS;
> >
> > +  UINTN                      HeadersLength;
> >
> > +  VOID*                      Headers;
> >
> > +
> >
> > +  HeadersLength = sizeof(EFI_FIRMWARE_VOLUME_HEADER) +
> >
> > +                  sizeof(EFI_FV_BLOCK_MAP_ENTRY) +
> >
> > +                  sizeof(VARIABLE_STORE_HEADER);
> >
> > +  Headers = AllocateZeroPool(HeadersLength);
> > [SAMI] Error handling for allocation failure is needed here.
> > [/SAMI]
>
> Yes missed that
>
> >
>
> [...]
>
> > +  //
> > [SAMI] ASSERT (Addr != NULL); could be removed from the line above an instead ASSERT (0); could be added here.
> > [/SAMI]
>
> Ok
>
> > +    return EFI_OUT_OF_RESOURCES;
> > +
>
> I'll fix those and send a v5.
>
> Thanks!
> /Ilias