[edk2-devel] [PATCH 1/2 v4] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver
Ilias Apalodimas
ilias.apalodimas at linaro.org
Fri Feb 12 15:35:34 UTC 2021
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 at 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 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 (#71646): https://edk2.groups.io/g/devel/message/71646
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