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

[Laszlo Ersek <lersek@redhat.com>]

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