Re: [PATCH v3 0/4] AndroidBootImgLib improvements

["Leif Lindholm" <leif@nuviainc.com>]

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