diff mbox series

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

Message ID 20170331105607.3477-4-ard.biesheuvel@linaro.org
State New
Headers show
Series EmbeddedPkg: revert DTB loading to platform lib | expand

Commit Message

Ard Biesheuvel March 31, 2017, 10:56 a.m. UTC 
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(-)

-- 
2.9.3

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek March 31, 2017, 11:39 a.m. UTC | #1 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 31, 2017, 12:22 p.m. UTC | #2 
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek March 31, 2017, 1:38 p.m. UTC | #3 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

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