From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yb1-f170.google.com (mail-yb1-f170.google.com [209.85.219.170]) by mx.groups.io with SMTP id smtpd.web09.5441.1613144171759486553 for ; Fri, 12 Feb 2021 07:36:12 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=B1+F0HOk; spf=pass (domain: linaro.org, ip: 209.85.219.170, mailfrom: ilias.apalodimas@linaro.org) Received: by mail-yb1-f170.google.com with SMTP id x19so6735422ybe.0 for ; Fri, 12 Feb 2021 07:36:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=EA6YfBAMsFdFssU5Gt15lvNoEmRQYMOun2X0/ISaVLg=; b=B1+F0HOkI+DriHn1DPHzY3fYZmWsUi6yEjWM6ciJYQB2ERuWdIQClUkoWJ+YQ8Ukbn v0A7SIZhtFh9hy2GtQoTRC9TL3zSnsd8LSpsbDT7zIrMG6+FE/dY9V6BZw+gb6lq0QXw tWV11PCXPFGupKJIZVNuk9u5dZSRpOzccCrVFyIL2YFF8B0hca2Vf/UM//s84GTGXJqZ 3bYwzJEWxI0za1RxHf4AaRm7VXa5c/cX/o8KhEPWrGXogAqqwJIceUY+zw/yYD+nFQN/ 7tx4dB08JUt1jtaKtMfXuQekeMLB3k0Y9cxjXolMYmFR8r3oLvHNxu+d6+tOjx7bwlce ncDA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=EA6YfBAMsFdFssU5Gt15lvNoEmRQYMOun2X0/ISaVLg=; b=a12snNaT8TyHJOjjbghPSMBUBnCc37IKsIVnXy48uFswzr2tI8GF1fT1zRRIo6ZJAu oIIY59kKE7CP0T3jPX51e/fHKjOVUJ1DZV8m9iUIT7d2Dx9qARILB4bHzqNX5Q16RSAI nErGY3yOqwv3DK72i0RTizkaoicDW6Z7DS3DNCSLkz9O6tqxWRcCKo+18ze9qwv0YxVt +JyzFDpIxtkXOa2dEWXv00eTIzwgZt4JL8HB82F8XSLQwZKiN8oEjai34N3HI/dqS0l2 wMOJslxUFacY4keoBoT2CHIyi/a8nTEktcwUyw+8uBOuWyqr1LSCjXMUSKPhKZ8nSUbL 9tRg== X-Gm-Message-State: AOAM530PoZmeGaC/IH30k2w2RZPZkXyL2m3pCSa4OyTzIHu7eA8e0lv5 /psTjAxZMqe0BHLD0ZvmkfAXuJM01sDW5zGxFGRNBA== X-Google-Smtp-Source: ABdhPJzBy9IwiRKoWHNrLLxYzarB9HmHrwJNglxt0MOxnjPyjLAVm5YRdSkvXnYzNI6AUAqanI4L7sBGFs0Gq6DcxzE= X-Received: by 2002:a25:6dc3:: with SMTP id i186mr4588005ybc.421.1613144170904; Fri, 12 Feb 2021 07:36:10 -0800 (PST) MIME-Version: 1.0 References: <20210203101626.1200943-1-ilias.apalodimas@linaro.org> <20210203101626.1200943-2-ilias.apalodimas@linaro.org> In-Reply-To: From: "Ilias Apalodimas" Date: Fri, 12 Feb 2021 17:35:34 +0200 Message-ID: Subject: Re: [PATCH 1/2 v4] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver To: Sami Mujawar Cc: "devel@edk2.groups.io" , "ardb+tianocore@kernel.org" , "sughosh.ganu@linaro.org" , "leif@nuviainc.com" , nd Content-Type: text/plain; charset="UTF-8" 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 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 > Sent: 12 February 2021 01:38 PM > To: Sami Mujawar > Cc: devel@edk2.groups.io; ard+tianocore@kernel.org; sughosh.ganu@linaro.org; leif@nuviainc.com; nd > 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