Restore phandles from binary representations #151

ukleinek · 2024-10-21T07:45:40Z

device trees generated with options -@ and -L contain information about the label names used originally and which values are phandle values. This information can be reused when compiling back to dts format to make the result much more human readable.

Also dtdiff is adapted to make use of this to generate smaller diffs without hunks e.g. for a different phandle allocation.

This superseeds pull request #93.

ukleinek · 2024-10-21T08:02:34Z

A possible followup improvement would be to restore phandles from __fixup__ .

ukleinek · 2024-10-24T06:11:09Z

Just a quick headsup: I found a dtbo file where my code crashes with -@ -L. So don't merge yet please.

ukleinek · 2024-10-24T06:29:09Z

OK, I think that's unrelated to my changes and just presents a situation to the decompiler that didn't happen before. Here is a reproducer:

$ git diff
diff --git a/treesource.c b/treesource.c
index 067647b60a17..fa1a48579489 100644
--- a/treesource.c
+++ b/treesource.c
@@ -104,6 +104,12 @@ static void write_propval_string(FILE *f, const char *s, size_t len)
 static void write_propval_int(FILE *f, const char *p, size_t len, size_t width)
 {
        const char *end = p + len;
+
+       if (len % width) {
+               fprintf(stderr, "Huh, len = %zu, width = %zu\n", len, width);
+               exit(1);
+       }
+
        assert(len % width == 0);
 
        for (; p < end; p += width) {

uwe@taurus:~/dts-decompile-labels$ cat test3.dts
/dts-v1/;

/ {
	tralala = <&somelabel>, "text making the next label unaligned",
		<&somelabel>, "some more text";

	somelabel: somenode {

	};
};
$ dtc -@ -L -I dts -O dtb test3.dts > test3.dtb
$ dtc -@ -L -I dtb -O dts test3.dtb 
/dts-v1/;

/ {
Huh, len = 37, width = 4
	tralala = <&somelabel

So I think some heuristic considers the property "tralala" an array of ints but that doesn't handle that it ends at offset 41 which is unaligned.

dgibson

I think this is a great idea. The implementation looks mostly sound but there are a few minor issues that need addressing.

dgibson · 2024-10-28T09:02:40Z

dtc.c

 		generate_label_tree(dti, "__symbols__", true);
+		generate_labels_from_tree(dti, "__symbols__");


I'd not immediately sure if it make sense to only do this when generate_symbols is set.

Maybe do it unconditionally and remove the __symbols__ node if generate_symbols is set?

Yes, I think that makes sense.

dgibson · 2024-10-28T09:05:52Z

livetree.c

+		if (labeled_node)
+			add_label(&labeled_node->labels, p->name);
+		else if (quiet < 1)
+			fprintf(stderr, "Warning: Path %s referenced in %s missing",


I'd suggest suggest an explicit "referenced in symbol %s" to make it clearer what's going on without context.

I guess you expect that p->name is passed then for your %s? I think mentioning name is still a good idea, so I suggest to make this

fprintf(stderr, "Warning: Path %s referenced in property %s/%s missing", p->val.val, name, p->name);

dgibson · 2024-10-28T09:06:48Z

dtc.c

@@ -344,6 +344,7 @@ int main(int argc, char *argv[])
 	if (generate_fixups) {
 		generate_fixups_tree(dti, "__fixups__");
 		generate_local_fixups_tree(dti, "__local_fixups__");
+		local_fixup_phandles(dti, "__local_fixups__");


Again, not sure if it only makes sense to do this with -L.

As with __symbols__ I think it probably makes sense to do this unconditionally, then strip it out of -L is not given.

dgibson · 2024-10-28T09:08:33Z

livetree.c

@@ -3,6 +3,8 @@
 * (C) Copyright David Gibson <[email protected]>, IBM Corporation.  2005.
 */

+#include <libfdt/libfdt.h>
+


Oof. We don't currently use libfdt within dtc, which is sort-of deliberate (they provide a cross-check to each other). There's a case to be made to drop that policy and instead use libfdt for much of the flat tree logic within dtc. However, I'd really prefer to do that as an overall change, using it for as much as we can, rather than adding small pieces of libfdt usage ad-hoc.

..also, I can't actually see what you're using this for.

Maybe this is only a development relict, will try without.

FTR: Just dropping the #include doesn't work. Have to take a look at a more appropriate time.

livetree.c

dgibson · 2024-10-28T09:19:28Z

treesource.c

+			m->ref = refn->fullpath;
+	} else if (quiet < 1) {
+		fprintf(stderr, "Warning: node referenced by phandle 0x%x in property %s not found\n",
+			phandle, prop->name);


I think you do need to consider __fixups__ as well. Without that, this error will be triggered if you're decompiling a dtbo with any external phandle references.

dtdiff

dgibson · 2024-10-28T09:22:59Z

A possible followup improvement would be to restore phandles from __fixup__ .

I think you do need to implement this. For one thing processing but __local_fixups__ but not __fixups__ would just be surprising behaviour to a user, I think. Secondly, it could cause some bogus errors in some cases (see detailed comments).

ukleinek · 2024-10-28T09:29:26Z

Do you have an idea for the unaligned phandle problem?

dgibson · 2024-10-28T09:43:17Z

Do you have an idea for the unaligned phandle problem?

Sorry, I'm not sure exactly what problem you mean.

ukleinek · 2024-10-28T14:08:06Z

Do you have an idea for the unaligned phandle problem?

Sorry, I'm not sure exactly what problem you mean.

I mean the crash reported in #151 (comment) with the follow up in the next comment.

dgibson · 2024-10-30T02:07:48Z

Do you have an idea for the unaligned phandle problem?

Sorry, I'm not sure exactly what problem you mean.

I mean the crash reported in #151 (comment) with the follow up in the next comment.

Ah, right, sorry.

I think the basic problem is that previously the only case where we'd have phandle markers during decompile is with -I dts -O dts, in which case we'd also have type markers helping us figure out how to print each part of each property. The new code is putting in phandle markers, but without type markers. We should handle that, of course, but looks like there are some edge cases.

More specifically, it looks like write_propval() calls guess_value_type() to figure out a missing type only once for the whole property. Having a phandle marker does tell us that those specific 4 bytes should be in integer < ... > context. But we'll need to separately guess the type for each chunk between phandles. I suspect that might be irritatingly fiddly, but I'm afraid it's going to be a prerequisite for implementing this.

ukleinek · 2025-01-12T01:24:39Z

This addresses the crash mentioned in #151 (comment) and a few suggestions by @dgibson (which I marked as resolved). Still missing is restoring of phandles from __fixups__.

treesource.c

dgibson · 2025-01-13T02:59:57Z

treesource.c

@@ -178,7 +177,9 @@ static enum markertype guess_value_type(struct property *prop)
 	}

 	for_each_marker_of_type(m, LABEL) {
-		if ((m->offset > 0) && (prop->val.val[m->offset - 1] != '\0'))
+		if (m->offset < offset || m->offset >= offset + len)
+			continue;


I think there's another thing to consider here too. IIUC this is now attempting to guess the type of just the sub-piece of the property given by offset and len.

The tests below are classifying things as "not a string" or "not cells" based on labels which aren't plausibly aligned for a string or label type. However the presence of the label affects of the classification of the data before the label, not after the label. So because you've already excluded those labels, they'll no longer have the effect they need to.

Either I'm not following you, or you're wrong. If there is a label in the middle of a data chunk, I consider that a boundary and guess the types of the two chunks separately. IMHO that makes sense because a label (or a phandle) is very likely a boundary for a type change.

dgibson · 2025-01-13T03:02:54Z

dtc.c

@@ -344,6 +344,7 @@ int main(int argc, char *argv[])
 	if (generate_fixups) {
 		generate_fixups_tree(dti, "__fixups__");
 		generate_local_fixups_tree(dti, "__local_fixups__");
+		local_fixup_phandles(dti, "__local_fixups__");


As with __symbols__ I think it probably makes sense to do this unconditionally, then strip it out of -L is not given.

ukleinek · 2025-01-14T11:24:02Z

OK, this got bigger now than I anticipated and also contains a few fixes, but I'm happy that I did it now and I can get it out of my head and happily use it.

Quick demo:

$ cat test.dts
/dts-v1/;
/plugin/;

/ {
	tralal = <&somelabel>, "text making the",
		<&somelabel>, "some more text";

	tralala = <&somelabel>, "text making the next label unaligned",
		<&somelabel>, "some more text";

	somelabel: somenode {
		property = <&nonexisting>;
	};
};
$ dtc -@ test.dts > test.dtb

Decompiling this dtb with dtc 1.7.2 gives you:

$ /usr/bin/dtc test.dtb
/dts-v1/;

/ {
	tralal = [00 00 00 01 74 65 78 74 20 6d 61 6b 69 6e 67 20 74 68 65 00 00 00 00 01 73 6f 6d 65 20 6d 6f 72 65 20 74 65 78 74 00];
	tralala = <0x01 0x74657874 0x206d616b 0x696e6720 0x74686520 0x6e657874 0x206c6162 0x656c2075 0x6e616c69 0x676e6564 0x00 0x1736f6d 0x65206d6f 0x72652074 0x65787400>;

	somenode {
		property = <0xffffffff>;
		phandle = <0x01>;
	};

	__symbols__ {
		somelabel = "/somenode";
	};

	__fixups__ {
		nonexisting = "/somenode:property:0";
	};

	__local_fixups__ {
		tralal = <0x00 0x14>;
		tralala = <0x00 0x29>;
	};
};

With the changes from this PR applied it gets:

$ dtc test.dtb
/dts-v1/;
/plugin/;

/ {
	tralal = <&somelabel>, "text making the", <&somelabel>, "some more text";
	tralala = <&somelabel>, "text making the next label unaligned", <&somelabel>, "some more text";

	somelabel: somenode {
		property = <&nonexisting>;
		phandle = <0x01>;
	};

	__symbols__ {
		somelabel = "/somenode";
	};

	__fixups__ {
		nonexisting = "/somenode:property:0";
	};

	__local_fixups__ {
		tralal = <0x00 0x14>;
		tralala = <0x00 0x29>;
	};
};

and if test.dtb was compiled without -@ it uses &{/somenode} instead of &somelabel.

ukleinek · 2025-01-15T09:17:13Z

BTW, I didn't implement stripping out the __fixups__, __local_fixups__ and __symbols__ nodes. Mostly because it doesn't seem to be right for -I dts -O dts compilations. Instead the two fixup nodes are removed before they are generated to make sure that the information isn't duplicated.

dgibson

I've applied the first three patches, because they're sensible fixes independent of the rest of the series. I've made some comments on the next few, but I haven't completed a full review yet.

dgibson · 2025-01-24T03:21:02Z

treesource.c

+	fprintf(f, "/dts-v1/;\n");
+	if (any_fixup_tree(dti, dti->dt))
+		fprintf(f, "/plugin/;\n");
+	fprintf(f, "\n");


I think this is correct, but I seems like a very indirect way of diong it. We're taking the __fixups__ node, distributing across the tree as a bunch of fixup markers, then scanning the entire tree for fixup markers to mark it as /plugin/.

I think a clear way to do this would be to set the DTSF_PLUGIN flag on dtb input if there is a (non empty) __fixups__ or __local_fixups__ node, then on dts output output the /plugin/ tag based on that flag.

dgibson · 2025-01-24T03:23:12Z

tests/over-determined.dts

@@ -0,0 +1,23 @@
+/dts-v1/;


This commit definitely needs a proper commit message. What's the rationale for this test?

dgibson · 2025-01-24T03:24:07Z

treesource.c

@@ -139,26 +139,49 @@ static const char *delim_end[] = {
 	[TYPE_STRING] = "",
 };

+/*
+ * The invariants in the marker list are:
+ *  - offsets are monotonically rising


Maybe "non-strictly monotonically increasing", since adjacent offsets can be equal.

dgibson · 2025-01-24T03:25:59Z

treesource.c

+ * The invariants in the marker list are:
+ *  - offsets are monotonically rising
+ *  - for a single offset there is at most one type marker
+ *  - for a single offset there is at most one non-type marker


This one isn't true.

prop = &somenode, <&othernode>;

Will have the REF_PATH and REF_PHANDLE markers at the same offset, until the REF_PATH marker is resolved (before that it's essentially marking a region of zero length).

With the next few commits phandles are restored when compiling to dts. Depending on the source these phandles might be undefined. To allow recompiling the generated source, mark the dts as "plugin" in the presence of undefined phandles. Otherwise compilation fails with Reference to non-existent node or label "nonexisting" . Signed-off-by: Uwe Kleine-König <[email protected]>

A dts file that is over-determined in the sense that it already contains both a __local_fixups__ and/or a __fixups__ node and phandles that result in entries in these nodes, should not compiled to a device tree that has duplicate entries. This was a problem before commit 915daad ("Start with empty __local_fixups__ and __fixups__ nodes"). Add a test that ensures this issue isn't reintroduced later. Signed-off-by: Uwe Kleine-König <[email protected]>

ukleinek · 2025-01-25T15:25:43Z

I've applied the first three patches, because they're sensible fixes independent of the rest of the series. I've made some comments on the next few, but I haven't completed a full review yet.

The topmost commit is also simple and orthogonal to the rest of the series. That one might be eligible for fast tracking, too.
I'll look into your other feedback later.

The add_marker() function is used to create a new marker and add it at the right spot to the relevant marker list. Use it in the add_string_markers() helper (which gets slightly quicker by it). Signed-off-by: Uwe Kleine-König <[email protected]>

In the presence of (non-type) markers guess the type of each chunk between markers individually instead of only once for the whole property. Note that this only gets relevant with the next few commits that restore labels and phandles. Note further that this rework is necessary with these further changes, because phandle markers are currently not considered for type guessing and so a phandle at an offset that isn't a multiple of 4 triggers an assertion if the property was guessed to have type TYPE_UINT32. Now that guess_value_type() is only called for data chunks without markers, the function can be simplified a bit. Signed-off-by: Uwe Kleine-König <[email protected]>

If the input has a __symbols__ node, restore the named labels for the respective nodes. Signed-off-by: Uwe Kleine-König <[email protected]>

The __local_fixups__ node contains information about phandles. Parse it to improve the result when decompiling a device tree blob. Signed-off-by: Uwe Kleine-König <[email protected]>

The __fixups__ node contains information about labels. Parse its properties to create phandle markers which improve the resulting dts when decompiling a device tree blob. Signed-off-by: Uwe Kleine-König <[email protected]>

The file ending .dtbo is typically used for device tree overlays. These are in the dtb input format, too. So assume this input format for *.dtbo as is already done for *.dtb. Signed-off-by: Uwe Kleine-König <[email protected]>

ukleinek mentioned this pull request Oct 21, 2024

Device tree overlays cleanup analogdevicesinc/linux#2624

Merged

ukleinek mentioned this pull request Oct 21, 2024

Some phandle related improvements and some bug fixes #93

Closed

dgibson requested changes Oct 28, 2024

View reviewed changes

dgibson mentioned this pull request Oct 30, 2024

Improve dtdiff by avoiding or nulling the phandles #145

Open

ukleinek force-pushed the restore_phandles branch from 845fb23 to 07f279e Compare January 12, 2025 01:20

dgibson requested changes Jan 13, 2025

View reviewed changes

ukleinek force-pushed the restore_phandles branch from 07f279e to 5c04a2d Compare January 14, 2025 11:04

ukleinek force-pushed the restore_phandles branch from 5c04a2d to 9827002 Compare January 14, 2025 11:35

dgibson reviewed Jan 24, 2025

View reviewed changes

Uwe Kleine-König added 2 commits January 24, 2025 22:33

Uwe Kleine-König added 6 commits January 25, 2025 16:45

New helper to add markers

3a1041c

The add_marker() function is used to create a new marker and add it at the right spot to the relevant marker list. Use it in the add_string_markers() helper (which gets slightly quicker by it). Signed-off-by: Uwe Kleine-König <[email protected]>

Restore labels from __symbols__ node

54b1603

If the input has a __symbols__ node, restore the named labels for the respective nodes. Signed-off-by: Uwe Kleine-König <[email protected]>

Restore phandle references from __local_fixups__ node

004a5fe

The __local_fixups__ node contains information about phandles. Parse it to improve the result when decompiling a device tree blob. Signed-off-by: Uwe Kleine-König <[email protected]>

Restore phandle references from __fixups__ node

8465902

The __fixups__ node contains information about labels. Parse its properties to create phandle markers which improve the resulting dts when decompiling a device tree blob. Signed-off-by: Uwe Kleine-König <[email protected]>

dtdiff: Use input format dtb for dtbo files

c21b18f

The file ending .dtbo is typically used for device tree overlays. These are in the dtb input format, too. So assume this input format for *.dtbo as is already done for *.dtb. Signed-off-by: Uwe Kleine-König <[email protected]>

ukleinek force-pushed the restore_phandles branch from 9827002 to c21b18f Compare January 25, 2025 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore phandles from binary representations #151

Restore phandles from binary representations #151

ukleinek commented Oct 21, 2024

ukleinek commented Oct 21, 2024

ukleinek commented Oct 24, 2024

ukleinek commented Oct 24, 2024

dgibson left a comment

dgibson Oct 28, 2024

ukleinek Jan 12, 2025

dgibson Jan 13, 2025

dgibson Oct 28, 2024

ukleinek Jan 12, 2025

dgibson Oct 28, 2024

dgibson Jan 13, 2025

dgibson Oct 28, 2024

ukleinek Oct 28, 2024

ukleinek Jan 12, 2025

dgibson Oct 28, 2024

dgibson commented Oct 28, 2024

ukleinek commented Oct 28, 2024

dgibson commented Oct 28, 2024

ukleinek commented Oct 28, 2024

dgibson commented Oct 30, 2024

ukleinek commented Jan 12, 2025

dgibson Jan 13, 2025

ukleinek Jan 14, 2025

dgibson Jan 13, 2025

ukleinek commented Jan 14, 2025

ukleinek commented Jan 15, 2025

dgibson left a comment

dgibson Jan 24, 2025

dgibson Jan 24, 2025

dgibson Jan 24, 2025

dgibson Jan 24, 2025

ukleinek commented Jan 25, 2025

		generate_label_tree(dti, "__symbols__", true);
		generate_labels_from_tree(dti, "__symbols__");

Restore phandles from binary representations #151

Are you sure you want to change the base?

Restore phandles from binary representations #151

Conversation

ukleinek commented Oct 21, 2024

ukleinek commented Oct 21, 2024

ukleinek commented Oct 24, 2024

ukleinek commented Oct 24, 2024

dgibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgibson commented Oct 28, 2024

ukleinek commented Oct 28, 2024

dgibson commented Oct 28, 2024

ukleinek commented Oct 28, 2024

dgibson commented Oct 30, 2024

ukleinek commented Jan 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ukleinek commented Jan 14, 2025

ukleinek commented Jan 15, 2025

dgibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ukleinek commented Jan 25, 2025