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

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

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