[PATCH v2 0/3] EmbeddedPkg: revert DTB loading to platform lib

[PATCH v2 0/3] EmbeddedPkg: revert DTB loading to platform lib

[Ard Biesheuvel]

This updates the recently added DtPlatformDxe to indirect loading of the
actual DTB binary to a separate library. A base implementation is provided
that preserves the original behavior.

This way, we can allow platforms such as FVP or Juno (which use the same
firmware image but different DTB images) to override this library class
and insert its own logic to select the correct DTB, without resorting to
BEFORE depexes and dynamic PCDs, and the associated load order issues.

v2: - drop ArmPlatformPkg changes for now

Ard Biesheuvel (3):
  EmbeddedPkg: add DtPlatformDtbLoaderLib library class
  EmbeddedPkg: add base DtPlatformDtbLoaderLib implementation
  EmbeddedPkg/DtPlatformDxe: load platform DTB via new library

 EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c                             | 26 ++++-----
 EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf                           |  2 +-
 EmbeddedPkg/EmbeddedPkg.dec                                                   |  1 +
 EmbeddedPkg/EmbeddedPkg.dsc                                                   |  6 ++
 EmbeddedPkg/Include/Library/DtPlatformDtbLoaderLib.h                          | 37 ++++++++++++
 EmbeddedPkg/Library/DtPlatformDtbLoaderBaseLib/DtPlatformDtbLoaderBaseLib.c   | 60 ++++++++++++++++++++
 EmbeddedPkg/Library/DtPlatformDtbLoaderBaseLib/DtPlatformDtbLoaderBaseLib.inf | 36 ++++++++++++
 7 files changed, 154 insertions(+), 14 deletions(-)
 create mode 100644 EmbeddedPkg/Include/Library/DtPlatformDtbLoaderLib.h
 create mode 100644 EmbeddedPkg/Library/DtPlatformDtbLoaderBaseLib/DtPlatformDtbLoaderBaseLib.c
 create mode 100644 EmbeddedPkg/Library/DtPlatformDtbLoaderBaseLib/DtPlatformDtbLoaderBaseLib.inf

-- 
2.9.3

[PATCH v2 1/3] EmbeddedPkg: add DtPlatformDtbLoaderLib library class

[Ard Biesheuvel]

To abstract the way a platform reasons about which DTB is appropriate,
and the way it ultimately supplies the DTB image, introduce a new library
class to encapsulate this functionality.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/EmbeddedPkg.dec                          |  1 +
 EmbeddedPkg/Include/Library/DtPlatformDtbLoaderLib.h | 37 ++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index 871fc5ff4016..0bed2736c8c3 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -45,6 +45,7 @@ [LibraryClasses.common]
   GdbSerialLib|Include/Library/GdbSerialLib.h
   DebugAgentTimerLib|Include/Library/DebugAgentTimerLib.h
 
+  DtPlatformDtbLoaderLib|Include/Library/DtPlatformDtbLoaderLib.h
 
 [Guids.common]
   gEmbeddedTokenSpaceGuid       = { 0xe0d8ca17, 0x4276, 0x4386, { 0xbb, 0x79, 0x48, 0xcb, 0x81, 0x3d, 0x3c, 0x4f }}
diff --git a/EmbeddedPkg/Include/Library/DtPlatformDtbLoaderLib.h b/EmbeddedPkg/Include/Library/DtPlatformDtbLoaderLib.h
new file mode 100644
index 000000000000..bb79d2a190f4
--- /dev/null
+++ b/EmbeddedPkg/Include/Library/DtPlatformDtbLoaderLib.h
@@ -0,0 +1,37 @@
+/** @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.
+*
+**/
+
+#ifndef __DT_PLATFORM_DTB_LOADER_LIB_H__
+#define __DT_PLATFORM_DTB_LOADER_LIB_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
+  );
+
+#endif
-- 
2.9.3

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

[Ard Biesheuvel]

Introduce an implementation of the new DtPlatformDtbLoaderLib library
class that simply retrieves the first raw section for an FV file named
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
-- 
2.9.3

[PATCH v2 3/3] EmbeddedPkg/DtPlatformDxe: load platform DTB via new library

[Ard Biesheuvel]

To give platforms some room to decide which DTB is suitable and where
to load it from, indirect loading of the DTB image via the new
DtPlatformDtbLoaderLib library class.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c   | 26 ++++++++++----------
 EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf |  2 +-
 EmbeddedPkg/EmbeddedPkg.dsc                         |  2 ++
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
index 5778633b4985..c75f088a34e5 100644
--- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
+++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
@@ -15,7 +15,7 @@
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
 #include <Library/DevicePathLib.h>
-#include <Library/DxeServicesLib.h>
+#include <Library/DtPlatformDtbLoaderLib.h>
 #include <Library/HiiLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/UefiBootServicesTableLib.h>
@@ -114,14 +114,12 @@ DtPlatformDxeEntryPoint (
   UINTN                           BufferSize;
   VOID                            *Dtb;
   UINTN                           DtbSize;
-  VOID                            *DtbCopy;
 
   //
   // Check whether a DTB blob is included in the firmware image.
   //
   Dtb = NULL;
-  Status = GetSectionFromAnyFv (&gDtPlatformDefaultDtbFileGuid,
-             EFI_SECTION_RAW, 0, &Dtb, &DtbSize);
+  Status = DtPlatformLoadDtb (&Dtb, &DtbSize);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_WARN, "%a: no DTB blob found, defaulting to ACPI\n",
       __FUNCTION__));
@@ -157,7 +155,7 @@ DtPlatformDxeEntryPoint (
                     EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS,
                     sizeof (DtAcpiPref), &DtAcpiPref);
     if (EFI_ERROR (Status)) {
-      return Status;
+      goto FreeDtb;
     }
   }
 
@@ -172,23 +170,18 @@ DtPlatformDxeEntryPoint (
       DEBUG ((DEBUG_ERROR,
         "%a: failed to install gEdkiiPlatformHasAcpiGuid as a protocol\n",
         __FUNCTION__));
-      return Status;
+      goto FreeDtb;
     }
   } else if (DtAcpiPref.Pref == DT_ACPI_SELECT_DT) {
     //
     // DT was selected: copy the blob into newly allocated memory and install
     // a reference to it as the FDT configuration table.
     //
-    DtbCopy = AllocateCopyPool (DtbSize, Dtb);
-    if (DtbCopy == NULL) {
-      return EFI_OUT_OF_RESOURCES;
-    }
-    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DtbCopy);
+    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, Dtb);
     if (EFI_ERROR (Status)) {
       DEBUG ((DEBUG_ERROR, "%a: failed to install FDT configuration table\n",
         __FUNCTION__));
-      FreePool (DtbCopy);
-      return Status;
+      goto FreeDtb;
     }
   } else {
     ASSERT (FALSE);
@@ -210,4 +203,11 @@ DtPlatformDxeEntryPoint (
   // installed in that case.
   //
   return InstallHiiPages ();
+
+FreeDtb:
+  if (Dtb != NULL) {
+    FreePool (Dtb);
+  }
+
+  return Status;
 }
diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
index b73877a6086b..45dfd9088bf0 100644
--- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
+++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
@@ -40,7 +40,7 @@ [Packages]
 [LibraryClasses]
   BaseLib
   DebugLib
-  DxeServicesLib
+  DtPlatformDtbLoaderLib
   HiiLib
   MemoryAllocationLib
   UefiBootServicesTableLib
diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc
index 0d5db68631bb..b75678276e8d 100644
--- a/EmbeddedPkg/EmbeddedPkg.dsc
+++ b/EmbeddedPkg/EmbeddedPkg.dsc
@@ -294,5 +294,7 @@ [Components.common]
   EmbeddedPkg/Library/PrePiHobLib/PrePiHobLib.inf
   EmbeddedPkg/Library/PrePiMemoryAllocationLib/PrePiMemoryAllocationLib.inf
 
+  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
+
 [Components.IA32, Components.X64, Components.IPF, Components.ARM]
   EmbeddedPkg/GdbStub/GdbStub.inf
-- 
2.9.3

Re: [PATCH v2 1/3] EmbeddedPkg: add DtPlatformDtbLoaderLib library class

[Laszlo Ersek]

On 03/31/17 12:56, Ard Biesheuvel wrote:
> To abstract the way a platform reasons about which DTB is appropriate,
> and the way it ultimately supplies the DTB image, introduce a new library
> class to encapsulate this functionality.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/EmbeddedPkg.dec                          |  1 +
>  EmbeddedPkg/Include/Library/DtPlatformDtbLoaderLib.h | 37 ++++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> index 871fc5ff4016..0bed2736c8c3 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -45,6 +45,7 @@ [LibraryClasses.common]
>    GdbSerialLib|Include/Library/GdbSerialLib.h
>    DebugAgentTimerLib|Include/Library/DebugAgentTimerLib.h
>  
> +  DtPlatformDtbLoaderLib|Include/Library/DtPlatformDtbLoaderLib.h
>  
>  [Guids.common]
>    gEmbeddedTokenSpaceGuid       = { 0xe0d8ca17, 0x4276, 0x4386, { 0xbb, 0x79, 0x48, 0xcb, 0x81, 0x3d, 0x3c, 0x4f }}
> diff --git a/EmbeddedPkg/Include/Library/DtPlatformDtbLoaderLib.h b/EmbeddedPkg/Include/Library/DtPlatformDtbLoaderLib.h
> new file mode 100644
> index 000000000000..bb79d2a190f4
> --- /dev/null
> +++ b/EmbeddedPkg/Include/Library/DtPlatformDtbLoaderLib.h
> @@ -0,0 +1,37 @@
> +/** @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.
> +*
> +**/
> +
> +#ifndef __DT_PLATFORM_DTB_LOADER_LIB_H__
> +#define __DT_PLATFORM_DTB_LOADER_LIB_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
> +  );
> +
> +#endif
> 

