Re: [PATCH 6/8] of: overlay: add FIT overlay support

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

On 24-03-25, Sascha Hauer wrote:
> On Fri, Mar 22, 2024 at 05:49:51PM +0100, Marco Felsch wrote:
> > This adds the support to load devicetree overlays from an FIT image.
> > There are quite a few options to handle FIT overlays since the FIT
> > overlay spec is not very strict.
> > 
> > This patch implement the most configurable case where each overlay does
> > have it's own config node (including the optional signature).
> > 
> > - The "name" filter check is performed on the config-node name (the node
> >   under the configurations) and not the FIT overlay image name (the node
> >   name under the images node).
> > - The "content" filter check does not differ from the file based overlay
> >   handling.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/of/overlay.c | 110 ++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 103 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > index e9fd5c0a1f7d..c8e70ab00091 100644
> > --- a/drivers/of/overlay.c
> > +++ b/drivers/of/overlay.c
> > @@ -8,10 +8,12 @@
> >   */
> >  #define pr_fmt(fmt) "of_overlay: " fmt
> >  
> > +#include <bootm.h>
> >  #include <common.h>
> >  #include <of.h>
> >  #include <errno.h>
> >  #include <globalvar.h>
> > +#include <image-fit.h>
> >  #include <magicvar.h>
> >  #include <string.h>
> >  #include <libfile.h>
> > @@ -473,9 +475,103 @@ static int of_overlay_global_fixup_dir(struct device_node *root, const char *ovl
> >  	return ret;
> >  }
> >  
> > +static int of_overlay_apply_fit(struct device_node *root, struct fit_handle *fit,
> > +				struct device_node *config)
> > +{
> > +	const char *name = config->name;
> > +	struct device_node *overlay;
> > +	unsigned long ovl_sz;
> > +	const void *ovl;
> > +	int ret;
> > +
> > +	if (!fit_has_image(fit, config, "fdt"))
> > +		return 0;
> > +
> > +	if (!of_overlay_matches_filter(name, NULL))
> > +		return 0;
> > +
> > +	ret = fit_open_image(fit, config, "fdt", &ovl, &ovl_sz);
> > +	if (ret)
> > +		return ret;
> > +
> > +	overlay = of_unflatten_dtb(ovl, ovl_sz);
> > +
> > +	if (!of_overlay_matches_filter(NULL, overlay)) {
> > +		ret = 0;
> > +		goto out;
> > +	}
> > +
> > +	ret = of_overlay_apply_tree(root, overlay);
> > +	if (ret == -ENODEV)
> > +		pr_debug("Not applied %s (not compatible)\n", name);
> > +	else if (ret)
> > +		pr_err("Cannot apply %s: %s\n", name, strerror(-ret));
> > +	else
> > +		pr_info("Applied %s\n", name);
> > +
> > +out:
> > +	of_delete_node(overlay);
> > +
> > +	return ret;
> > +}
> > +
> > +static int of_overlay_global_fixup_fit(struct device_node *root, const char *ovl_dev)
> > +{
> > +	enum bootm_verify verify = bootm_get_verify_mode();
> > +	struct device_node *conf_node;
> > +	struct fit_handle *fit;
> > +	struct stat s;
> > +	int ret;
> > +
> > +	if (!IS_ENABLED(CONFIG_FITIMAGE))
> > +		return 0;
> > +
> > +	if (stat(of_overlay_path, &s))
> > +		return -errno;
> 
> Why this? The caller already checked for existence of of_overlay_path.
> Besides, it is not even used in this function.

Yes, good point. I wanted to make the of_overlay_global_fixup_{dir,fit}
APIs symmetrical but I think we can drop this.

> > +
> > +	fit = fit_open(ovl_dev, 0, verify, s.st_size);
> > +	if (IS_ERR(fit)) {
> > +		pr_err("Loading FIT image %s failed with: %pe\n", ovl_dev, fit);
> > +		return PTR_ERR(fit);
> > +	}
> 
> Are you anticipating taking only the overlays from a FIT image and the
> kernel coming from somewhere else? Otherwise I would expect the
> integration to happen in the bootm and FIT code where we already have a
> handle to the opened FIT image. It seems wasteful to open the same FIT
> image here again.

I thought about this too but didn't went this way since it would not
allow us to patch the barebox-dt with overlays coming from the fit nor
is it possible to run the of_overlay command. It shouldn't matter to the
end-user if the overlay is coming from the (root)fs or the fit-image.

> > +	for_each_child_of_node(fit->configurations, conf_node) {
> > +		if (!fit_config_is_overlay(conf_node))
> > +			continue;
> > +
> > +		ret = fit_config_verify_signature(fit, conf_node);
> > +		if (ret)
> > +			goto out;
> > +
> > +		ret = of_overlay_apply_fit(root, fit, conf_node);
> > +		if (ret)
> > +			goto out;
> > +	}
> > +
> > +out:
> > +	fit_close(fit);
> > +	return ret;
> > +}
> > +
> >  static int of_overlay_global_fixup(struct device_node *root, void *data)
> >  {
> > -	return of_overlay_global_fixup_dir(root, of_overlay_path);
> > +	struct stat s;
> > +
> > +	if (isempty(of_overlay_path))
> > +		return 0;
> > +
> > +	if (stat(of_overlay_path, &s)) {
> > +		pr_err("Failed to detect file status\n");
> > +		return -errno;
> > +	}
> > +
> > +	if (S_ISDIR(s.st_mode))
> > +		return of_overlay_global_fixup_dir(root, of_overlay_path);
> > +	else if (S_ISCHR(s.st_mode) || S_ISBLK(s.st_mode))
> > +		return of_overlay_global_fixup_fit(root, of_overlay_path);
> 
> Why must the FIT image providing overlays be on a plain block device?
> Shouldn't we allow FIT images to live in a filesystem?

Good point, I didn't considered this since fit-images are mostly used on
secure/verified-boot devices. These devices often have a plain fit-image
partition.

> Anyway, as said I think this is the wrong place to implement this. When
> opening a FIT image it's already clear that we should take the overlays
> from that image, and not open some image again.

Please consider the use-cases listed above as well. If we strictly bind
it to the bootm command we can't do the live-patching of barebox dtbs
and IMHO we shouldn't care if the overlay is coming from the FIT or an
FS.

Regards,
  Marco