[edk2-devel] [PATCH 1/2 v4] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver
Sami Mujawar
sami.mujawar at arm.com
Fri Feb 12 15:28:57 UTC 2021
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 at linaro.org>
Sent: 12 February 2021 01:38 PM
To: Sami Mujawar <Sami.Mujawar at arm.com>
Cc: devel at edk2.groups.io; ard+tianocore at kernel.org; sughosh.ganu at linaro.org; leif at nuviainc.com; nd <nd at 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71645): https://edk2.groups.io/g/devel/message/71645
Mute This Topic: https://groups.io/mt/80350216/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list