Re: [PATCH v2 2/3] EmbeddedPkg: add base DtPlatformDtbLoaderLib implementation

[Laszlo Ersek <lersek@redhat.com>]

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 <ard.biesheuvel@linaro.org>
> ---
>  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 <PiDxe.h>
> +
> +#include <Library/BaseLib.h>
> +#include <Library/DxeServicesLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +
> +/**
> +  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:

<phase or module type> + <library class name> + <implementation details>

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, <phase or module type> 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 <lersek@redhat.com>

Thanks!
Laszlo