Fix reserve map output for asm format.

PATCH In review Jerry Van Baren 4 participants 9 messages Apr 15, 2007 – Apr 16, 2007

lore

Share & tools

lore

Jerry Van Baren

author

2007-04-15 02:38 UTC #0 lore raw

PATCH 1/1 Fix reserve map output for asm format.

Add extra reserve map slots output for asm format (previously done for dtb
output).

Signed-off-by Gerald Van Baren <[email protected]>

---

Hi Jon, David,

Here is a patch that fixes the asm output without the (unnecessary) calloc change.

Best regards,
gvb

 flattree.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

flattree.c +11 -3

index 151d16e..6f0c9b7 100644
@@ -21,6 +21,9 @@
 #include "dtc.h"
 #include "flat_dt.h"
 
+#define WARNMSG(...) if (quiet < 1) fprintf(stderr, "Warning: " __VA_ARGS__)
+
+
 #define FTF_FULLPATH	0x1
 #define FTF_VARALIGN	0x2
 #define FTF_NAMEPROPS	0x4

@@ -490,6 +493,9 @@ void dt_to_asm(FILE *f, struct boot_info *bi, int version, int boot_cpuid_phys)
 			(unsigned int)(re->re.size >> 32),
 			(unsigned int)(re->re.size & 0xffffffff));
 	}
+	for (i = 0; i < reservenum; i++) {
+		fprintf(f, "\t.long\t0, 0\n\t.long\t0, 0\n");
+	}
 
 	fprintf(f, "\t.long\t0, 0\n\t.long\t0, 0\n");

