From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f42.google.com (mail-wr1-f42.google.com [209.85.221.42]) by mx.groups.io with SMTP id smtpd.web11.3637.1613137064170835032 for ; Fri, 12 Feb 2021 05:37:44 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=zPht8xK9; spf=pass (domain: linaro.org, ip: 209.85.221.42, mailfrom: ilias.apalodimas@linaro.org) Received: by mail-wr1-f42.google.com with SMTP id v14so7938413wro.7 for ; Fri, 12 Feb 2021 05:37:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=M0XTZigfFneC1Ow3X6TlhMZtJRACoXZ+KQxGi1uq+Co=; b=zPht8xK9ywXNwuesnG9XzPzy2A6/vHiJIO6up025WPhwN5FmWyLTjurw9zZuCoVy1N 6jJJvW8AWhxoTHPL1wSusd24Ifzsc/7Ptz8q4CRG71SED/C50kZoUTsrUKhfkvgg/5U3 sFJtL0PixqIErnR7CvgwWLicUhju0GMwI6GX76EnwOmIucFwwbr7VCEVyE/lD7CuvcJH 7NadjgNTa+cQ400NzH8/FqhCi6jr4q9BDy6+4xSN+YIBz+238xjMVELacgjq96gj7ZOP dIrBzJ61takgP/7wweMeU2XSAckDahp+KBDREBvXUhxS8YS4VlFDYiouByiobfU7910w WJ4g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=M0XTZigfFneC1Ow3X6TlhMZtJRACoXZ+KQxGi1uq+Co=; b=GtLce92BQOJoMBk0Lm3f0P8EVZ8MZmBHgyuv/DoPoc2+62W+gPUQoq5gO6YDSweU8k avCsn4IFMTZsLN4/hMMYmX7QwZCIYoQyAr0Sv57CqbvZ7brzriBehxsaU4AjnGNV8wjx AnYz3cYmGFbi13XlNU2SftEEMNTfzyeTsmEUXbCbUlgaRdeZHlRW5e0nheY+XYwI2rrH tdhMQSqzixk42LTCcwBRccAs4kQ6U29Qy4vbiWyzqiXl+esWDC0Vv5Lg34L8Yr0XhWWy iM6HFU04BOwIfT3Hv/AaCEpB1roq7wCTJ//PhzRCk5gMyUcsmRQKU6vBZhw8UWliOMrg x3aQ== X-Gm-Message-State: AOAM533kMjC5s+2P7VenNV/0k4pAvpawDOUR0QW9Y6qEcTYYaUTBMmCE J+bMSnANcZpyZfi2Yxxj8ipG6w== X-Google-Smtp-Source: ABdhPJwf/e/x5P6utI5n+sY5D2ffCHHrOpHQQPtVD5nP8/9GBn+GeS0tSab3xHtZ0RG7HGHPmarS2w== X-Received: by 2002:a5d:620d:: with SMTP id y13mr3530905wru.88.1613137062702; Fri, 12 Feb 2021 05:37:42 -0800 (PST) Return-Path: Received: from apalos.home (ppp-94-64-113-158.home.otenet.gr. [94.64.113.158]) by smtp.gmail.com with ESMTPSA id 64sm12762018wrc.50.2021.02.12.05.37.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 12 Feb 2021 05:37:42 -0800 (PST) Date: Fri, 12 Feb 2021 15:37:39 +0200 From: "Ilias Apalodimas" 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 Message-ID: References: <20210203101626.1200943-1-ilias.apalodimas@linaro.org> <20210203101626.1200943-2-ilias.apalodimas@linaro.org> MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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