Re: [PATCH 2/8] FIT: skip possible overlay config nodes

[Marco Felsch <m.felsch@pengutronix.de>]

On 24-06-17, Sascha Hauer wrote:
> On Tue, Jun 11, 2024 at 10:36:47AM +0200, Marco Felsch wrote:
> > Hi,
> > 
> > sorry for the delay on this patchset.
> > 
> > On 24-03-25, Sascha Hauer wrote:
> > > Hi Marco,
> > > 
> > > On Fri, Mar 22, 2024 at 05:49:47PM +0100, Marco Felsch wrote:
> > > > The FIT spec is not very specific when it comes to device-tree overlay
> > > > handling.
> > > 
> > > By FIT spec you mean
> > > https://github.com/u-boot/u-boot/blob/master/doc/usage/fit/overlay-fdt-boot.rst,
> > > right, or is there more?
> > 
> > this is just an example which is not complete e.g. it misses the
> > signature node in case of verified boot. I used [1] as reference but
> > after reading it again I see that this reference list the kernel or
> > firmware properties as mandatory.
> > 
> > [1] https://docs.u-boot.org/en/latest/usage/fit/source_file_format.html#configurations-node
> > 
> > > > Overlays can be added directely to an config node:
> > > > 
> > > > 	config-a {
> > > > 		compatible = "machine-compatible";
> > > > 		kernel = "kernel-img-name";
> > > > 		fdt = "fdt-base-name", "fdt-overlay1-name", "...";
> > > > 	}
> > > > 
> > > > or they are supplied via dedicated config nodes:
> > > > 
> > > > 	overlay-2 {
> > > > 		fdt = "fdt-overlay2-name";
> > > > 	}
> > > > 
> > > > Of course these config nodes can have compatibles as well:
> > > > 
> > > > 	overlay-3 {
> > > > 		compatible = "machine-compatible";
> > > > 		fdt = "fdt-overlay3-name";
> > > > 	}
> > > 
> > > The text I referenced above doesn't mention compatible properties in
> > > overlay config nodes.
> > 
> > You're right, but the format description chapter [1] does.
> > 
> > > > The current fit_find_compatible_unit() code would skip the overlay node
> > > > if the config-a compatible has the same score as the overlay-3
> > > > compatible and if the overlay-3 config-node is listed after the config-a
> > > > config-node. But if the compatible of config-a config-node has a lower
> > > > score or the overlay-3 config-note is listed first (the spec does not
> > > > specify any order) we end up in taking the overlay-3 config-node instead
> > > > of config-a config-node.
> > > 
> > > You could distinguish overlay config nodes from full config nodes by the
> > > presence of a "kernel" property. Overlay config nodes do not have it.
> > 
> > Of course this could be done but I wanted to make it more explicit since
> > the FIT spec is not very clear when it comes to overlays. Instead of
> > adding an explicit image type like:
> > 
> >   - type = "flat_dt_overlay";
> > 
> > they used the already existing definitions. Therefore I went this way so
> > it is up to user to specify the overlay config nodes explicit.
> 
> I don't buy this argumentation. A node that doesn't have a 'kernel'
> property can not be booted, hence you can ignore it when looking for a
> bootable config. A node that does have a 'kernel' property by
> definition doesn't describe an overlay.

As written in the commit message the overlays can be applied in two
ways:

| 	config-a {
| 		compatible = "machine-compatible";
| 		kernel = "kernel-img-name";
| 		fdt = "fdt-base-name", "fdt-overlay1-name", "...";
| 	}

or via:

| 	overlay-2 {
| 		fdt = "fdt-overlay2-name";
| 	}

albeit this patchset targets the latter. This helper is used later as
well to detect if the the config node is an overlay not just to detect
if it is bootable or not. Right now there are also config nodes which
don't have the kernel property included but also aren't overlays e.g.
(doc/usage/fit/sec_firmware_ppa.rst):

|        configurations {
|            default = "config-1";
|            config-1 {
|                description = "PPA Secure firmware";
|                firmware = "firmware@1";
|                loadables = "trustedOS@1", "fuse_scr";
|            };
|        };

and I assume that more such config nodes will be added over time.

> I agree that it's unfortunate that a overlay config node can only be
> indirectly detected as such. Nevertheless I don't see a need for
> adding another globalvar for that.

Of course I can convert the check to drop specifying the overlay-config
node explicit and instead add an implicit check but my concerns are that
this is not as robust as the explicit mechanism and over time we end up
in false-positives while detecting overlays.

Regards,
  Marco