Please #include <Uefi/UefiBaseType.h> for the EFI_STATUS type here. With
that change:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

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

[Laszlo Ersek]

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

Re: [PATCH v2 3/3] EmbeddedPkg/DtPlatformDxe: load platform DTB via new library

[Laszlo Ersek]

On 03/31/17 12:56, Ard Biesheuvel wrote:
> To give platforms some room to decide which DTB is suitable and where
> to load it from, indirect loading of the DTB image via the new
> DtPlatformDtbLoaderLib library class.

I think you accidentally the verb in the above sentence :)

> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c   | 26 ++++++++++----------
>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf |  2 +-
>  EmbeddedPkg/EmbeddedPkg.dsc                         |  2 ++
>  3 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
> index 5778633b4985..c75f088a34e5 100644
> --- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
> @@ -15,7 +15,7 @@
>  #include <Library/BaseLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/DevicePathLib.h>
> -#include <Library/DxeServicesLib.h>
> +#include <Library/DtPlatformDtbLoaderLib.h>
>  #include <Library/HiiLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
> @@ -114,14 +114,12 @@ DtPlatformDxeEntryPoint (
>    UINTN                           BufferSize;
>    VOID                            *Dtb;
>    UINTN                           DtbSize;
> -  VOID                            *DtbCopy;
>  
>    //
>    // Check whether a DTB blob is included in the firmware image.
>    //

Please drop this comment; this detail is now abstracted out.

>    Dtb = NULL;
> -  Status = GetSectionFromAnyFv (&gDtPlatformDefaultDtbFileGuid,
> -             EFI_SECTION_RAW, 0, &Dtb, &DtbSize);
> +  Status = DtPlatformLoadDtb (&Dtb, &DtbSize);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_WARN, "%a: no DTB blob found, defaulting to ACPI\n",
>        __FUNCTION__));

This can now fail due to out-of-memory in DtPlatformLoadDtb(), so I
suggest printing Status with %r in the debug message.

> @@ -157,7 +155,7 @@ DtPlatformDxeEntryPoint (
>                      EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS,
>                      sizeof (DtAcpiPref), &DtAcpiPref);
>      if (EFI_ERROR (Status)) {
> -      return Status;
> +      goto FreeDtb;
>      }
>    }
>  
> @@ -172,23 +170,18 @@ DtPlatformDxeEntryPoint (
>        DEBUG ((DEBUG_ERROR,
>          "%a: failed to install gEdkiiPlatformHasAcpiGuid as a protocol\n",
>          __FUNCTION__));
> -      return Status;
> +      goto FreeDtb;
>      }
>    } else if (DtAcpiPref.Pref == DT_ACPI_SELECT_DT) {
>      //
>      // DT was selected: copy the blob into newly allocated memory and install
>      // a reference to it as the FDT configuration table.
>      //
> -    DtbCopy = AllocateCopyPool (DtbSize, Dtb);
> -    if (DtbCopy == NULL) {
> -      return EFI_OUT_OF_RESOURCES;
> -    }
> -    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DtbCopy);
> +    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, Dtb);
>      if (EFI_ERROR (Status)) {
>        DEBUG ((DEBUG_ERROR, "%a: failed to install FDT configuration table\n",
>          __FUNCTION__));
> -      FreePool (DtbCopy);
> -      return Status;
> +      goto FreeDtb;
>      }
>    } else {
>      ASSERT (FALSE);
> @@ -210,4 +203,11 @@ DtPlatformDxeEntryPoint (
>    // installed in that case.
>    //
>    return InstallHiiPages ();
> +
> +FreeDtb:
> +  if (Dtb != NULL) {
> +    FreePool (Dtb);
> +  }
> +
> +  return Status;
>  }
> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
> index b73877a6086b..45dfd9088bf0 100644
> --- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
> @@ -40,7 +40,7 @@ [Packages]
>  [LibraryClasses]
>    BaseLib
>    DebugLib
> -  DxeServicesLib
> +  DtPlatformDtbLoaderLib
>    HiiLib
>    MemoryAllocationLib
>    UefiBootServicesTableLib
> diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc
> index 0d5db68631bb..b75678276e8d 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dsc
> +++ b/EmbeddedPkg/EmbeddedPkg.dsc
> @@ -294,5 +294,7 @@ [Components.common]
>    EmbeddedPkg/Library/PrePiHobLib/PrePiHobLib.inf
>    EmbeddedPkg/Library/PrePiMemoryAllocationLib/PrePiMemoryAllocationLib.inf
>  
> +  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
> +
>  [Components.IA32, Components.X64, Components.IPF, Components.ARM]
>    EmbeddedPkg/GdbStub/GdbStub.inf
> 

Please split off the last hunk (for "EmbeddedPkg/EmbeddedPkg.dsc") to a
separate patch. (You might want to make that the first patch in the
series, saying "we forgot this before, but it's needed to build the
driver at all", or some such.)

With these changes, for both patches:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [PATCH v2 3/3] EmbeddedPkg/DtPlatformDxe: load platform DTB via new library

[Ard Biesheuvel]

On 31 March 2017 at 12:39, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/31/17 12:56, Ard Biesheuvel wrote:
>> To give platforms some room to decide which DTB is suitable and where
>> to load it from, indirect loading of the DTB image via the new
>> DtPlatformDtbLoaderLib library class.
>
> I think you accidentally the verb in the above sentence :)
>

'indirect' was intended as the verb here, but I'll replace it for legibility


>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c   | 26 ++++++++++----------
>>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf |  2 +-
>>  EmbeddedPkg/EmbeddedPkg.dsc                         |  2 ++
>>  3 files changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
>> index 5778633b4985..c75f088a34e5 100644
>> --- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
>> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
>> @@ -15,7 +15,7 @@
>>  #include <Library/BaseLib.h>
>>  #include <Library/DebugLib.h>
>>  #include <Library/DevicePathLib.h>
>> -#include <Library/DxeServicesLib.h>
>> +#include <Library/DtPlatformDtbLoaderLib.h>
>>  #include <Library/HiiLib.h>
>>  #include <Library/MemoryAllocationLib.h>
>>  #include <Library/UefiBootServicesTableLib.h>
>> @@ -114,14 +114,12 @@ DtPlatformDxeEntryPoint (
>>    UINTN                           BufferSize;
>>    VOID                            *Dtb;
>>    UINTN                           DtbSize;
>> -  VOID                            *DtbCopy;
>>
>>    //
>>    // Check whether a DTB blob is included in the firmware image.
>>    //
>
> Please drop this comment; this detail is now abstracted out.
>
>>    Dtb = NULL;
>> -  Status = GetSectionFromAnyFv (&gDtPlatformDefaultDtbFileGuid,
>> -             EFI_SECTION_RAW, 0, &Dtb, &DtbSize);
>> +  Status = DtPlatformLoadDtb (&Dtb, &DtbSize);
>>    if (EFI_ERROR (Status)) {
>>      DEBUG ((DEBUG_WARN, "%a: no DTB blob found, defaulting to ACPI\n",
>>        __FUNCTION__));
>
> This can now fail due to out-of-memory in DtPlatformLoadDtb(), so I
> suggest printing Status with %r in the debug message.
>
>> @@ -157,7 +155,7 @@ DtPlatformDxeEntryPoint (
>>                      EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS,
>>                      sizeof (DtAcpiPref), &DtAcpiPref);
>>      if (EFI_ERROR (Status)) {
>> -      return Status;
>> +      goto FreeDtb;
>>      }
>>    }
>>
>> @@ -172,23 +170,18 @@ DtPlatformDxeEntryPoint (
>>        DEBUG ((DEBUG_ERROR,
>>          "%a: failed to install gEdkiiPlatformHasAcpiGuid as a protocol\n",
>>          __FUNCTION__));
>> -      return Status;
>> +      goto FreeDtb;
>>      }
>>    } else if (DtAcpiPref.Pref == DT_ACPI_SELECT_DT) {
>>      //
>>      // DT was selected: copy the blob into newly allocated memory and install
>>      // a reference to it as the FDT configuration table.
>>      //
>> -    DtbCopy = AllocateCopyPool (DtbSize, Dtb);
>> -    if (DtbCopy == NULL) {
>> -      return EFI_OUT_OF_RESOURCES;
>> -    }
>> -    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DtbCopy);
>> +    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, Dtb);
>>      if (EFI_ERROR (Status)) {
>>        DEBUG ((DEBUG_ERROR, "%a: failed to install FDT configuration table\n",
>>          __FUNCTION__));
>> -      FreePool (DtbCopy);
>> -      return Status;
>> +      goto FreeDtb;
>>      }
>>    } else {
>>      ASSERT (FALSE);
>> @@ -210,4 +203,11 @@ DtPlatformDxeEntryPoint (
>>    // installed in that case.
>>    //
>>    return InstallHiiPages ();
>> +
>> +FreeDtb:
>> +  if (Dtb != NULL) {
>> +    FreePool (Dtb);
>> +  }
>> +
>> +  return Status;
>>  }
>> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
>> index b73877a6086b..45dfd9088bf0 100644
>> --- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
>> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
>> @@ -40,7 +40,7 @@ [Packages]
>>  [LibraryClasses]
>>    BaseLib
>>    DebugLib
>> -  DxeServicesLib
>> +  DtPlatformDtbLoaderLib
>>    HiiLib
>>    MemoryAllocationLib
>>    UefiBootServicesTableLib
>> diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc
>> index 0d5db68631bb..b75678276e8d 100644
>> --- a/EmbeddedPkg/EmbeddedPkg.dsc
>> +++ b/EmbeddedPkg/EmbeddedPkg.dsc
>> @@ -294,5 +294,7 @@ [Components.common]
>>    EmbeddedPkg/Library/PrePiHobLib/PrePiHobLib.inf
>>    EmbeddedPkg/Library/PrePiMemoryAllocationLib/PrePiMemoryAllocationLib.inf
>>
>> +  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
>> +
>>  [Components.IA32, Components.X64, Components.IPF, Components.ARM]
>>    EmbeddedPkg/GdbStub/GdbStub.inf
>>
>
> Please split off the last hunk (for "EmbeddedPkg/EmbeddedPkg.dsc") to a
> separate patch. (You might want to make that the first patch in the
> series, saying "we forgot this before, but it's needed to build the
> driver at all", or some such.)
>
> With these changes, for both patches:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks
> Laszlo

Re: [PATCH v2 0/3] EmbeddedPkg: revert DTB loading to platform lib

[Ard Biesheuvel]

On 31 March 2017 at 11:56, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> This updates the recently added DtPlatformDxe to indirect loading of the
> actual DTB binary to a separate library. A base implementation is provided
> that preserves the original behavior.
>
> This way, we can allow platforms such as FVP or Juno (which use the same
> firmware image but different DTB images) to override this library class
> and insert its own logic to select the correct DTB, without resorting to
> BEFORE depexes and dynamic PCDs, and the associated load order issues.
>
> v2: - drop ArmPlatformPkg changes for now
>
> Ard Biesheuvel (3):
>   EmbeddedPkg: add DtPlatformDtbLoaderLib library class
>   EmbeddedPkg: add base DtPlatformDtbLoaderLib implementation
>   EmbeddedPkg/DtPlatformDxe: load platform DTB via new library
>

Committed as

f07cc45883d0 EmbeddedPkg: add DtPlatformDxe to .dsc file
449a5df455ed EmbeddedPkg: add DtPlatformDtbLoaderLib library class
4c725c895990 EmbeddedPkg: add base DtPlatformDtbLoaderLib implementation
12c71010b84a EmbeddedPkg/DtPlatformDxe: load platform DTB via new library

Thanks,
Ard.

Re: [PATCH v2 3/3] EmbeddedPkg/DtPlatformDxe: load platform DTB via new library

[Laszlo Ersek]

On 03/31/17 14:22, Ard Biesheuvel wrote:
> On 31 March 2017 at 12:39, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 03/31/17 12:56, Ard Biesheuvel wrote:
>>> To give platforms some room to decide which DTB is suitable and where
>>> to load it from, indirect loading of the DTB image via the new
>>> DtPlatformDtbLoaderLib library class.
>>
>> I think you accidentally the verb in the above sentence :)
>>
> 
> 'indirect' was intended as the verb here, but I'll replace it for legibility

Ah, I guess I would have said "divert", or just "direct" or "redirect".
Thanks for the info; I'll keep this use in mind.

Laszlo