mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Sascha Hauer <s.hauer@pengutronix.de>
To: Marco Felsch <m.felsch@pengutronix.de>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH 2/8] FIT: skip possible overlay config nodes
Date: Mon, 1 Jul 2024 14:06:09 +0200	[thread overview]
Message-ID: <ZoKbsY22cvuY-VUj@pengutronix.de> (raw)
In-Reply-To: <20240626100442.z6prerlewpdwzeh5@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 |



  reply	other threads:[~2024-07-01 12:36 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-22 16:49 [PATCH 1/8] of: overlay: add of.overlay.fitconfigpattern param Marco Felsch
2024-03-22 16:49 ` [PATCH 2/8] FIT: skip possible overlay config nodes Marco Felsch
2024-03-25  8:27   ` Sascha Hauer
2024-06-11  8:36     ` Marco Felsch
2024-06-17  8:04       ` Sascha Hauer
2024-06-26 10:04         ` Marco Felsch
2024-07-01 12:06           ` Sascha Hauer [this message]
2024-03-22 16:49 ` [PATCH 3/8] of: overlay: make the pattern match function more generic Marco Felsch
2024-03-22 16:49 ` [PATCH 4/8] of: overlay: make search dir/path " Marco Felsch
2024-03-22 16:49 ` [PATCH 5/8] FIT: expose useful helpers Marco Felsch
2024-03-22 16:49 ` [PATCH 6/8] of: overlay: add FIT overlay support Marco Felsch
2024-03-25  8:51   ` Sascha Hauer
2024-06-11  9:09     ` Marco Felsch
2024-03-22 16:49 ` [PATCH 7/8] of: overlay: drop unnecessary empty check in of_overlay_global_fixup_dir Marco Felsch
2024-03-22 16:49 ` [PATCH 8/8] of: overlay: replace filename with an more unique name Marco Felsch

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZoKbsY22cvuY-VUj@pengutronix.de \
    --to=s.hauer@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --cc=m.felsch@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox