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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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