[RFC] libfdt: add fdt_alignprop

[RFC] libfdt: add fdt_alignprop

[Bjørn Mork]

Properties are sometimes used to store data with stricter alignment
requirements than the 4-byte fdt tag alignment. Aligning the offset
of the property data will alloe a client to directly address it,
without reloaction, provided the fdt is loaded on a similarily
aligned boundary.

One known use-case for this is the U-Boot FIT images, which may
embed nested fdt blobs inside properties of the outer FIT fdt.
Although strongly discouraged, U-Boot can be configured to use these
embedded blobs "in place". This is discouraged because if only will
work if the offset of the property value, i.e. the embedded fdt,
happens to match the 8-byte boundary required by the devicetree
spec.

Adding the ability to align property data allows U-Boot tools to
reliably create FIT images for such use.

Other use cases are currently not known, but anticipated.

Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
---
Throwing out this idea as an RFC here to get an idea whether
this route is worth following or not.  I've not yet tested this
on the U-Boot community so I don't know if they actually want
it.

But I would not be surprised if people find other uses for this
than my example use case.

Please comment, if only to say "go away!" :-)


Bjørn


 libfdt/fdt_rw.c          | 26 ++++++++++++++++++++++++++
 libfdt/fdt_wip.c         |  2 +-
 libfdt/libfdt.h          | 30 ++++++++++++++++++++++++++++++
 libfdt/libfdt_internal.h |  1 +
 4 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
index 3621d3651d3f..797132cfef74 100644
--- a/libfdt/fdt_rw.c
+++ b/libfdt/fdt_rw.c
@@ -330,6 +330,32 @@ int fdt_delprop(void *fdt, int nodeoffset, const char *name)
 	return fdt_splice_struct_(fdt, prop, proplen, 0);
 }
 
+int fdt_alignprop(void *fdt, int nodeoffset, const char *name, size_t align)
+{
+	struct fdt_property *prop;
+	int err, len, diff;
+
+	FDT_RW_PROBE(fdt);
+
+	if (align != FDT_TAGALIGN(align))
+		return -FDT_ERR_BADOFFSET;
+
+	prop = fdt_get_property_w(fdt, nodeoffset, name, &len);
+	if (!prop)
+		return len;
+
+	diff = FDT_ALIGN((uintptr_t)prop->data, align) - (uintptr_t)prop->data;
+	if (!diff)
+		return 0;
+
+	err = fdt_splice_struct_(fdt, prop, 0, diff);
+	if (err)
+		return err;
+
+	fdt_nop_region_(prop, diff);
+	return 0;
+}
+
 int fdt_add_subnode_namelen(void *fdt, int parentoffset,
 			    const char *name, int namelen)
 {
diff --git a/libfdt/fdt_wip.c b/libfdt/fdt_wip.c
index c2d7566a67dc..10a75058f0f0 100644
--- a/libfdt/fdt_wip.c
+++ b/libfdt/fdt_wip.c
@@ -48,7 +48,7 @@ int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name,
 						   val, len);
 }
 
