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
>