@@ -818,9 +824,11 @@ struct boot_info *dt_from_blob(FILE *f)
 	p = blob + sizeof(magic)  + sizeof(totalsize);
 
 	while (sizeleft) {
-		if (feof(f))
-			die("EOF before reading %d bytes of DT blob\n",
-			    totalsize);
+		if (feof(f)) {
+			WARNMSG("EOF after reading %d of %d bytes of DT blob, assuming there is extra space in the blob.\n",
+			    totalsize - sizeleft, totalsize);
+			break;
+		}
 
 		rc = fread(p, 1, sizeleft, f);
 		if (ferror(f))

Signature · 1 line

1.4.4.4

Milton Miller

2007-04-15 20:00 UTC #1 lore raw

Quoted 6 lines · Jerry Van Baren

Add extra reserve map slots output for asm format (previously done for dtb
output).
Signed-off-by Gerald Van Baren <vanbaren at cideas.com>
---

Quoted 7 lines

Hi Jon, David,
Here is a patch that fixes the asm output without the (unnecessary) calloc change.
Best regards,
gvb

The previous description had

Quoted 3 lines

Use cmalloc to pre-zero memory (for dtb input) and handle dtb (binary)
input being shorter than the total blob length (result of putting extra space in the blob).

Which at least said in the description the unrelated things it was doing.

flattree.c struct boot_info *dt_from_blob(FILE *f) · line 831

Quoted 10 lines

        while (sizeleft) {

flattree.c +2 -3

-               if (feof(f))
-                       die("EOF before reading %d bytes of DT blob\n",
-                           totalsize);
+               if (feof(f)) {
+                       WARNMSG("EOF after reading %d of %d bytes of

DT blob, assuming there is extra space in the blob.\n",

flattree.c +3 -0

+                           totalsize - sizeleft, totalsize);
+                       break;
+               }

I thnk the above should be an ERROR and cause failure without the -f (force) option.

The total_size says how much data should be copied. Anything
less and there is data missing. Assuming zeros is wrong for
most sections (the exception being the memory reserve list that had a terminating 0 entry within the read portion).

milton

Jerry Van Baren

2007-04-16 00:27 UTC #2 lore raw

Quoted 16 lines · Milton Miller

Quoted 6 lines · Jerry Van Baren
Add extra reserve map slots output for asm format (previously done for dtb
output).
Signed-off-by Gerald Van Baren <vanbaren at cideas.com>
---
Quoted 7 lines
Hi Jon, David,
Here is a patch that fixes the asm output without the (unnecessary) calloc change.
Best regards,
gvb

Quoted 7 lines

The previous description had
Quoted 3 lines
Use cmalloc to pre-zero memory (for dtb input) and handle dtb (binary)
input being shorter than the total blob length (result of putting extra space in the blob).
Which at least said in the description the unrelated things it was doing.

That was my added comment WRT the change from malloc to cmalloc. David wasn't wild about using cmalloc, so I removed it. Using cmalloc is not necessary.

Quoted 14 lines

flattree.c struct boot_info *dt_from_blob(FILE *f) · line 831

Quoted 10 lines

        while (sizeleft) {

flattree.c +2 -3

-               if (feof(f))
-                       die("EOF before reading %d bytes of DT blob\n",
-                           totalsize);
+               if (feof(f)) {
+                       WARNMSG("EOF after reading %d of %d bytes of

DT blob, assuming there is extra space in the blob.\n",

flattree.c +3 -0

+                           totalsize - sizeleft, totalsize);
+                       break;
+               }

I thnk the above should be an ERROR and cause failure without the -f (force) option.

Quoted 6 lines

The total_size says how much data should be copied. Anything
less and there is data missing. Assuming zeros is wrong for
most sections (the exception being the memory reserve list that had a terminating 0 entry within the read portion).
milton

The reason total_size is bigger than the actual size is because I created the blob with extra space using the -S parameter. It is intentionally bigger. The extra space is ignored by dtc when creating a dts/asm format output which is why cmalloc() is unnecessary.

I suppose we could require a -f force but I'm not wild about creating a nanny program. There is nothing wrong with the blob - it parses just fine. If there were problems with the blob contents, other errors would be raised.

Best regards,
gvb

David Gibson

2007-04-16 00:51 UTC #3 lore raw

Quoted 57 lines · Jerry Van Baren

Quoted 17 lines · Milton Miller
Quoted 7 lines · Jerry Van Baren
Add extra reserve map slots output for asm format (previously done for dtb
output).
Signed-off-by Gerald Van Baren <vanbaren at cideas.com>
---
Quoted 7 lines
Hi Jon, David,
Here is a patch that fixes the asm output without the (unnecessary) calloc change.
Best regards,
gvb
Quoted 8 lines
The previous description had
Quoted 3 lines
Use cmalloc to pre-zero memory (for dtb input) and handle dtb (binary)
input being shorter than the total blob length (result of putting extra space in the blob).
Which at least said in the description the unrelated things it was doing.
That was my added comment WRT the change from malloc to cmalloc. David wasn't wild about using cmalloc, so I removed it. Using cmalloc is not necessary.
Quoted 14 lines
flattree.c struct boot_info *dt_from_blob(FILE *f) · line 831
Quoted 10 lines
        while (sizeleft) {
flattree.c +2 -3
-               if (feof(f))
-                       die("EOF before reading %d bytes of DT blob\n",
-                           totalsize);
+               if (feof(f)) {
+                       WARNMSG("EOF after reading %d of %d bytes of 
DT blob, assuming there is extra space in the blob.\n",
flattree.c +3 -0
+                           totalsize - sizeleft, totalsize);
+                       break;
+               }
I thnk the above should be an ERROR and cause failure without the -f (force) option.
Quoted 6 lines
The total_size says how much data should be copied. Anything
less and there is data missing. Assuming zeros is wrong for
most sections (the exception being the memory reserve list that had a terminating 0 entry within the read portion).
milton
The reason total_size is bigger than the actual size is because I created the blob with extra space using the -S parameter. It is intentionally bigger. The extra space is ignored by dtc when creating a dts/asm format output which is why cmalloc() is unnecessary.

Quoted 4 lines

I suppose we could require a -f force but I'm not wild about creating a nanny program. There is nothing wrong with the blob - it parses just fine. If there were problems with the blob contents, other errors would be raised.

I think the warning is fine, but not for exactly the reasons you state. Several points:

- At least with v17 input, where it's possible, we probably *should* check that an input blob isn't truncated in the middle of the strings or structure sections. That should be more than a warning.

- Milton, saying totalsize indicates the amount of data to be copied is misleading in this context. That's a good philosphy for things that just read and/or slightly tweak the tree - data outside the known sections which it can't interpret should be left unaltered wherever possible. dtc, however, *always* fully interprets and re-emits the tree. Any data outside the known and understood sections is *always* discarded, so I don't think there's any problem assuming it to be zero.

- That said, I think when using -S, at least the default behaviour should emit extra zero bytes in addition to changing the totalsize header. Then at least in the simplest case of feeding dtc's dtb output back into dtc, the warning will not occur.

Signature · 4 lines

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

Jerry Van Baren

2007-04-16 01:20 UTC #4 lore raw

Quoted 64 lines · David Gibson

Quoted 42 lines · Jerry Van Baren
Quoted 15 lines · Milton Miller
Quoted 6 lines · Jerry Van Baren
Add extra reserve map slots output for asm format (previously done for dtb
output).
Signed-off-by Gerald Van Baren <vanbaren at cideas.com>
---
Quoted 7 lines
Hi Jon, David,
Here is a patch that fixes the asm output without the (unnecessary) calloc change.
Best regards,
gvb
Quoted 6 lines
The previous description had
Quoted 3 lines
Use cmalloc to pre-zero memory (for dtb input) and handle dtb (binary)
input being shorter than the total blob length (result of putting extra space in the blob).
Which at least said in the description the unrelated things it was doing.
That was my added comment WRT the change from malloc to cmalloc. David wasn't wild about using cmalloc, so I removed it. Using cmalloc is not necessary.
Quoted 18 lines
flattree.c struct boot_info *dt_from_blob(FILE *f) · line 831
Quoted 10 lines
        while (sizeleft) {
flattree.c +2 -3
-               if (feof(f))
-                       die("EOF before reading %d bytes of DT blob\n",
-                           totalsize);
+               if (feof(f)) {
+                       WARNMSG("EOF after reading %d of %d bytes of 
DT blob, assuming there is extra space in the blob.\n",
flattree.c +3 -0
+                           totalsize - sizeleft, totalsize);
+                       break;
+               }
I thnk the above should be an ERROR and cause failure without the -f (force) option.
The total_size says how much data should be copied. Anything
less and there is data missing. Assuming zeros is wrong for
most sections (the exception being the memory reserve list that had a terminating 0 entry within the read portion).
Quoted 4 lines
Quoted 1 line
milton
The reason total_size is bigger than the actual size is because I created the blob with extra space using the -S parameter. It is intentionally bigger. The extra space is ignored by dtc when creating a dts/asm format output which is why cmalloc() is unnecessary.
I suppose we could require a -f force but I'm not wild about creating a nanny program. There is nothing wrong with the blob - it parses just fine. If there were problems with the blob contents, other errors would be raised.
I think the warning is fine, but not for exactly the reasons you state. Several points:
- At least with v17 input, where it's possible, we probably *should* check that an input blob isn't truncated in the middle of the strings or structure sections. That should be more than a warning.
- Milton, saying totalsize indicates the amount of data to be copied is misleading in this context. That's a good philosphy for things that just read and/or slightly tweak the tree - data outside the known sections which it can't interpret should be left unaltered wherever possible. dtc, however, *always* fully interprets and re-emits the tree. Any data outside the known and understood sections is *always* discarded, so I don't think there's any problem assuming it to be zero.

Quoted 4 lines

- That said, I think when using -S, at least the default behaviour should emit extra zero bytes in addition to changing the totalsize header. Then at least in the simplest case of feeding dtc's dtb output back into dtc, the warning will not occur.

Hi David,

Yes, emitting zeros (or asm equiv) for the extra space makes sense. That would be a better way to fix the read problem.

I'm thinking about a fill/nofill and/or specifying the value for the fill bytes, but I cannot think of a reason that would be useful (my first thought was filling with 0xFF for flash, but I think most or all fdt writes need to modify some existing values so being able to modify the extra space in-place doesn't help because the existing values cannot be modified in-place in flash).

Now to find some time to implement it...

Best regards,
gvb

Milton Miller

2007-04-16 03:50 UTC #5 lore raw

In reply to David Gibson

Quoted 25 lines · David Gibson

Quoted 24 lines · Jerry Van Baren
Quoted 23 lines · Milton Miller
Quoted 7 lines · Jerry Van Baren
Add extra reserve map slots output for asm format (previously done for dtb
output).
Signed-off-by Gerald Van Baren <vanbaren at cideas.com>
---
Quoted 7 lines
Hi Jon, David,
Here is a patch that fixes the asm output without the (unnecessary) calloc change.
Best regards,
gvb
The previous description had
Quoted 3 lines
Use cmalloc to pre-zero memory (for dtb input) and handle dtb (binary)
input being shorter than the total blob length (result of putting extra space in the blob).

Quoted 6 lines

Quoted 6 lines
Quoted 2 lines
Which at least said in the description the unrelated things it was doing.
That was my added comment WRT the change from malloc to cmalloc. David wasn't wild about using cmalloc, so I removed it. Using cmalloc is not necessary.

Jerry,

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
and handle dtb (binary)
input being shorter than the total blob length (result of putting extra space in the blob).

That part is still in this patch.

And I think it should be a separate patch. Its unrelated to filling in .long 0 for the memory reserve map.

That said, one could use .space there I suppose. Its fine the way it is.

Quoted 20 lines

Quoted 20 lines
Quoted 20 lines
flattree.c struct boot_info *dt_from_blob(FILE *f) · line 831
Quoted 11 lines
        while (sizeleft) {
(quoted diff) +0 -2
-               if (feof(f))
-                       die("EOF before reading %d bytes of DT 
blob\n",
flattree.c +2 -1
-                           totalsize);
+               if (feof(f)) {
+                       WARNMSG("EOF after reading %d of %d bytes of
DT blob, assuming there is extra space in the blob.\n",
flattree.c +3 -0
+                           totalsize - sizeleft, totalsize);
+                       break;
+               }
I thnk the above should be an ERROR and cause failure without the -f (force) option.
The total_size says how much data should be copied. Anything
less and there is data missing. Assuming zeros is wrong for
most sections (the exception being the memory reserve list that had a terminating 0 entry within the read portion).

Quoted 6 lines

Quoted 6 lines
Quoted 1 line
milton
The reason total_size is bigger than the actual size is because I created the blob with extra space using the -S parameter. It is intentionally bigger. The extra space is ignored by dtc when creating a dts/asm format output which is why cmalloc() is unnecessary.

If this is a case of reading in the files it creates, then its wrong to have the size created less than total_size. The space needs to be in the output file. To have it not be in the output is wrong. For instance it will not be allocated by objcopy nor the linker when its inserted into the dtb section of the zImage wrapper, which would lead to scribbling on memory belonging to something else, or at least unallocated. Similar for a firmware that treats the dt_struct as binary data. It might be loaded just before the initrd for instance.

Quoted 11 lines

Quoted 4 lines
I suppose we could require a -f force but I'm not wild about creating a nanny program. There is nothing wrong with the blob - it parses just fine. If there were problems with the blob contents, other errors would be raised.
I think the warning is fine, but not for exactly the reasons you state. Several points:
- At least with v17 input, where it's possible, we probably *should* check that an input blob isn't truncated in the middle of the strings or structure sections. That should be more than a warning.

Or check that (1) the memory reserve list is terminated before this point, (2) the dt_struct has matching node begin and end count and ends with tree end, and (3) all strings referenced by dt_struct are before the read size.

Quoted 5 lines

- Milton, saying totalsize indicates the amount of data to be copied is misleading in this context. That's a good philosphy for things that just read and/or slightly tweak the tree - data outside the known sections which it can't interpret should be left unaltered wherever possible.

Actually, I think anything modifiny a tree with a higher revision than it understands, that has to move sections to increase the space, must downgrade the tree to the revision that it understands. It just doesn't know what the new structure revison means, and that its safe to claim it still meets the new revision.

Quoted 4 lines

dtc, however, *always* fully interprets and re-emits the tree. Any data outside the known and understood sections is *always* discarded, so I don't think there's any problem assuming it to be zero.

I haven't looked at the code, but it appears that this is just reading the blob into memory. I'd be fine with short file handling if it was parsing the structure as it was reading.

If you want to make it not an error, then it should be confirmed that
the missing information is not accessed. That can be done by parsing
the structure and verifying one doesn't read beyond what was read, or by moving it to but up against unmapped pages for all I care. For that matter, I don't care if any section is truncated as long as the parse shows all data was read before the end.

Regardless, this is unrelated to properly allocating the requested space for extra memory reserve slots in asm output, which is the subject of this patch.

Quoted 4 lines

- That said, I think when using -S, at least the default behaviour should emit extra zero bytes in addition to changing the totalsize header. Then at least in the simplest case of feeding dtc's dtb output back into dtc, the warning will not occur.

Change default behavior to only behavior and I agree. And its the real fix for the problem.

milton

David Gibson

2007-04-16 04:16 UTC #6 lore raw

Quoted 39 lines · Milton Miller

Quoted 27 lines · David Gibson
Quoted 26 lines · Jerry Van Baren
Quoted 25 lines · Milton Miller
Quoted 8 lines · Jerry Van Baren
Add extra reserve map slots output for asm format (previously done for
dtb
output).
Signed-off-by Gerald Van Baren <vanbaren at cideas.com>
---
Quoted 7 lines
Hi Jon, David,
Here is a patch that fixes the asm output without the (unnecessary) calloc change.
Best regards,
gvb
The previous description had
Quoted 4 lines
Use cmalloc to pre-zero memory (for dtb input) and handle dtb (binary)
input being shorter than the total blob length (result of putting extra space in the blob).
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
Which at least said in the description the unrelated things it was doing.
That was my added comment WRT the change from malloc to cmalloc. David
wasn't wild about using cmalloc, so I removed it. Using cmalloc is not
necessary.
Jerry,

Quoted 8 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
and handle dtb (binary)
input being shorter than the total blob length (result of putting extra space in the blob).
That part is still in this patch.
And I think it should be a separate patch. Its unrelated to filling in .long 0 for the memory reserve map.

Yes.

Quoted 2 lines

That said, one could use .space there I suppose. Its fine the way it is.

I think .space would be the preferred method for adding the padding space at the end in asm format.

Quoted 28 lines

Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
flattree.c struct boot_info *dt_from_blob(FILE *f) · line 831
Quoted 11 lines
        while (sizeleft) {
(quoted diff) +0 -2
-               if (feof(f))
-                       die("EOF before reading %d bytes of DT 
blob\n",
flattree.c +2 -1
-                           totalsize);
+               if (feof(f)) {
+                       WARNMSG("EOF after reading %d of %d bytes of
DT blob, assuming there is extra space in the blob.\n",
flattree.c +3 -0
+                           totalsize - sizeleft, totalsize);
+                       break;
+               }
I thnk the above should be an ERROR and cause failure without the -f (force) option.
The total_size says how much data should be copied. Anything
less and there is data missing. Assuming zeros is wrong for
most sections (the exception being the memory reserve list that had a terminating 0 entry within the read portion).
Quoted 7 lines
Quoted 7 lines
Quoted 1 line
milton
The reason total_size is bigger than the actual size is because I created the blob with extra space using the -S parameter. It is intentionally bigger. The extra space is ignored by dtc when creating a
dts/asm format output which is why cmalloc() is unnecessary.

Quoted 9 lines

If this is a case of reading in the files it creates, then its wrong to have the size created less than total_size. The space needs to be in the output file. To have it not be in the output is wrong. For instance it will not be allocated by objcopy nor the linker when its inserted into the dtb section of the zImage wrapper, which would lead to scribbling on memory belonging to something else, or at least unallocated.
Similar for a firmware that treats the dt_struct as binary data. It might be loaded just before the initrd for instance.

Well, I can see specialized case uses for totalsize greater than stored size: where you know the blob is going to be copied into another staging area with more space, for example.

Quoted 10 lines

Quoted 10 lines
Quoted 6 lines
I suppose we could require a -f force but I'm not wild about creating a
nanny program. There is nothing wrong with the blob - it parses just fine. If there were problems with the blob contents, other errors would
be raised.
I think the warning is fine, but not for exactly the reasons you state. Several points:

Quoted 8 lines

Quoted 3 lines
- At least with v17 input, where it's possible, we probably *should* check that an input blob isn't truncated in the middle of the strings or structure sections. That should be more than a warning.
Or check that (1) the memory reserve list is terminated before this point, (2) the dt_struct has matching node begin and end count and ends with tree end, and (3) all strings referenced by dt_struct are before the read size.

I think just checking the header lengths of the sub-blocks should be sufficient at this point. Checking that the begin/end count matches in the structure block and that all the string references are valid can, I think, be correctly delayed until we actually parse the structure block.

Quoted 11 lines

Quoted 5 lines
- Milton, saying totalsize indicates the amount of data to be copied is misleading in this context. That's a good philosphy for things that just read and/or slightly tweak the tree - data outside the known sections which it can't interpret should be left unaltered wherever possible.
Actually, I think anything modifiny a tree with a higher revision than it understands, that has to move sections to increase the space, must downgrade the tree to the revision that it understands. It just doesn't know what the new structure revison means, and that its safe to claim it still meets the new revision.

Oh, absolutely the version should be downgraded on writes. I wasn't really thinking of new format revisions adding extra data - I was thinking of hacks like programs stuffing some extra, runtime-use data into the "gaps" in the device tree blob. Clearly any such approach is always going to be risky to combine with r/w tree access, but it could be useful in some circumstances, so it doesn't hurt to be robust against it when we can.

Quoted 9 lines

Quoted 4 lines
dtc, however, *always* fully interprets and re-emits the tree. Any data outside the known and understood sections is *always* discarded, so I don't think there's any problem assuming it to be zero.
I haven't looked at the code, but it appears that this is just reading the blob into memory. I'd be fine with short file handling if it was parsing the structure as it was reading.

Quoted 6 lines

If you want to make it not an error, then it should be confirmed that
the missing information is not accessed. That can be done by parsing
the structure and verifying one doesn't read beyond what was read, or by moving it to but up against unmapped pages for all I care. For that matter, I don't care if any section is truncated as long as the parse shows all data was read before the end.

Again just checking that the truncation doesn't occur in the middle of one of the data blocks as described by the header I think is sufficient here. More detailed checks happen when we parse the subsections.

Quoted 11 lines

Regardless, this is unrelated to properly allocating the requested space for extra memory reserve slots in asm output, which is the subject of this patch.
Quoted 4 lines
- That said, I think when using -S, at least the default behaviour should emit extra zero bytes in addition to changing the totalsize header. Then at least in the simplest case of feeding dtc's dtb output back into dtc, the warning will not occur.
Change default behavior to only behavior and I agree. And its the real fix for the problem.

Signature · 4 lines

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

Milton Miller

2007-04-16 05:09 UTC #7 lore raw

Quoted 20 lines · David Gibson

Quoted 14 lines · Milton Miller
Quoted 8 lines · David Gibson
Quoted 6 lines · Jerry Van Baren
Quoted 5 lines · Milton Miller
Quoted 4 lines · Jerry Van Baren
Add extra reserve map slots output for asm format (previously done for
dtb
output).
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
and handle dtb (binary)
input being shorter than the total blob length (result of
putting
extra space in the blob).
Quoted 4 lines
That part is still in this patch.
And I think it should be a separate patch. Its unrelated to filling in .long 0 for the memory reserve map.

Quoted 8 lines

Yes.
Quoted 2 lines
That said, one could use .space there I suppose. Its fine the way it is.
I think .space would be the preferred method for adding the padding space at the end in asm format.

For the space at the end, I agree. Or use .org 99b+size with 99: at the
begining of the struct. The .long 0 was more for memory reserve, where
the entries might be replaced before assembly.

Quoted 23 lines

Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
The total_size says how much data should be copied. Anything
less and there is data missing. Assuming zeros is wrong for
most sections (the exception being the memory reserve list that had a terminating 0 entry within the read portion).
Quoted 7 lines
Quoted 7 lines
Quoted 1 line
milton
The reason total_size is bigger than the actual size is because I created the blob with extra space using the -S parameter. It is intentionally bigger. The extra space is ignored by dtc when creating a
dts/asm format output which is why cmalloc() is unnecessary.
Quoted 9 lines
If this is a case of reading in the files it creates, then its wrong to have the size created less than total_size. The space needs to be in the output file. To have it not be in the output is wrong. For instance it will not be allocated by objcopy nor the linker when its inserted into the dtb section of the zImage wrapper, which would lead to scribbling on memory belonging to something else, or at least unallocated.
Similar for a firmware that treats the dt_struct as binary data. It might be loaded just before the initrd for instance.

Quoted 3 lines

Well, I can see specialized case uses for totalsize greater than stored size: where you know the blob is going to be copied into another staging area with more space, for example.

That can either be done with your embedding script processing and noticing the zeros at the end or by a special option I guess.

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
I suppose we could require a -f force but I'm not wild about creating a
nanny program. There is nothing wrong with the blob - it parses just fine. If there were problems with the blob contents, other errors would
be raised.

You mean like not creating output for -I fs -O dts when there are expected properties (files) missing? :-) The kernel booted just fine, it was processing /proc/device-tree after all.

Its probably wrong to check for expected properties when the output is dts. At least the checks should not be more than a warning (that you may have selected a subtree).

Quoted 12 lines

Quoted 11 lines
Quoted 6 lines
I think the warning is fine, but not for exactly the reasons you state. Several points:
- At least with v17 input, where it's possible, we probably *should* check that an input blob isn't truncated in the middle of the strings or structure sections. That should be more than a warning.
Or check that (1) the memory reserve list is terminated before this point, (2) the dt_struct has matching node begin and end count and ends with tree end, and (3) all strings referenced by dt_struct are before the read size.

Quoted 5 lines

I think just checking the header lengths of the sub-blocks should be sufficient at this point. Checking that the begin/end count matches in the structure block and that all the string references are valid can, I think, be correctly delayed until we actually parse the structure block.

Checking the sublock lengths is sufficient, but actually more
restrictive that what I said. And mine works on older formats
without the size fields. But yes, yours is simpler when the fields exist.

Or just leave it an error, fix the output, and make the user fix the
input. If you took off the zeros, you can add padding from /dev/zero
or /dev/random or /etc/motd, as you said the data should not be used.

milton

David Gibson

2007-04-16 06:30 UTC #8 lore raw

Quoted 59 lines · Milton Miller

Quoted 21 lines · David Gibson
Quoted 15 lines · Milton Miller
Quoted 9 lines · David Gibson
Quoted 7 lines · Jerry Van Baren
Quoted 6 lines · Milton Miller
Sometime around Sun Apr 15 12:29:14 EST 2007, Jerry Van Baren
Quoted 4 lines · wrote:
Add extra reserve map slots output for asm format (previously done for
dtb
output).
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
and handle dtb (binary)
input being shorter than the total blob length (result of
putting
extra space in the blob).
Quoted 4 lines
That part is still in this patch.
And I think it should be a separate patch. Its unrelated to filling in .long 0 for the memory reserve map.
Quoted 8 lines
Yes.
Quoted 2 lines
That said, one could use .space there I suppose. Its fine the way it is.
I think .space would be the preferred method for adding the padding space at the end in asm format.
For the space at the end, I agree. Or use .org 99b+size with 99: at the
begining of the struct. The .long 0 was more for memory reserve, where
the entries might be replaced before assembly.
Quoted 24 lines
Quoted 23 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
The total_size says how much data should be copied. Anything
less and there is data missing. Assuming zeros is wrong for
most sections (the exception being the memory reserve list that had a terminating 0 entry within the read portion).
Quoted 7 lines
Quoted 7 lines
Quoted 1 line
milton
The reason total_size is bigger than the actual size is because I created the blob with extra space using the -S parameter. It is intentionally bigger. The extra space is ignored by dtc when creating a
dts/asm format output which is why cmalloc() is unnecessary.
If this is a case of reading in the files it creates, then its wrong to have the size created less than total_size. The space needs to be in the output file. To have it not be in the output is wrong. For instance it will not be allocated by objcopy nor the linker when its inserted into the dtb section of the zImage wrapper, which would lead to
scribbling on memory belonging to something else, or at least unallocated.
Similar for a firmware that treats the dt_struct as binary data. It might be loaded just before the initrd for instance.

Quoted 7 lines

Quoted 3 lines
Well, I can see specialized case uses for totalsize greater than stored size: where you know the blob is going to be copied into another staging area with more space, for example.
That can either be done with your embedding script processing and noticing
the zeros at the end or by a special option I guess.

Such a possible future special option was all I had in mind when I said emitting the zeroes should be default rather than only behaviour.

Quoted 9 lines

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
I suppose we could require a -f force but I'm not wild about creating
a
nanny program. There is nothing wrong with the blob - it parses just
fine. If there were problems with the blob contents, other errors would
be raised.

Quoted 7 lines

You mean like not creating output for -I fs -O dts when there are expected properties (files) missing? :-) The kernel booted just fine, it was processing /proc/device-tree after all.
Its probably wrong to check for expected properties when the output is dts. At least the checks should not be more than a warning (that you may have selected a subtree).

Yes, yes, I know. This all comes back to the need for a substantially reworked warning/error system.

Quoted 28 lines

Quoted 12 lines
Quoted 11 lines
Quoted 6 lines
I think the warning is fine, but not for exactly the reasons you state. Several points:
- At least with v17 input, where it's possible, we probably *should* check that an input blob isn't truncated in the middle of the strings or structure sections. That should be more than a warning.
Or check that (1) the memory reserve list is terminated before this point, (2) the dt_struct has matching node begin and end count and ends with tree end, and (3) all strings referenced by dt_struct are before the read size.
Quoted 5 lines
I think just checking the header lengths of the sub-blocks should be sufficient at this point. Checking that the begin/end count matches in the structure block and that all the string references are valid can, I think, be correctly delayed until we actually parse the structure block.
Checking the sublock lengths is sufficient, but actually more
restrictive that what I said. And mine works on older formats
without the size fields. But yes, yours is simpler when the fields exist.
Or just leave it an error, fix the output, and make the user fix the
input. If you took off the zeros, you can add padding from /dev/zero
or /dev/random or /etc/motd, as you said the data should not be used.
milton

Signature · 4 lines

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