-static void fdt_nop_region_(void *start, int len)
+void fdt_nop_region_(void *start, int len)
 {
 	fdt32_t *p;
 
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index d0a2ed2741f3..6c4060c9e492 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -2016,6 +2016,36 @@ int fdt_appendprop_addrrange(void *fdt, int parent, int nodeoffset,
  */
 int fdt_delprop(void *fdt, int nodeoffset, const char *name);
 
+/**
+ * fdt_alignprop - align property data to a given boundary
+ * @fdt: pointer to the device tree blob
+ * @nodeoffset: offset of the node whose property to nop
+ * @name: name of the property to nop
+ * @align: requested alignment boundary in bytes (must be multiple of 4)
+ *
+ * fdt_alignprop() will align the property data to a chosen boundary
+ * by injecting nop tags in front of the given property.  The
+ * alignment must be a multiple of 4.
+ *
+ * This function may insert nop tags in the blob, and will therefore
+ * change the offsets of some existing nodes.
+ *
+ * returns:
+ *	0, on success
+ *	-FDT_ERR_NOSPACE, there is insufficient free space in the blob to
+ *		move the property to the new boundary
+ *	-FDT_ERR_NOTFOUND, node does not have the named property
+ *	-FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
+ *		or the requested alignment was not a multiple of 4
+ *	-FDT_ERR_BADLAYOUT,
+ *	-FDT_ERR_BADMAGIC,
+ *	-FDT_ERR_BADVERSION,
+ *	-FDT_ERR_BADSTATE,
+ *	-FDT_ERR_BADSTRUCTURE,
+ *	-FDT_ERR_TRUNCATED, standard meanings
+ */
+int fdt_alignprop(void *fdt, int nodeoffset, const char *name, size_t align);
+
 /**
  * fdt_add_subnode_namelen - creates a new node based on substring
  * @fdt: pointer to the device tree blob
diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
index 16bda1906a7b..ce4aedd19965 100644
--- a/libfdt/libfdt_internal.h
+++ b/libfdt/libfdt_internal.h
@@ -22,6 +22,7 @@ int fdt_check_node_offset_(const void *fdt, int offset);
 int fdt_check_prop_offset_(const void *fdt, int offset);
 const char *fdt_find_string_(const char *strtab, int tabsize, const char *s);
 int fdt_node_end_offset_(void *fdt, int nodeoffset);
+void fdt_nop_region_(void *start, int len);
 
 static inline const void *fdt_offset_ptr_(const void *fdt, int offset)
 {
-- 
2.30.2

Re: [RFC] libfdt: add fdt_alignprop

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6567 bytes --]

On Sun, Nov 06, 2022 at 03:12:47PM +0100, Bjørn Mork wrote:
> Properties are sometimes used to store data with stricter alignment
> requirements than the 4-byte fdt tag alignment. Aligning the offset
> of the property data will alloe a client to directly address it,
> without reloaction, provided the fdt is loaded on a similarily
> aligned boundary.
> 
> One known use-case for this is the U-Boot FIT images, which may
> embed nested fdt blobs inside properties of the outer FIT fdt.
> Although strongly discouraged, U-Boot can be configured to use these
> embedded blobs "in place". This is discouraged because if only will
> work if the offset of the property value, i.e. the embedded fdt,
> happens to match the 8-byte boundary required by the devicetree
> spec.
> 
> Adding the ability to align property data allows U-Boot tools to
> reliably create FIT images for such use.
> 
> Other use cases are currently not known, but anticipated.
> 
> Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
> ---
> Throwing out this idea as an RFC here to get an idea whether
> this route is worth following or not.  I've not yet tested this
> on the U-Boot community so I don't know if they actually want
> it.
> 
> But I would not be surprised if people find other uses for this
> than my example use case.
> 
> Please comment, if only to say "go away!" :-)

Adjusting the positioning and filling with nop tags is a clever idea.
The main concern I have about it is that it's pretty fragile.

Calling fdt_alignprop() with align the property you requested, but
could unalign any property that lies after it in the blob (even in a
wholly unrelated node).  So if you need to align multiple properties
in a tree, you have to do it in order, which is a somewhat unnatural
mode of using libfdt, at least with the higher level functions.

Such alignment is alsonot likely to survive processing by other tools
(e.g. dtc), or indeed using any other read-write libfdt function.
Essentially alignprop calls would have to be done as just about the
last step in fdt preparation.

Given that, I wonder if we could come up with some interface that
enforces - or at least encourages - that use mode alone.  Maybe some
kind of finalize function that aligns every property that needs it
(according to a callback, maybe?) then finishes with an fdt_pack().

> Bjørn
> 
> 
>  libfdt/fdt_rw.c          | 26 ++++++++++++++++++++++++++
>  libfdt/fdt_wip.c         |  2 +-
>  libfdt/libfdt.h          | 30 ++++++++++++++++++++++++++++++
>  libfdt/libfdt_internal.h |  1 +
>  4 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> index 3621d3651d3f..797132cfef74 100644
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -330,6 +330,32 @@ int fdt_delprop(void *fdt, int nodeoffset, const char *name)
>  	return fdt_splice_struct_(fdt, prop, proplen, 0);
>  }
>  
> +int fdt_alignprop(void *fdt, int nodeoffset, const char *name, size_t align)
> +{
> +	struct fdt_property *prop;
> +	int err, len, diff;
> +
> +	FDT_RW_PROBE(fdt);
> +
> +	if (align != FDT_TAGALIGN(align))
> +		return -FDT_ERR_BADOFFSET;
> +
> +	prop = fdt_get_property_w(fdt, nodeoffset, name, &len);
> +	if (!prop)
> +		return len;
> +
> +	diff = FDT_ALIGN((uintptr_t)prop->data, align) - (uintptr_t)prop->data;
> +	if (!diff)
> +		return 0;
> +
> +	err = fdt_splice_struct_(fdt, prop, 0, diff);
> +	if (err)
> +		return err;
> +
> +	fdt_nop_region_(prop, diff);
> +	return 0;
> +}
> +
>  int fdt_add_subnode_namelen(void *fdt, int parentoffset,
>  			    const char *name, int namelen)
>  {
> diff --git a/libfdt/fdt_wip.c b/libfdt/fdt_wip.c
> index c2d7566a67dc..10a75058f0f0 100644
> --- a/libfdt/fdt_wip.c
> +++ b/libfdt/fdt_wip.c
> @@ -48,7 +48,7 @@ int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name,
>  						   val, len);
>  }
>  
> -static void fdt_nop_region_(void *start, int len)
> +void fdt_nop_region_(void *start, int len)
>  {
>  	fdt32_t *p;
>  
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index d0a2ed2741f3..6c4060c9e492 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -2016,6 +2016,36 @@ int fdt_appendprop_addrrange(void *fdt, int parent, int nodeoffset,
>   */
>  int fdt_delprop(void *fdt, int nodeoffset, const char *name);
>  
> +/**
> + * fdt_alignprop - align property data to a given boundary
> + * @fdt: pointer to the device tree blob
> + * @nodeoffset: offset of the node whose property to nop
> + * @name: name of the property to nop
> + * @align: requested alignment boundary in bytes (must be multiple of 4)
> + *
> + * fdt_alignprop() will align the property data to a chosen boundary
> + * by injecting nop tags in front of the given property.  The
> + * alignment must be a multiple of 4.
> + *
> + * This function may insert nop tags in the blob, and will therefore
> + * change the offsets of some existing nodes.
> + *
> + * returns:
> + *	0, on success
> + *	-FDT_ERR_NOSPACE, there is insufficient free space in the blob to
> + *		move the property to the new boundary
> + *	-FDT_ERR_NOTFOUND, node does not have the named property
> + *	-FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
> + *		or the requested alignment was not a multiple of 4
> + *	-FDT_ERR_BADLAYOUT,
> + *	-FDT_ERR_BADMAGIC,
> + *	-FDT_ERR_BADVERSION,
> + *	-FDT_ERR_BADSTATE,
> + *	-FDT_ERR_BADSTRUCTURE,
> + *	-FDT_ERR_TRUNCATED, standard meanings
> + */
> +int fdt_alignprop(void *fdt, int nodeoffset, const char *name, size_t align);
> +
>  /**
>   * fdt_add_subnode_namelen - creates a new node based on substring
>   * @fdt: pointer to the device tree blob
> diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
> index 16bda1906a7b..ce4aedd19965 100644
> --- a/libfdt/libfdt_internal.h
> +++ b/libfdt/libfdt_internal.h
> @@ -22,6 +22,7 @@ int fdt_check_node_offset_(const void *fdt, int offset);
>  int fdt_check_prop_offset_(const void *fdt, int offset);
>  const char *fdt_find_string_(const char *strtab, int tabsize, const char *s);
>  int fdt_node_end_offset_(void *fdt, int nodeoffset);
> +void fdt_nop_region_(void *start, int len);
>  
>  static inline const void *fdt_offset_ptr_(const void *fdt, int offset)
>  {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC] libfdt: add fdt_alignprop

[Bjørn Mork]

David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> writes:

> Adjusting the positioning and filling with nop tags is a clever idea.
> The main concern I have about it is that it's pretty fragile.
>
> Calling fdt_alignprop() with align the property you requested, but
> could unalign any property that lies after it in the blob (even in a
> wholly unrelated node).  So if you need to align multiple properties
> in a tree, you have to do it in order, which is a somewhat unnatural
> mode of using libfdt, at least with the higher level functions.

Yes, there's an underlying assumption that aligned properties are
processed in order. I guess this should be part of the docs.  Or does
it prevent the creation of a high level API altogether?

> Such alignment is alsonot likely to survive processing by other tools
> (e.g. dtc), or indeed using any other read-write libfdt function.
> Essentially alignprop calls would have to be done as just about the
> last step in fdt preparation.

Yes, depending on property alignment is fragile by design. No doubt
about that. It will break with any sort of normal processing like
reordering etc. U-Boot discourage the use of their "in place" fdt
feature for good reasons.

But the feature exists and is in use.

I guess reliable alignment, regardless of tool, would require some hint
inside the blob?  That's unnecessarily complicated IMHO.  At least for
FIT images/U-Boot.  Any sort of manipulation by dtc of the final FIT
blob is out of the question anyway.

The FIT spec includes a signing algorithm which absolutely must be the
last modification of the blob.  Another unfortunate design...

FIT signing works by hashing sectons of the binary blob.  A more robust
way would have been an ordered list of properties to sign, similar to
DKIM email header signatures for example.  The FIT hashed data even
includes FDT_NOP tags. So any sort of normal manipulation, like
reordering properties or injecting FDT_NOP, will "break" the blob from
the U-Boot consumer's point of view.

To make things even worse, the signatures are coded as variable length
nodes, inserted into the blob at a number of parent nodes. I'm currently
signing twice and re-aligning in-between the steps.  The first step adds
signature nodes with their final size and order.  Only then can we
calculate the proper alignment. Last, if aligning changed the blob, then
we have to re-sign to update the signature nodes.  The order and size of
every object is constant in this last step, fortunately.

dtc can be (and is) used to prepare initial FIT images.  But it cannot
touch images after mkimage is finished post-processing them, whether or
not we add an alignment policy. Only mkimage can do that.

Which is why I believe it's fine to have the specific FIT alignment
policy coded into mkimage, and not stored inside the blob itself.

That's the background for my simple best effort proposal

> Given that, I wonder if we could come up with some interface that
> enforces - or at least encourages - that use mode alone.  Maybe some
> kind of finalize function that aligns every property that needs it
> (according to a callback, maybe?) then finishes with an fdt_pack().

Yes, this would be great. I just couldn't come up with any working
proposal.

But I don't think it makes much difference for the U-Boot case.  Users
manipulating FIT images with dtc will be able to mess up.  I'm happy if
I can make the mkimage tool safe for FIT images. And fdt_alignprop() is
sufficient for that.

If someone has generic use cases, then the picture is different of
course.




Bj√∏rn