From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2447921A04800 for ; Fri, 31 Mar 2017 04:29:34 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 86C5B1556C; Fri, 31 Mar 2017 11:29:33 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 86C5B1556C Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 86C5B1556C Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-31.phx2.redhat.com [10.3.116.31]) by smtp.corp.redhat.com (Postfix) with ESMTP id 766565C6F3; Fri, 31 Mar 2017 11:29:32 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org References: <20170331105607.3477-1-ard.biesheuvel@linaro.org> <20170331105607.3477-3-ard.biesheuvel@linaro.org> Cc: leif.lindholm@linaro.org From: Laszlo Ersek Message-ID: <42b6fd83-1b5c-4d6e-ff76-24e13d14c3a3@redhat.com> Date: Fri, 31 Mar 2017 13:29:31 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170331105607.3477-3-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Fri, 31 Mar 2017 11:29:33 +0000 (UTC) Subject: Re: [PATCH v2 2/3] EmbeddedPkg: add base DtPlatformDtbLoaderLib implementation X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 31 Mar 2017 11:29:34 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/31/17 12:56, Ard Biesheuvel wrote: > Introduce an implementation of the new DtPlatformDtbLoaderLib library > class that simply retrieves the first raw section for an FV file named s/for/of/ ? > gDtPlatformDefaultDtbFileGuid. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > EmbeddedPkg/EmbeddedPkg.dsc | 4 ++ > EmbeddedPkg/Library/DtPlatformDtbLoaderBaseLib/DtPlatformDtbLoaderBaseLib.c | 60 ++++++++++++++++++++ > EmbeddedPkg/Library/DtPlatformDtbLoaderBaseLib/DtPlatformDtbLoaderBaseLib.inf | 36 ++++++++++++ > 3 files changed, 100 insertions(+) > > diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc > index ba4f1ea0f004..0d5db68631bb 100644 > --- a/EmbeddedPkg/EmbeddedPkg.dsc > +++ b/EmbeddedPkg/EmbeddedPkg.dsc > @@ -109,6 +109,9 @@ [LibraryClasses.common] > HiiLib|MdeModulePkg/Library/UefiHiiLib/UefiHiiLib.inf > UefiHiiServicesLib|MdeModulePkg/Library/UefiHiiServicesLib/UefiHiiServicesLib.inf > > + DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf > + DtPlatformDtbLoaderLib|EmbeddedPkg/Library/DtPlatformDtbLoaderBaseLib/DtPlatformDtbLoaderBaseLib.inf > + > [LibraryClasses.common.DXE_DRIVER] > PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf > ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf > @@ -247,6 +250,7 @@ [Components.common] > EmbeddedPkg/Library/TemplateRealTimeClockLib/TemplateRealTimeClockLib.inf > EmbeddedPkg/Library/LzmaHobCustomDecompressLib/LzmaHobCustomDecompressLib.inf > EmbeddedPkg/Library/NullDmaLib/NullDmaLib.inf > + EmbeddedPkg/Library/DtPlatformDtbLoaderBaseLib/DtPlatformDtbLoaderBaseLib.inf > > EmbeddedPkg/Ebl/Ebl.inf > #### EmbeddedPkg/EblExternCmd/EblExternCmd.inf > diff --git a/EmbeddedPkg/Library/DtPlatformDtbLoaderBaseLib/DtPlatformDtbLoaderBaseLib.c b/EmbeddedPkg/Library/DtPlatformDtbLoaderBaseLib/DtPlatformDtbLoaderBaseLib.c > new file mode 100644 > index 000000000000..313d0b104681 > --- /dev/null > +++ b/EmbeddedPkg/Library/DtPlatformDtbLoaderBaseLib/DtPlatformDtbLoaderBaseLib.c > @@ -0,0 +1,60 @@ > +/** @file > +* > +* Copyright (c) 2017, Linaro, Ltd. All rights reserved. > +* > +* This program and the accompanying materials > +* are licensed and made available under the terms and conditions of the BSD License > +* which accompanies this distribution. The full text of the license may be found at > +* http://opensource.org/licenses/bsd-license.php > +* > +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +* > +**/ > + > +#include > + > +#include > +#include > +#include > + > +/** > + Return a pool allocated copy of the DTB image that is appropriate for > + booting the current platform via DT. > + > + @param[out] Dtb Pointer to the DTB copy > + @param[out] DtbSize Size of the DTB copy > + > + @retval EFI_SUCCESS Operation completed successfully > + @retval EFI_NOT_FOUND No suitable DTB image could be located > + @retval EFI_OUT_OF_RESOURCES No pool memory available > + > +**/ > +EFI_STATUS > +EFIAPI > +DtPlatformLoadDtb ( > + OUT VOID **Dtb, > + OUT UINTN *DtbSize > + ) > +{ > + EFI_STATUS Status; > + VOID *OrigDtb; > + VOID *CopyDtb; > + UINTN OrigDtbSize; > + > + Status = GetSectionFromAnyFv (&gDtPlatformDefaultDtbFileGuid, > + EFI_SECTION_RAW, 0, &OrigDtb, &OrigDtbSize); > + if (EFI_ERROR (Status)) { > + return EFI_NOT_FOUND; > + } > + > + CopyDtb = AllocateCopyPool (OrigDtbSize, OrigDtb); > + if (CopyDtb == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + *Dtb = CopyDtb; > + *DtbSize = OrigDtbSize; > + > + return EFI_SUCCESS; > +} > diff --git a/EmbeddedPkg/Library/DtPlatformDtbLoaderBaseLib/DtPlatformDtbLoaderBaseLib.inf b/EmbeddedPkg/Library/DtPlatformDtbLoaderBaseLib/DtPlatformDtbLoaderBaseLib.inf > new file mode 100644 > index 000000000000..7c337aace3ef > --- /dev/null > +++ b/EmbeddedPkg/Library/DtPlatformDtbLoaderBaseLib/DtPlatformDtbLoaderBaseLib.inf > @@ -0,0 +1,36 @@ > +/** @file > +* > +* Copyright (c) 2017, Linaro, Ltd. All rights reserved. > +* > +* This program and the accompanying materials > +* are licensed and made available under the terms and conditions of the BSD License > +* which accompanies this distribution. The full text of the license may be found at > +* http://opensource.org/licenses/bsd-license.php > +* > +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +* > +**/ > + > +[Defines] > + INF_VERSION = 0x00010019 > + BASE_NAME = DtPlatformDtbLoaderBaseLib > + FILE_GUID = 419a1910-70da-4c99-8696-ba81a57be508 > + MODULE_TYPE = DXE_DRIVER > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = DtPlatformDtbLoaderLib|DXE_DRIVER > + > +[Sources] > + DtPlatformDtbLoaderBaseLib.c > + > +[Packages] > + MdePkg/MdePkg.dec > + EmbeddedPkg/EmbeddedPkg.dec > + > +[LibraryClasses] > + BaseLib > + DxeServicesLib > + MemoryAllocationLib > + > +[Guids] > + gDtPlatformDefaultDtbFileGuid > This patch looks good, but I think the naming isn't right. Generally we form the names of library instances in the following pattern: + + For example, if you have a library class called FooBarLib, then the do-nothing instance is usually: Base + FooBarLib + Null, that is, BaseFoobarLibNull. Examples: $ ls -1d Mde{,Module}Pkg/Library/Base*LibNull MdeModulePkg/Library/BaseIpmiLibNull MdeModulePkg/Library/BasePlatformHookLibNull MdeModulePkg/Library/BaseResetSystemLibNull MdePkg/Library/BaseDebugLibNull MdePkg/Library/BasePalLibNull MdePkg/Library/BasePcdLibNull MdePkg/Library/BasePeCoffExtraActionLibNull MdePkg/Library/BasePerformanceLibNull MdePkg/Library/BaseReportStatusCodeLibNull MdePkg/Library/BaseS3BootScriptLibNull MdePkg/Library/BaseSerialPortLibNull MdePkg/Library/BaseSmbusLibNull The first component, can be Base (suitable for all modules), Pei, Dxe, Uefi, or a combination thereof. $ ls -1d Mde{,Module}Pkg/Library/Pei*Lib* $ ls -1d Mde{,Module}Pkg/Library/Dxe*Lib* $ ls -1d Mde{,Module}Pkg/Library/Uefi*Lib* Finally, the last component of the lib instance name is usually a hint to the core implementation trait of the library instance, the one that basically sets it apart from all other lib instances of the same class. Therefore, I suggest the following name for this instance: DxeDtPlatformDtbLoaderLibFfsFileSection0 Feel free to replace "FfsFileSection0" with something less gross, if you like. With the commit message and the lib instance name updated (please locate all occurrences -- file names, file contents, and commit message too): Reviewed-by: Laszlo Ersek Thanks! Laszlo