Re: [PATCH v3 0/4] AndroidBootImgLib improvements

["Jeff Brasen" <jbrasen@nvidia.com>]

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
> > >