[PATCH v3 0/4] AndroidBootImgLib improvements

[PATCH v3 0/4] AndroidBootImgLib improvements

[Jeff Brasen]

Added support for using loadfile2 approach for passing ramdisk to linux.
Created patch series for general error handling improvments based on
review feedback.
If ACPI tables are in system table or PCD is defined the LoadFile2 method
of passing initrd will be used.

[v3]
-Code review cleanup
-Removed duplicate header file
-Added change to allow FDT to install if UpdateDtb function is not defined
-Added specific ACPI check
-Moved install functions to subfunctions

[v2]
-Added review feedback
-General improvements to error handling

[v1]
- Intial revision


Jeff Brasen (4):
  EmbeddedPkg: Remove duplicate libfdt.h include
  EmbeddedPkg: AndroidBootImgBoot error handling updates
  EmbeddedPkg: Install FDT if UpdateDtb is not present
  EmbeddedPkg: Add LoadFile2 for linux initrd

 EmbeddedPkg/EmbeddedPkg.dec                   |   1 +
 .../AndroidBootImgLib/AndroidBootImgLib.inf   |   4 +
 .../AndroidBootImgLib/AndroidBootImgLib.c     | 275 +++++++++++++++---
 3 files changed, 233 insertions(+), 47 deletions(-)

-- 
2.17.1

[PATCH v3 1/4] EmbeddedPkg: Remove duplicate libfdt.h include

[Jeff Brasen]

Remove duplicate libfdt.h include statement in AndroidBootImgLib

Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
index bbc240c3632a..33425060b15d 100644
--- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
@@ -16,8 +16,6 @@
 #include <Protocol/AndroidBootImg.h>
 #include <Protocol/LoadedImage.h>
 
