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

[Sascha Hauer <s.hauer@pengutronix.de>]

On Wed, Jun 26, 2024 at 12:04:42PM +0200, Marco Felsch wrote:
> 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.

Then let's put it differently:

Currently the bootm code calls fit_open_configuration() to get the
configuration node for a kernel. From the nodes above only one qualifies
being a kernel configuration, namely the one with the "kernel" property.

So instead of ignoring all nodes that are overlays you could instead
ignore all nodes that are not kernels.

Doing so would limit fit_open_configuration() to only kernel
configurations. This would break arch/arm/mach-layerscape/ppa.c
which calls fit_open_configuration() to find exactly the configuration
you are referring to above. So I think fit_open_configuration() needs
some way to let the user specify which configuration we are looking for,
maybe by adding some match() function pointer argument.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |