[PATCH RFC 6/6] binman: doc: update Optional entries

Tue Feb 18 14:15:07 CET 2025

On Mon, 2025-02-17 at 06:13 -0700, Simon Glass wrote:
> Hi Yannic,
> 
> On Mon, 17 Feb 2025 at 00:21, Yannic Moog <Y.Moog at phytec.de> wrote:
> > 
> > Hi Simon,
> > 
> > On Fri, 2025-02-14 at 06:48 -0700, Simon Glass wrote:
> > > Hi Yannic,
> > > 
> > > On Fri, 14 Feb 2025 at 00:05, Yannic Moog <Y.Moog at phytec.de>
> > > wrote:
> > > > 
> > > > On Thu, 2025-02-13 at 07:01 -0700, Simon Glass wrote:
> > > > > Hi Yannic,
> > > > > 
> > > > > On Thu, 13 Feb 2025 at 00:21, Yannic Moog <Y.Moog at phytec.de>
> > > > > wrote:
> > > > > > 
> > > > > > On Mon, 2025-02-10 at 06:08 -0700, Simon Glass wrote:
> > > > > > > On Wed, 29 Jan 2025 at 03:30, Yannic Moog
> > > > > > > <y.moog at phytec.de> wrote:
> > > > > > > > 
> > > > > > > > The binman documentation of Optional entries is not
> > > > > > > > accurate in the
> > > > > > > > sense that it does not cover blobs entry type. As this
> > > > > > > > is also the most
> > > > > > > > widely used type to have the optional entry, document
> > > > > > > > the interaction
> > > > > > > > with faking blobs.
> > > > > > > > 
> > > > > > > > Signed-off-by: Yannic Moog <y.moog at phytec.de>
> > > > > > > > ---
> > > > > > > >  tools/binman/binman.rst | 9 ++++++++-
> > > > > > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > > > > > 
> > > > > > > > diff --git a/tools/binman/binman.rst
> > > > > > > > b/tools/binman/binman.rst
> > > > > > > > index 990fc295770..2bafa6ca408 100644
> > > > > > > > --- a/tools/binman/binman.rst
> > > > > > > > +++ b/tools/binman/binman.rst
> > > > > > > > @@ -1145,7 +1145,14 @@ called on all entries.
> > > > > > > >  It is not possible for an entry to mark itself absent
> > > > > > > > at any other point in the
> > > > > > > >  processing. It must happen in the ObtainContents()
> > > > > > > > method.
> > > > > > > > 
> > > > > > > > -The effect is as if the entry had never been present
> > > > > > > > at all, since the image
> > > > > > > > +The effect depends on the type of entry.
> > > > > > > > +
> > > > > > > > +Blobs
> > > > > > > > +~~~~~
> > > > > > > > +For blobs, the effect depends on whether --fake-ext-
> > > > > > > > blobs is passed
> > > > > > > > +to binman. (This is the case by default)
> > > > > > > > +In case --fake-ext-blobs is set, any missing entries
> > > > > > > > will be faked.
> > > > > > > > +If not set, it is as if the entry had never been
> > > > > > > > present at all, since the image
> > > > > > > >  is packed without it and it disappears from the list
> > > > > > > > of entries.
> > > > > > > 
> > > > > > > I'm not quite following this. The text seems OK but your
> > > > > > > heading
> > > > > > > implies that things other than blobs can be faked. All of
> > > > > > > this
> > > > > > > functionality is only for blobs.
> > > > > > 
> > > > > > The heading is a subsection of "Optional entries". To my
> > > > > > knowledge the "optional" property
> > > > > > is
> > > > > > not
> > > > > > limited to blobs. But blobs get special treatment in
> > > > > > regards to "optional" due to the --
> > > > > > fake-
> > > > > > ext-
> > > > > > blobs option. Hence the subsection.
> > > > > > 
> > > > > > What would you like me to change to make this clearer?
> > > > > 
> > > > > Oh I see. Are you saying that optional entries end up in the
> > > > > image
> > > > > when they are faked, external blobs? If so, that seems like a
> > > > > bug to
> > > > > me.
> > > > 
> > > > To clarify, optional images are faked only when they are
> > > > missing. So in case the image is
> > > > missing, a
> > > > faked image is indeed packaged into the image.
> > > > 
> > > 
> > > OK, but I still think it is a bug.
> > 
> > I didn't question your judgement, I simply don't have the broader
> > goal of binman in mind to be able
> > to judge if this behaviour is unintended.
> > What should happen instead? Should the image simply be absent in
> > the final image?
> > 
> > > Why add an optional thing in this way?
> > 
> > Because it seems like a simple, elegant solution.
> > To elaborate:
> > The goal (in case of OP-TEE) is to have the binary packaged when
> > its there and not packaged when it
> > is not present.
> > Precisely because it is not required for a bootable image, but may
> > be added to enhance functionality
> > . I.e. it is optional.
> > I found the U-Boot doc to explain optional entries and thought it
> > to be a perfect fit.
> > What do you have in mind to achieve this goal?
> 
> I think we are talking about slightly different things.
> 
> If a binary is optional then I don't see much point in including an
> empty (or faked) binary. If it is the only thing missing, the image
> should still boot OK if it just doesn't appear in the image.
> 
> The idea with Binman is to produce a functional image, where
> possible.
> An image without an optional blob is functional, so we should not
> need
> to add it.
> 
> I'm not saying this is a big problem, just that it seems surprising
> and not what I intended.

I am with you on this. It was suprising to me, too. And I agree there
is no point (at least to me) in including it in the final image.
So we were talking about the same thing, I simply want to make sure we
are on the same page.

> 
> Perhaps we do need to fake the blob in case it is used by some other
> tool. But I wonder if we can stop Binman adding the faked blob into
> the image, if it is optional?

From my point of view, It would be better to stop binman from adding
faked blobs than what I proposed in these RFC patches. I didn't know,
however, if there was a use case for adding them to the images that I
am not aware of.
Since you now confirm that it was never your intention to have them
included in the first place I think we can go ahead in preventing
binman from adding faked blobs to images.
I'll work on it for the next version.

Yannic

> 
> 
> > 
> > Yannic
> > 
> > > 
> > > 
> > > > Here is the image map from an (phycore-)imx8mp build.
> > > > 
> > > > Missing tee-os (faked):
> > > > 
> > > > ImagePos    Offset      Size  Name
> > > > 00000000  00000000  00159e10  image
> > > > 00000000   00000000  00159e10  section
> > > > [...]
> > > > 00058000    00058000  00101e10  fit
> > > > 0005b000     00003000  000e4910  uboot
> > > > 0005b000      00000000  000e4910  blob-ext
> > > > 0013f910     000e7910  00008458  atf
> > > > 0013f910      00000000  00008458  atf-blob
> > > > 00147d68     000efd68  00000000  tee
> > > > 00147d68      00000000  00000000  tee-os
> > > > 
> > > > tee-os present:
> > > > 
> > > > ImagePos    Offset      Size  Name
> > > > 00000000  00000000  0022b5b0  image
> > > > 00000000   00000000  0022b5b0  section
> > > > [...]
> > > > 00058000    00058000  001d35b0  fit
> > > > 0005b000     00003000  000e4910  uboot
> > > > 0005b000      00000000  000e4910  blob-ext
> > > > 0013f910     000e7910  00008458  atf
> > > > 0013f910      00000000  00008458  atf-blob
> > > > 00147d68     000efd68  000d17a0  tee
> > > > 00147d68      00000000  000d17a0  tee-os
> > > > 
> > > > Yannic
> 
> Regards,
> Simon