-#include <libfdt.h>
-
 #define FDT_ADDITIONAL_ENTRIES_SIZE 0x400
 
 typedef struct {
-- 
2.17.1

[PATCH v3 2/4] EmbeddedPkg: AndroidBootImgBoot error handling updates

[Jeff Brasen]

Update AndroidBootImgBoot to use a single return point
Make sure Kernel args are freed and Image is unloaded.

Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
 .../AndroidBootImgLib/AndroidBootImgLib.c     | 50 +++++++++++--------
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
index 33425060b15d..324933013d91 100644
--- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
@@ -382,10 +382,13 @@ AndroidBootImgBoot (
   UINTN                               RamdiskSize;
   IN  VOID                           *FdtBase;
 
+  NewKernelArg = NULL;
+  ImageHandle = NULL;
+
   Status = gBS->LocateProtocol (&gAndroidBootImgProtocolGuid, NULL,
                                 (VOID **) &mAndroidBootImg);
   if (EFI_ERROR (Status)) {
-    return Status;
+    goto Exit;
   }
 
   Status = AndroidBootImgGetKernelInfo (
@@ -394,19 +397,19 @@ AndroidBootImgBoot (
             &KernelSize
             );
   if (EFI_ERROR (Status)) {
-    return Status;
+    goto Exit;
   }
 
   NewKernelArg = AllocateZeroPool (ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
   if (NewKernelArg == NULL) {
     DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
-    return EFI_OUT_OF_RESOURCES;
+    Status = EFI_OUT_OF_RESOURCES;
+    goto Exit;
   }
 
   Status = AndroidBootImgUpdateArgs (Buffer, NewKernelArg);
   if (EFI_ERROR (Status)) {
-    FreePool (NewKernelArg);
-    return Status;
+    goto Exit;
   }
 
   Status = AndroidBootImgGetRamdiskInfo (
@@ -415,19 +418,17 @@ AndroidBootImgBoot (
             &RamdiskSize
             );
   if (EFI_ERROR (Status)) {
-    return Status;
+    goto Exit;
   }
 
   Status = AndroidBootImgLocateFdt (Buffer, &FdtBase);
   if (EFI_ERROR (Status)) {
-    FreePool (NewKernelArg);
-    return Status;
+    goto Exit;
   }
 
   Status = AndroidBootImgUpdateFdt (Buffer, FdtBase, RamdiskData, RamdiskSize);
   if (EFI_ERROR (Status)) {
-    FreePool (NewKernelArg);
-    return Status;
+    goto Exit;
   }
 
   KernelDevicePath = mMemoryDevicePathTemplate;
@@ -440,21 +441,15 @@ AndroidBootImgBoot (
                            (EFI_DEVICE_PATH *)&KernelDevicePath,
                            (VOID*)(UINTN)Kernel, KernelSize, &ImageHandle);
   if (EFI_ERROR (Status)) {
-    //
-    // With EFI_SECURITY_VIOLATION retval, the Image was loaded and an ImageHandle was created
-    // with a valid EFI_LOADED_IMAGE_PROTOCOL, but the image can not be started right now.
-    // If the caller doesn't have the option to defer the execution of an image, we should
-    // unload image for the EFI_SECURITY_VIOLATION to avoid resource leak.
-    //
-    if (Status == EFI_SECURITY_VIOLATION) {
-      gBS->UnloadImage (ImageHandle);
-    }
-    return Status;
+    goto Exit;
   }
 
   // Set kernel arguments
   Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid,
                                 (VOID **) &ImageInfo);
+  if (EFI_ERROR (Status)) {
+    goto Exit;
+  }
   ImageInfo->LoadOptions = NewKernelArg;
   ImageInfo->LoadOptionsSize = StrLen (NewKernelArg) * sizeof (CHAR16);
 
@@ -464,5 +459,18 @@ AndroidBootImgBoot (
   Status = gBS->StartImage (ImageHandle, NULL, NULL);
   // Clear the Watchdog Timer if the image returns
   gBS->SetWatchdogTimer (0, 0x10000, 0, NULL);
-  return EFI_SUCCESS;
+
+Exit:
+  //Unload image as it will not be used anymore
+  if (ImageHandle != NULL) {
+    gBS->UnloadImage (ImageHandle);
+    ImageHandle = NULL;
+  }
+  if (EFI_ERROR (Status)) {
+    if (NewKernelArg != NULL) {
+      FreePool (NewKernelArg);
+      NewKernelArg = NULL;
+    }
+  }
+  return Status;
 }
-- 
2.17.1

[PATCH v3 3/4] EmbeddedPkg: Install FDT if UpdateDtb is not present

[Jeff Brasen]

Currently if mAndroidBootImg->UpdateDtb is not supported on the platform
the device tree updates of the initrd are not made.

Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
 .../Library/AndroidBootImgLib/AndroidBootImgLib.c     | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
index 324933013d91..4f8ff5b261ca 100644
--- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
@@ -349,12 +349,13 @@ AndroidBootImgUpdateFdt (
     if (EFI_ERROR (Status)) {
       goto Fdt_Exit;
     }
-
-    Status = gBS->InstallConfigurationTable (
-                    &gFdtTableGuid,
-                    (VOID *)(UINTN)NewFdtBase
-                    );
+  } else {
+    NewFdtBase = UpdatedFdtBase;
   }
+  Status = gBS->InstallConfigurationTable (
+                  &gFdtTableGuid,
+                  (VOID *)(UINTN)NewFdtBase
+                  );
 
   if (!EFI_ERROR (Status)) {
     return EFI_SUCCESS;
-- 
2.17.1

[PATCH v3 4/4] EmbeddedPkg: Add LoadFile2 for linux initrd

[Jeff Brasen]

Add support under a pcd feature for using the new interface to pass
initrd to the linux kernel instead of via device tree.
This feature is also enabled if ACPI tables are present, and will skip
locating and installation of device tree.

Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
 EmbeddedPkg/EmbeddedPkg.dec                   |   1 +
 .../AndroidBootImgLib/AndroidBootImgLib.inf   |   4 +
 .../AndroidBootImgLib/AndroidBootImgLib.c     | 218 ++++++++++++++++--
 3 files changed, 201 insertions(+), 22 deletions(-)

diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index c2068ce5a8ce..7638aaaadeb8 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -86,6 +86,7 @@ [Ppis]
 [PcdsFeatureFlag.common]
   gEmbeddedTokenSpaceGuid.PcdPrePiProduceMemoryTypeInformationHob|FALSE|BOOLEAN|0x0000001b
   gEmbeddedTokenSpaceGuid.PcdGdbSerial|FALSE|BOOLEAN|0x00000053
+  gEmbeddedTokenSpaceGuid.PcdAndroidBootLoadFile2|FALSE|BOOLEAN|0x0000005b
 
 [PcdsFixedAtBuild.common]
   gEmbeddedTokenSpaceGuid.PcdPrePiStackBase|0|UINT32|0x0000000b
diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
index bfed4ba9dcd4..affde50f617a 100644
--- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
@@ -41,4 +41,8 @@ [Protocols]
   gEfiLoadedImageProtocolGuid
 
 [Guids]
+  gEfiAcpiTableGuid
   gFdtTableGuid
+
+[FeaturePcd]
+  gEmbeddedTokenSpaceGuid.PcdAndroidBootLoadFile2
diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
index 4f8ff5b261ca..cc39fef4895c 100644
--- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
@@ -10,12 +10,16 @@
 #include <libfdt.h>
 #include <Library/AndroidBootImgLib.h>
 #include <Library/PrintLib.h>
+#include <Library/DevicePathLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiLib.h>
 
 #include <Protocol/AndroidBootImg.h>
+#include <Protocol/LoadFile2.h>
 #include <Protocol/LoadedImage.h>
 
+#include <Guid/LinuxEfiInitrdMedia.h>
+
 #define FDT_ADDITIONAL_ENTRIES_SIZE 0x400
 
 typedef struct {
@@ -23,7 +27,15 @@ typedef struct {
   EFI_DEVICE_PATH_PROTOCOL                End;
 } MEMORY_DEVICE_PATH;
 
+typedef struct {
+  VENDOR_DEVICE_PATH                      VendorMediaNode;
+  EFI_DEVICE_PATH_PROTOCOL                EndNode;
+} RAMDISK_DEVICE_PATH;
+
 STATIC ANDROID_BOOTIMG_PROTOCOL                 *mAndroidBootImg;
+STATIC VOID                                     *mRamdiskData = NULL;
+STATIC UINTN                                    mRamdiskSize = 0;
+STATIC EFI_HANDLE                               mRamDiskLoadFileHandle = NULL;
 
 STATIC CONST MEMORY_DEVICE_PATH mMemoryDevicePathTemplate =
 {
@@ -46,6 +58,99 @@ STATIC CONST MEMORY_DEVICE_PATH mMemoryDevicePathTemplate =
   } // End
 };
 
+STATIC CONST RAMDISK_DEVICE_PATH mRamdiskDevicePath =
+{
+  {
+    {
+      MEDIA_DEVICE_PATH,
+      MEDIA_VENDOR_DP,
+      { sizeof (VENDOR_DEVICE_PATH), 0 }
+    },
+    LINUX_EFI_INITRD_MEDIA_GUID
+  },
+  {
+    END_DEVICE_PATH_TYPE,
+    END_ENTIRE_DEVICE_PATH_SUBTYPE,
+    { sizeof (EFI_DEVICE_PATH_PROTOCOL), 0 }
+  }
+};
+
+/**
+  Causes the driver to load a specified file.
+
+  @param  This       Protocol instance pointer.
+  @param  FilePath   The device specific path of the file to load.
+  @param  BootPolicy Should always be FALSE.
+  @param  BufferSize On input the size of Buffer in bytes. On output with a return
+                     code of EFI_SUCCESS, the amount of data transferred to
+                     Buffer. On output with a return code of EFI_BUFFER_TOO_SMALL,
+                     the size of Buffer required to retrieve the requested file.
+  @param  Buffer     The memory buffer to transfer the file to. IF Buffer is NULL,
+                     then no the size of the requested file is returned in
+                     BufferSize.
+
+  @retval EFI_SUCCESS           The file was loaded.
+  @retval EFI_UNSUPPORTED       BootPolicy is TRUE.
+  @retval EFI_INVALID_PARAMETER FilePath is not a valid device path, or
+                                BufferSize is NULL.
+  @retval EFI_NO_MEDIA          No medium was present to load the file.
+  @retval EFI_DEVICE_ERROR      The file was not loaded due to a device error.
+  @retval EFI_NO_RESPONSE       The remote system did not respond.
+  @retval EFI_NOT_FOUND         The file was not found
+  @retval EFI_ABORTED           The file load process was manually canceled.
+  @retval EFI_BUFFER_TOO_SMALL  The BufferSize is too small to read the current
+                                directory entry. BufferSize has been updated with
+                                the size needed to complete the request.
+
+
+**/
+EFI_STATUS
+EFIAPI
+AndroidBootImgLoadFile2 (
+  IN EFI_LOAD_FILE2_PROTOCOL    *This,
+  IN EFI_DEVICE_PATH_PROTOCOL   *FilePath,
+  IN BOOLEAN                    BootPolicy,
+  IN OUT UINTN                  *BufferSize,
+  IN VOID                       *Buffer OPTIONAL
+  )
+
+{
+  // Verify if the valid parameters
+  if (This == NULL ||
+      BufferSize == NULL ||
+      FilePath == NULL ||
+      !IsDevicePathValid (FilePath, 0)) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (BootPolicy) {
+    return EFI_UNSUPPORTED;
+  }
+
+  // Check if the given buffer size is big enough
+  // EFI_BUFFER_TOO_SMALL to allow caller to allocate a bigger buffer
+  if (mRamdiskSize == 0) {
+    return EFI_NOT_FOUND;
+  }
+  if (Buffer == NULL || *BufferSize < mRamdiskSize) {
+    *BufferSize = mRamdiskSize;
+    return EFI_BUFFER_TOO_SMALL;
+  }
+
+  // Copy InitRd
+  CopyMem (Buffer, mRamdiskData, mRamdiskSize);
+  *BufferSize = mRamdiskSize;
+
+  return EFI_SUCCESS;
+}
+
+///
+/// Load File Protocol instance
+///
+STATIC EFI_LOAD_FILE2_PROTOCOL  mAndroidBootImgLoadFile2 = {
+  AndroidBootImgLoadFile2
+};
+
 EFI_STATUS
 AndroidBootImgGetImgSize (
   IN  VOID    *BootImg,
@@ -214,6 +319,60 @@ AndroidBootImgUpdateArgs (
   return Status;
 }
 
+EFI_STATUS
+AndroidBootImgInstallLoadFile2 (
+  IN  VOID                  *RamdiskData,
+  IN  UINTN                  RamdiskSize
+  )
+{
+  mRamDiskLoadFileHandle = NULL;
+  mRamdiskData = RamdiskData;
+  mRamdiskSize = RamdiskSize;
+  return gBS->InstallMultipleProtocolInterfaces (
+                &mRamDiskLoadFileHandle,
+                &gEfiLoadFile2ProtocolGuid,
+                &mAndroidBootImgLoadFile2,
+                &gEfiDevicePathProtocolGuid,
+                &mRamdiskDevicePath,
+                NULL
+                );
+}
+
+EFI_STATUS
+AndroidBootImgUninstallLoadFile2 (
+  VOID
+  )
+{
+  EFI_STATUS Status;
+
+  Status = EFI_SUCCESS;
+  mRamdiskData = NULL;
+  mRamdiskSize = 0;
+  if (mRamDiskLoadFileHandle != NULL) {
+    Status = gBS->UninstallMultipleProtocolInterfaces (
+                    mRamDiskLoadFileHandle,
+                    &gEfiLoadFile2ProtocolGuid,
+                    &mAndroidBootImgLoadFile2,
+                    &gEfiDevicePathProtocolGuid,
+                    &mRamdiskDevicePath,
+                    NULL
+                    );
+    mRamDiskLoadFileHandle = NULL;
+  }
+  return Status;
+}
+
+BOOLEAN AndroidBootImgAcpiSupported (
+  VOID
+  )
+{
+  EFI_STATUS  Status;
+  VOID       *AcpiTable;
+
+  Status = EfiGetSystemConfigurationTable (&gEfiAcpiTableGuid, &AcpiTable);
+  return !EFI_ERROR (Status);
+}
+
 EFI_STATUS
 AndroidBootImgLocateFdt (
   IN  VOID                  *BootImg,
@@ -325,23 +484,30 @@ AndroidBootImgUpdateFdt (
     goto Fdt_Exit;
   }
 
-  ChosenNode = AndroidBootImgGetChosenNode(UpdatedFdtBase);
-  if (!ChosenNode) {
-    goto Fdt_Exit;
-  }
+  if (FeaturePcdGet (PcdAndroidBootLoadFile2)) {
+    Status = AndroidBootImgInstallLoadFile2 (RamdiskData, RamdiskSize);
+    if (EFI_ERROR (Status)) {
+      goto Fdt_Exit;
+    }
+  } else {
+    ChosenNode = AndroidBootImgGetChosenNode(UpdatedFdtBase);
+    if (!ChosenNode) {
+      goto Fdt_Exit;
+    }
 
-  Status = AndroidBootImgSetProperty64 (UpdatedFdtBase, ChosenNode,
-                                        "linux,initrd-start",
-                                        (UINTN)RamdiskData);
-  if (EFI_ERROR (Status)) {
-    goto Fdt_Exit;
-  }
+    Status = AndroidBootImgSetProperty64 (UpdatedFdtBase, ChosenNode,
+                                          "linux,initrd-start",
+                                          (UINTN)RamdiskData);
+    if (EFI_ERROR (Status)) {
+      goto Fdt_Exit;
+    }
 
-  Status = AndroidBootImgSetProperty64 (UpdatedFdtBase, ChosenNode,
-                                        "linux,initrd-end",
-                                        (UINTN)RamdiskData + RamdiskSize);
-  if (EFI_ERROR (Status)) {
-    goto Fdt_Exit;
+    Status = AndroidBootImgSetProperty64 (UpdatedFdtBase, ChosenNode,
+                                          "linux,initrd-end",
+                                          (UINTN)RamdiskData + RamdiskSize);
+    if (EFI_ERROR (Status)) {
+      goto Fdt_Exit;
+    }
   }
 
   if (mAndroidBootImg->UpdateDtb) {
@@ -422,14 +588,21 @@ AndroidBootImgBoot (
     goto Exit;
   }
 
-  Status = AndroidBootImgLocateFdt (Buffer, &FdtBase);
-  if (EFI_ERROR (Status)) {
-    goto Exit;
-  }
+  if (AndroidBootImgAcpiSupported ()) {
+    Status = AndroidBootImgInstallLoadFile2 (RamdiskData, RamdiskSize);
+    if (EFI_ERROR (Status)) {
+      goto Exit;
+    }
+  } else {
+    Status = AndroidBootImgLocateFdt (Buffer, &FdtBase);
+    if (EFI_ERROR (Status)) {
+      goto Exit;
+    }
 
-  Status = AndroidBootImgUpdateFdt (Buffer, FdtBase, RamdiskData, RamdiskSize);
-  if (EFI_ERROR (Status)) {
-    goto Exit;
+    Status = AndroidBootImgUpdateFdt (Buffer, FdtBase, RamdiskData, RamdiskSize);
+    if (EFI_ERROR (Status)) {
+      goto Exit;
+    }
   }
 
   KernelDevicePath = mMemoryDevicePathTemplate;
@@ -473,5 +646,6 @@ AndroidBootImgBoot (
       NewKernelArg = NULL;
     }
   }
+  AndroidBootImgUninstallLoadFile2 ();
   return Status;
 }
-- 
2.17.1

Re: [PATCH v3 0/4] AndroidBootImgLib improvements

[Leif Lindholm]

Hi Jeff,

Thanks for this.
This set looks good to me, with a slight question mark wrt behaviour
compatibility with previous versions for 3/4.
(I think it's fine, but I'm a bear of very little brain, and it's been
several years since I reviewed this code, and even longer since I
really interacted with Android.
^
| shameless plug for more EmbeddedPkg reviewer volunteers.)

I've added Jun Nie, who wrote the original version of this code, to
see if he has any comments.

1-2/4 are obviously unproblematic, and I could merge those ahead of
time if preferred. You can add
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
for those if there are any further revisions of the set.

Best Regards,

Leif

On Mon, Sep 13, 2021 at 23:18:47 +0000, Jeff Brasen wrote:
> Added support for using loadfile2 approach for passing ramdisk to linux.
> Created patch series for general error handling improvments based on
> review feedback.
> If ACPI tables are in system table or PCD is defined the LoadFile2 method
> of passing initrd will be used.
> 
> [v3]
> -Code review cleanup
> -Removed duplicate header file
> -Added change to allow FDT to install if UpdateDtb function is not defined
> -Added specific ACPI check
> -Moved install functions to subfunctions
> 
> [v2]
> -Added review feedback
> -General improvements to error handling
> 
> [v1]
> - Intial revision
> 
> 
> Jeff Brasen (4):
>   EmbeddedPkg: Remove duplicate libfdt.h include
>   EmbeddedPkg: AndroidBootImgBoot error handling updates
>   EmbeddedPkg: Install FDT if UpdateDtb is not present
>   EmbeddedPkg: Add LoadFile2 for linux initrd
> 
>  EmbeddedPkg/EmbeddedPkg.dec                   |   1 +
>  .../AndroidBootImgLib/AndroidBootImgLib.inf   |   4 +
>  .../AndroidBootImgLib/AndroidBootImgLib.c     | 275 +++++++++++++++---
>  3 files changed, 233 insertions(+), 47 deletions(-)
> 
> -- 
> 2.17.1
> 

Re: [PATCH v3 0/4] AndroidBootImgLib improvements

[Jeff Brasen]

[-- Attachment #1: Type: text/plain, Size: 3126 bytes --]

So for patch 3: This is only a change if mAndroidBootImg->UpdateDtb == NULL.

This seemed like a bug as we would not add the initrd values nor would we use the fdt from the BootImg if that is where the device tree was sourced from.

It seems like either we should require UpdateDtb to be implemented (which seems to cause greater compatibility issues) or we install the device tree we have if that function isn't implemented.

As far as merging goes I am fine either way. Our particular flow won't hit this path as we don't have a device tree in the bootimg (use the system config table) and we will have the new pcd set, but this seemed like a bug while I looking at this code.


Thanks,

Jeff

________________________________
From: Leif Lindholm <leif@nuviainc.com>
Sent: Tuesday, September 14, 2021 9:00 AM
To: Jeff Brasen <jbrasen@nvidia.com>
Cc: devel@edk2.groups.io <devel@edk2.groups.io>; daniel.schaefer@hpe.com <daniel.schaefer@hpe.com>; abner.chang@hpe.com <abner.chang@hpe.com>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; Jun Nie <jun.nie@linaro.org>
Subject: Re: [PATCH v3 0/4] AndroidBootImgLib improvements

External email: Use caution opening links or attachments


Hi Jeff,

Thanks for this.
This set looks good to me, with a slight question mark wrt behaviour
compatibility with previous versions for 3/4.
(I think it's fine, but I'm a bear of very little brain, and it's been
several years since I reviewed this code, and even longer since I
really interacted with Android.
^
| shameless plug for more EmbeddedPkg reviewer volunteers.)

I've added Jun Nie, who wrote the original version of this code, to
see if he has any comments.

1-2/4 are obviously unproblematic, and I could merge those ahead of
time if preferred. You can add
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
for those if there are any further revisions of the set.

Best Regards,

Leif

On Mon, Sep 13, 2021 at 23:18:47 +0000, Jeff Brasen wrote:
> Added support for using loadfile2 approach for passing ramdisk to linux.
> Created patch series for general error handling improvments based on
> review feedback.
> If ACPI tables are in system table or PCD is defined the LoadFile2 method
> of passing initrd will be used.
>
> [v3]
> -Code review cleanup
> -Removed duplicate header file
> -Added change to allow FDT to install if UpdateDtb function is not defined
> -Added specific ACPI check
> -Moved install functions to subfunctions
>
> [v2]
> -Added review feedback
> -General improvements to error handling
>
> [v1]
> - Intial revision
>
>
> Jeff Brasen (4):
>   EmbeddedPkg: Remove duplicate libfdt.h include
>   EmbeddedPkg: AndroidBootImgBoot error handling updates
>   EmbeddedPkg: Install FDT if UpdateDtb is not present
>   EmbeddedPkg: Add LoadFile2 for linux initrd
>
>  EmbeddedPkg/EmbeddedPkg.dec                   |   1 +
>  .../AndroidBootImgLib/AndroidBootImgLib.inf   |   4 +
>  .../AndroidBootImgLib/AndroidBootImgLib.c     | 275 +++++++++++++++---
>  3 files changed, 233 insertions(+), 47 deletions(-)
>
> --
> 2.17.1
>

[-- Attachment #2: Type: text/html, Size: 5321 bytes --]

Re: [PATCH v3 0/4] AndroidBootImgLib improvements

[Jeff Brasen]

[-- Attachment #1: Type: text/plain, Size: 3660 bytes --]

Jun/Others,

  Any additional comments on this patch series?


Thanks,

Jeff

________________________________
From: Jeff Brasen <jbrasen@nvidia.com>
Sent: Tuesday, September 14, 2021 10:57 AM
To: Leif Lindholm <leif@nuviainc.com>
Cc: devel@edk2.groups.io <devel@edk2.groups.io>; daniel.schaefer@hpe.com <daniel.schaefer@hpe.com>; abner.chang@hpe.com <abner.chang@hpe.com>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; Jun Nie <jun.nie@linaro.org>
Subject: Re: [PATCH v3 0/4] AndroidBootImgLib improvements

So for patch 3: This is only a change if mAndroidBootImg->UpdateDtb == NULL.

This seemed like a bug as we would not add the initrd values nor would we use the fdt from the BootImg if that is where the device tree was sourced from.

It seems like either we should require UpdateDtb to be implemented (which seems to cause greater compatibility issues) or we install the device tree we have if that function isn't implemented.

As far as merging goes I am fine either way. Our particular flow won't hit this path as we don't have a device tree in the bootimg (use the system config table) and we will have the new pcd set, but this seemed like a bug while I looking at this code.


Thanks,

Jeff

________________________________
From: Leif Lindholm <leif@nuviainc.com>
Sent: Tuesday, September 14, 2021 9:00 AM
To: Jeff Brasen <jbrasen@nvidia.com>
Cc: devel@edk2.groups.io <devel@edk2.groups.io>; daniel.schaefer@hpe.com <daniel.schaefer@hpe.com>; abner.chang@hpe.com <abner.chang@hpe.com>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; Jun Nie <jun.nie@linaro.org>
Subject: Re: [PATCH v3 0/4] AndroidBootImgLib improvements

External email: Use caution opening links or attachments


Hi Jeff,

Thanks for this.
This set looks good to me, with a slight question mark wrt behaviour
compatibility with previous versions for 3/4.
(I think it's fine, but I'm a bear of very little brain, and it's been
several years since I reviewed this code, and even longer since I
really interacted with Android.
^
| shameless plug for more EmbeddedPkg reviewer volunteers.)

I've added Jun Nie, who wrote the original version of this code, to
see if he has any comments.

1-2/4 are obviously unproblematic, and I could merge those ahead of
time if preferred. You can add
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
for those if there are any further revisions of the set.

Best Regards,

Leif

On Mon, Sep 13, 2021 at 23:18:47 +0000, Jeff Brasen wrote:
> Added support for using loadfile2 approach for passing ramdisk to linux.
> Created patch series for general error handling improvments based on
> review feedback.
> If ACPI tables are in system table or PCD is defined the LoadFile2 method
> of passing initrd will be used.
>
> [v3]
> -Code review cleanup
> -Removed duplicate header file
> -Added change to allow FDT to install if UpdateDtb function is not defined
> -Added specific ACPI check
> -Moved install functions to subfunctions
>
> [v2]
> -Added review feedback
> -General improvements to error handling
>
> [v1]
> - Intial revision
>
>
> Jeff Brasen (4):
>   EmbeddedPkg: Remove duplicate libfdt.h include
>   EmbeddedPkg: AndroidBootImgBoot error handling updates
>   EmbeddedPkg: Install FDT if UpdateDtb is not present
>   EmbeddedPkg: Add LoadFile2 for linux initrd
>
>  EmbeddedPkg/EmbeddedPkg.dec                   |   1 +
>  .../AndroidBootImgLib/AndroidBootImgLib.inf   |   4 +
>  .../AndroidBootImgLib/AndroidBootImgLib.c     | 275 +++++++++++++++---
>  3 files changed, 233 insertions(+), 47 deletions(-)
>
> --
> 2.17.1
>

[-- Attachment #2: Type: text/html, Size: 6740 bytes --]

Re: [PATCH v3 0/4] AndroidBootImgLib improvements

[Jun Nie]

Hi Jeff,

I do not ever work on EDK soon after this patch set was merged.  It is
long time since then.
I am sorry that I have no comments other than no objections on your patch.

Regards,
Jun

Jeff Brasen <jbrasen@nvidia.com> 于2021年9月22日周三 上午12:33写道：
>
> Jun/Others,
>
>   Any additional comments on this patch series?
>
> Thanks,
>
> Jeff
>
> ________________________________
> From: Jeff Brasen <jbrasen@nvidia.com>
> Sent: Tuesday, September 14, 2021 10:57 AM
> To: Leif Lindholm <leif@nuviainc.com>
> Cc: devel@edk2.groups.io <devel@edk2.groups.io>; daniel.schaefer@hpe.com <daniel.schaefer@hpe.com>; abner.chang@hpe.com <abner.chang@hpe.com>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; Jun Nie <jun.nie@linaro.org>
> Subject: Re: [PATCH v3 0/4] AndroidBootImgLib improvements
>
> So for patch 3: This is only a change if mAndroidBootImg->UpdateDtb == NULL.
>
> This seemed like a bug as we would not add the initrd values nor would we use the fdt from the BootImg if that is where the device tree was sourced from.
>
> It seems like either we should require UpdateDtb to be implemented (which seems to cause greater compatibility issues) or we install the device tree we have if that function isn't implemented.
>
> As far as merging goes I am fine either way. Our particular flow won't hit this path as we don't have a device tree in the bootimg (use the system config table) and we will have the new pcd set, but this seemed like a bug while I looking at this code.
>
> Thanks,
>
> Jeff
>
> ________________________________
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Tuesday, September 14, 2021 9:00 AM
> To: Jeff Brasen <jbrasen@nvidia.com>
> Cc: devel@edk2.groups.io <devel@edk2.groups.io>; daniel.schaefer@hpe.com <daniel.schaefer@hpe.com>; abner.chang@hpe.com <abner.chang@hpe.com>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; Jun Nie <jun.nie@linaro.org>
> Subject: Re: [PATCH v3 0/4] AndroidBootImgLib improvements
>
> External email: Use caution opening links or attachments
>
>
> Hi Jeff,
>
> Thanks for this.
> This set looks good to me, with a slight question mark wrt behaviour
> compatibility with previous versions for 3/4.
> (I think it's fine, but I'm a bear of very little brain, and it's been
> several years since I reviewed this code, and even longer since I
> really interacted with Android.
> ^
> | shameless plug for more EmbeddedPkg reviewer volunteers.)
>
> I've added Jun Nie, who wrote the original version of this code, to
> see if he has any comments.
>
> 1-2/4 are obviously unproblematic, and I could merge those ahead of
> time if preferred. You can add
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> for those if there are any further revisions of the set.
>
> Best Regards,
>
> Leif
>
> On Mon, Sep 13, 2021 at 23:18:47 +0000, Jeff Brasen wrote:
> > Added support for using loadfile2 approach for passing ramdisk to linux.
> > Created patch series for general error handling improvments based on
> > review feedback.
> > If ACPI tables are in system table or PCD is defined the LoadFile2 method
> > of passing initrd will be used.
> >
> > [v3]
> > -Code review cleanup
> > -Removed duplicate header file
> > -Added change to allow FDT to install if UpdateDtb function is not defined
> > -Added specific ACPI check
> > -Moved install functions to subfunctions
> >
> > [v2]
> > -Added review feedback
> > -General improvements to error handling
> >
> > [v1]
> > - Intial revision
> >
> >
> > Jeff Brasen (4):
> >   EmbeddedPkg: Remove duplicate libfdt.h include
> >   EmbeddedPkg: AndroidBootImgBoot error handling updates
> >   EmbeddedPkg: Install FDT if UpdateDtb is not present
> >   EmbeddedPkg: Add LoadFile2 for linux initrd
> >
> >  EmbeddedPkg/EmbeddedPkg.dec                   |   1 +
> >  .../AndroidBootImgLib/AndroidBootImgLib.inf   |   4 +
> >  .../AndroidBootImgLib/AndroidBootImgLib.c     | 275 +++++++++++++++---
> >  3 files changed, 233 insertions(+), 47 deletions(-)
> >
> > --
> > 2.17.1
> >

Re: [PATCH v3 0/4] AndroidBootImgLib improvements

[Leif Lindholm]

Hi Jeff,

I was about to say "no more issues", and then I went to build
EmbeddedPkg, and it turns out this fails in
Applications/AndroidBootApp due to the missing dependency on
gEfiLoadFile2ProtocolGuid in AndroidBootImgLib.inf.

(Why this doesn't break AndroidFastbootApp build as well is not
immediately obvious to me.)

Would you like to figure out why, or would you prefer me to just fold
in

diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
index affde50f617a..8eefeef4f915 100644
--- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
@@ -39,6 +39,7 @@ [Packages]
 [Protocols]
   gAndroidBootImgProtocolGuid
   gEfiLoadedImageProtocolGuid
+  gEfiLoadFile2ProtocolGuid
 
 [Guids]
   gEfiAcpiTableGuid

?

/
    Leif

On Tue, Sep 21, 2021 at 16:32:58 +0000, Jeff Brasen wrote:
> Jun/Others,
> 
>   Any additional comments on this patch series?
> 
> 
> Thanks,
> 
> Jeff
> 
> ________________________________
> From: Jeff Brasen <jbrasen@nvidia.com>
> Sent: Tuesday, September 14, 2021 10:57 AM
> To: Leif Lindholm <leif@nuviainc.com>
> Cc: devel@edk2.groups.io <devel@edk2.groups.io>; daniel.schaefer@hpe.com <daniel.schaefer@hpe.com>; abner.chang@hpe.com <abner.chang@hpe.com>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; Jun Nie <jun.nie@linaro.org>
> Subject: Re: [PATCH v3 0/4] AndroidBootImgLib improvements
> 
> So for patch 3: This is only a change if mAndroidBootImg->UpdateDtb == NULL.
> 
> This seemed like a bug as we would not add the initrd values nor would we use the fdt from the BootImg if that is where the device tree was sourced from.
> 
> It seems like either we should require UpdateDtb to be implemented (which seems to cause greater compatibility issues) or we install the device tree we have if that function isn't implemented.
> 
> As far as merging goes I am fine either way. Our particular flow won't hit this path as we don't have a device tree in the bootimg (use the system config table) and we will have the new pcd set, but this seemed like a bug while I looking at this code.
> 
> 
> Thanks,
> 
> Jeff
> 
> ________________________________
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Tuesday, September 14, 2021 9:00 AM
> To: Jeff Brasen <jbrasen@nvidia.com>
> Cc: devel@edk2.groups.io <devel@edk2.groups.io>; daniel.schaefer@hpe.com <daniel.schaefer@hpe.com>; abner.chang@hpe.com <abner.chang@hpe.com>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; Jun Nie <jun.nie@linaro.org>
> Subject: Re: [PATCH v3 0/4] AndroidBootImgLib improvements
> 
> External email: Use caution opening links or attachments
> 
> 
> Hi Jeff,
> 
> Thanks for this.
> This set looks good to me, with a slight question mark wrt behaviour
> compatibility with previous versions for 3/4.
> (I think it's fine, but I'm a bear of very little brain, and it's been
> several years since I reviewed this code, and even longer since I
> really interacted with Android.
> ^
> | shameless plug for more EmbeddedPkg reviewer volunteers.)
> 
> I've added Jun Nie, who wrote the original version of this code, to
> see if he has any comments.
> 
> 1-2/4 are obviously unproblematic, and I could merge those ahead of
> time if preferred. You can add
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> for those if there are any further revisions of the set.
> 
> Best Regards,
> 
> Leif
> 
> On Mon, Sep 13, 2021 at 23:18:47 +0000, Jeff Brasen wrote:
> > Added support for using loadfile2 approach for passing ramdisk to linux.
> > Created patch series for general error handling improvments based on
> > review feedback.
> > If ACPI tables are in system table or PCD is defined the LoadFile2 method
> > of passing initrd will be used.
> >
> > [v3]
> > -Code review cleanup
> > -Removed duplicate header file
> > -Added change to allow FDT to install if UpdateDtb function is not defined
> > -Added specific ACPI check
> > -Moved install functions to subfunctions
> >
> > [v2]
> > -Added review feedback
> > -General improvements to error handling
> >
> > [v1]
> > - Intial revision
> >
> >
> > Jeff Brasen (4):
> >   EmbeddedPkg: Remove duplicate libfdt.h include
> >   EmbeddedPkg: AndroidBootImgBoot error handling updates
> >   EmbeddedPkg: Install FDT if UpdateDtb is not present
> >   EmbeddedPkg: Add LoadFile2 for linux initrd
> >
> >  EmbeddedPkg/EmbeddedPkg.dec                   |   1 +
> >  .../AndroidBootImgLib/AndroidBootImgLib.inf   |   4 +
> >  .../AndroidBootImgLib/AndroidBootImgLib.c     | 275 +++++++++++++++---
> >  3 files changed, 233 insertions(+), 47 deletions(-)
> >
> > --
> > 2.17.1
> >

Re: [PATCH v3 0/4] AndroidBootImgLib improvements

[Jeff Brasen]

That looks like something I missed and looks good, do you need me to make a v4 or will you just add that in. When I built I just used our application that has the guid in our inf so missed this.

>From my looking at it AndroidFastBootApp doesn't seem to use this lib.

> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Thursday, September 23, 2021 5:06 AM
> To: Jeff Brasen <jbrasen@nvidia.com>
> Cc: devel@edk2.groups.io; daniel.schaefer@hpe.com;
> abner.chang@hpe.com; ardb+tianocore@kernel.org; Jun Nie
> <jun.nie@linaro.org>
> Subject: Re: [PATCH v3 0/4] AndroidBootImgLib improvements
> 
> External email: Use caution opening links or attachments
> 
> 
> Hi Jeff,
> 
> I was about to say "no more issues", and then I went to build EmbeddedPkg,
> and it turns out this fails in Applications/AndroidBootApp due to the missing
> dependency on gEfiLoadFile2ProtocolGuid in AndroidBootImgLib.inf.
> 
> (Why this doesn't break AndroidFastbootApp build as well is not immediately
> obvious to me.)
> 
> Would you like to figure out why, or would you prefer me to just fold in
> 
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> index affde50f617a..8eefeef4f915 100644
> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> @@ -39,6 +39,7 @@ [Packages]
>  [Protocols]
>    gAndroidBootImgProtocolGuid
>    gEfiLoadedImageProtocolGuid
> +  gEfiLoadFile2ProtocolGuid
> 
>  [Guids]
>    gEfiAcpiTableGuid
> 
> ?
> 
> /
>     Leif
> 
> On Tue, Sep 21, 2021 at 16:32:58 +0000, Jeff Brasen wrote:
> > Jun/Others,
> >
> >   Any additional comments on this patch series?
> >
> >
> > Thanks,
> >
> > Jeff
> >
> > ________________________________
> > From: Jeff Brasen <jbrasen@nvidia.com>
> > Sent: Tuesday, September 14, 2021 10:57 AM
> > To: Leif Lindholm <leif@nuviainc.com>
> > Cc: devel@edk2.groups.io <devel@edk2.groups.io>;
> > daniel.schaefer@hpe.com <daniel.schaefer@hpe.com>;
> abner.chang@hpe.com
> > <abner.chang@hpe.com>; ardb+tianocore@kernel.org
> > <ardb+tianocore@kernel.org>; Jun Nie <jun.nie@linaro.org>
> > Subject: Re: [PATCH v3 0/4] AndroidBootImgLib improvements
> >
> > So for patch 3: This is only a change if mAndroidBootImg->UpdateDtb ==
> NULL.
> >
> > This seemed like a bug as we would not add the initrd values nor would we
> use the fdt from the BootImg if that is where the device tree was sourced
> from.
> >
> > It seems like either we should require UpdateDtb to be implemented
> (which seems to cause greater compatibility issues) or we install the device
> tree we have if that function isn't implemented.
> >
> > As far as merging goes I am fine either way. Our particular flow won't hit
> this path as we don't have a device tree in the bootimg (use the system
> config table) and we will have the new pcd set, but this seemed like a bug
> while I looking at this code.
> >
> >
> > Thanks,
> >
> > Jeff
> >
> > ________________________________
> > From: Leif Lindholm <leif@nuviainc.com>
> > Sent: Tuesday, September 14, 2021 9:00 AM
> > To: Jeff Brasen <jbrasen@nvidia.com>
> > Cc: devel@edk2.groups.io <devel@edk2.groups.io>;
> > daniel.schaefer@hpe.com <daniel.schaefer@hpe.com>;
> abner.chang@hpe.com
> > <abner.chang@hpe.com>; ardb+tianocore@kernel.org
> > <ardb+tianocore@kernel.org>; Jun Nie <jun.nie@linaro.org>
> > Subject: Re: [PATCH v3 0/4] AndroidBootImgLib improvements
> >
> > External email: Use caution opening links or attachments
> >
> >
> > Hi Jeff,
> >
> > Thanks for this.
> > This set looks good to me, with a slight question mark wrt behaviour
> > compatibility with previous versions for 3/4.
> > (I think it's fine, but I'm a bear of very little brain, and it's been
> > several years since I reviewed this code, and even longer since I
> > really interacted with Android.
> > ^
> > | shameless plug for more EmbeddedPkg reviewer volunteers.)
> >
> > I've added Jun Nie, who wrote the original version of this code, to
> > see if he has any comments.
> >
> > 1-2/4 are obviously unproblematic, and I could merge those ahead of
> > time if preferred. You can add
> > Reviewed-by: Leif Lindholm <leif@nuviainc.com> for those if there are
> > any further revisions of the set.
> >
> > Best Regards,
> >
> > Leif
> >
> > On Mon, Sep 13, 2021 at 23:18:47 +0000, Jeff Brasen wrote:
> > > Added support for using loadfile2 approach for passing ramdisk to linux.
> > > Created patch series for general error handling improvments based on
> > > review feedback.
> > > If ACPI tables are in system table or PCD is defined the LoadFile2
> > > method of passing initrd will be used.
> > >
> > > [v3]
> > > -Code review cleanup
> > > -Removed duplicate header file
> > > -Added change to allow FDT to install if UpdateDtb function is not
> > > defined -Added specific ACPI check -Moved install functions to
> > > subfunctions
> > >
> > > [v2]
> > > -Added review feedback
> > > -General improvements to error handling
> > >
> > > [v1]
> > > - Intial revision
> > >
> > >
> > > Jeff Brasen (4):
> > >   EmbeddedPkg: Remove duplicate libfdt.h include
> > >   EmbeddedPkg: AndroidBootImgBoot error handling updates
> > >   EmbeddedPkg: Install FDT if UpdateDtb is not present
> > >   EmbeddedPkg: Add LoadFile2 for linux initrd
> > >
> > >  EmbeddedPkg/EmbeddedPkg.dec                   |   1 +
> > >  .../AndroidBootImgLib/AndroidBootImgLib.inf   |   4 +
> > >  .../AndroidBootImgLib/AndroidBootImgLib.c     | 275 +++++++++++++++-
> --
> > >  3 files changed, 233 insertions(+), 47 deletions(-)
> > >
> > > --
> > > 2.17.1
> > >

Re: [PATCH v3 0/4] AndroidBootImgLib improvements

[Leif Lindholm]

On Thu, Sep 23, 2021 at 15:20:41 +0000, Jeff Brasen wrote:
> That looks like something I missed and looks good, do you need me to
> make a v4 or will you just add that in.

Nah, I just folded it in.
Pushed as 79019c7a4228..7ea7f9c07757.

Thanks!

> When I built I just used our
> application that has the guid in our inf so missed this.
>
> From my looking at it AndroidFastBootApp doesn't seem to use this lib.

(That ought to have been obvious even to me, oh well, thanks :)

/
    Leif

> > -----Original Message-----
> > From: Leif Lindholm <leif@nuviainc.com>
> > Sent: Thursday, September 23, 2021 5:06 AM
> > To: Jeff Brasen <jbrasen@nvidia.com>
> > Cc: devel@edk2.groups.io; daniel.schaefer@hpe.com;
> > abner.chang@hpe.com; ardb+tianocore@kernel.org; Jun Nie
> > <jun.nie@linaro.org>
> > Subject: Re: [PATCH v3 0/4] AndroidBootImgLib improvements
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > Hi Jeff,
> > 
> > I was about to say "no more issues", and then I went to build EmbeddedPkg,
> > and it turns out this fails in Applications/AndroidBootApp due to the missing
> > dependency on gEfiLoadFile2ProtocolGuid in AndroidBootImgLib.inf.
> > 
> > (Why this doesn't break AndroidFastbootApp build as well is not immediately
> > obvious to me.)
> > 
> > Would you like to figure out why, or would you prefer me to just fold in
> > 
> > diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> > b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> > index affde50f617a..8eefeef4f915 100644
> > --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> > +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> > @@ -39,6 +39,7 @@ [Packages]
> >  [Protocols]
> >    gAndroidBootImgProtocolGuid
> >    gEfiLoadedImageProtocolGuid
> > +  gEfiLoadFile2ProtocolGuid
> > 
> >  [Guids]
> >    gEfiAcpiTableGuid
> > 
> > ?
> > 
> > /
> >     Leif
> > 
> > On Tue, Sep 21, 2021 at 16:32:58 +0000, Jeff Brasen wrote:
> > > Jun/Others,
> > >
> > >   Any additional comments on this patch series?
> > >
> > >
> > > Thanks,
> > >
> > > Jeff
> > >
> > > ________________________________
> > > From: Jeff Brasen <jbrasen@nvidia.com>
> > > Sent: Tuesday, September 14, 2021 10:57 AM
> > > To: Leif Lindholm <leif@nuviainc.com>
> > > Cc: devel@edk2.groups.io <devel@edk2.groups.io>;
> > > daniel.schaefer@hpe.com <daniel.schaefer@hpe.com>;
> > abner.chang@hpe.com
> > > <abner.chang@hpe.com>; ardb+tianocore@kernel.org
> > > <ardb+tianocore@kernel.org>; Jun Nie <jun.nie@linaro.org>
> > > Subject: Re: [PATCH v3 0/4] AndroidBootImgLib improvements
> > >
> > > So for patch 3: This is only a change if mAndroidBootImg->UpdateDtb ==
> > NULL.
> > >
> > > This seemed like a bug as we would not add the initrd values nor would we
> > use the fdt from the BootImg if that is where the device tree was sourced
> > from.
> > >
> > > It seems like either we should require UpdateDtb to be implemented
> > (which seems to cause greater compatibility issues) or we install the device
> > tree we have if that function isn't implemented.
> > >
> > > As far as merging goes I am fine either way. Our particular flow won't hit
> > this path as we don't have a device tree in the bootimg (use the system
> > config table) and we will have the new pcd set, but this seemed like a bug
> > while I looking at this code.
> > >
> > >
> > > Thanks,
> > >
> > > Jeff
> > >
> > > ________________________________
> > > From: Leif Lindholm <leif@nuviainc.com>
> > > Sent: Tuesday, September 14, 2021 9:00 AM
> > > To: Jeff Brasen <jbrasen@nvidia.com>
> > > Cc: devel@edk2.groups.io <devel@edk2.groups.io>;
> > > daniel.schaefer@hpe.com <daniel.schaefer@hpe.com>;
> > abner.chang@hpe.com
> > > <abner.chang@hpe.com>; ardb+tianocore@kernel.org
> > > <ardb+tianocore@kernel.org>; Jun Nie <jun.nie@linaro.org>
> > > Subject: Re: [PATCH v3 0/4] AndroidBootImgLib improvements
> > >
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > Hi Jeff,
> > >
> > > Thanks for this.
> > > This set looks good to me, with a slight question mark wrt behaviour
> > > compatibility with previous versions for 3/4.
> > > (I think it's fine, but I'm a bear of very little brain, and it's been
> > > several years since I reviewed this code, and even longer since I
> > > really interacted with Android.
> > > ^
> > > | shameless plug for more EmbeddedPkg reviewer volunteers.)
> > >
> > > I've added Jun Nie, who wrote the original version of this code, to
> > > see if he has any comments.
> > >
> > > 1-2/4 are obviously unproblematic, and I could merge those ahead of
> > > time if preferred. You can add
> > > Reviewed-by: Leif Lindholm <leif@nuviainc.com> for those if there are
> > > any further revisions of the set.
> > >
> > > Best Regards,
> > >
> > > Leif
> > >
> > > On Mon, Sep 13, 2021 at 23:18:47 +0000, Jeff Brasen wrote:
> > > > Added support for using loadfile2 approach for passing ramdisk to linux.
> > > > Created patch series for general error handling improvments based on
> > > > review feedback.
> > > > If ACPI tables are in system table or PCD is defined the LoadFile2
> > > > method of passing initrd will be used.
> > > >
> > > > [v3]
> > > > -Code review cleanup
> > > > -Removed duplicate header file
> > > > -Added change to allow FDT to install if UpdateDtb function is not
> > > > defined -Added specific ACPI check -Moved install functions to
> > > > subfunctions
> > > >
> > > > [v2]
> > > > -Added review feedback
> > > > -General improvements to error handling
> > > >
> > > > [v1]
> > > > - Intial revision
> > > >
> > > >
> > > > Jeff Brasen (4):
> > > >   EmbeddedPkg: Remove duplicate libfdt.h include
> > > >   EmbeddedPkg: AndroidBootImgBoot error handling updates
> > > >   EmbeddedPkg: Install FDT if UpdateDtb is not present
> > > >   EmbeddedPkg: Add LoadFile2 for linux initrd
> > > >
> > > >  EmbeddedPkg/EmbeddedPkg.dec                   |   1 +
> > > >  .../AndroidBootImgLib/AndroidBootImgLib.inf   |   4 +
> > > >  .../AndroidBootImgLib/AndroidBootImgLib.c     | 275 +++++++++++++++-
> > --
> > > >  3 files changed, 233 insertions(+), 47 deletions(-)
> > > >
> > > > --
> > > > 2.17.1
> > > >