[PATCH 10/25] binman: Move section-building code into a function
Alper Nebi Yasak
alpernebiyasak at gmail.com
Wed Nov 4 22:50:23 CET 2020
On 03/11/2020 18:11, Simon Glass wrote:
> Hi Alper,
>
> On Mon, 26 Oct 2020 at 17:20, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
>>
>> On 26/10/2020 22:22, Simon Glass wrote:
>>> I've added a few test cases along these lines in v2, and one of them
>>> certainly different behaviour. This is good actually since it shows a
>>> simple case of what these padding changes are intended to fix.
>
> See what you think of the above test cases - testSectionPad() and
> testSectionAlign()
I've tried to visualize those tests a bit, in the following:
- The vertical line of numbers is the offsets, usually starts with 0 and
ends with entry.size of that entry.
- The offset to the upper-left of a block is that entry's entry.offset
- The "*" offsets are unconstrained, determined as parts are fitted
together.
- The "k*n" are offsets for alignment that must be a multiple of k
- The vertical "[...]" line is what the entry.data returns for that
entry.
- The horizontal line from a value is what the value corresponds to.
Hope things make sense. I kind of started drawing things to gather my
thoughts and improve my understanding, but didn't want to discard them.
Please do tell if anything's wrong.
== 177_skip_at_start.dts ==
binman {
[ 0
. | section {
. | [ 0
. | . | 16 -------------------- binman/section:skip-at-start
. | . | | u-boot {
. | . | | [ 0
. | . | | . | U_BOOT_DATA
. | . | | ] *
. | . | | }
. | . | *
. | ] *
. | }
] *
}
I understand skip-at-start as if it's creating a frame of reference for
alignments. Then, that frame is mapped back to starting from zero.
It looks weird here for me to use two nested offset-lines here. I can't
use one line that starts at skip-at-start, because that changes the
entry.offset if the section has pad-before.
== 178_skip_at_start_pad.dts ==
binman {
[ 0
. | section {
. | [ 0
. | . | 16 -------------------- binman/section:skip-at-start
. | . | | u-boot {
. | . | | 0
. | . | | | 8 x <0x00>
. | . | | | \--------------- binman/section/u-boot:pad-before
. | . | | [ *
. | . | | . | U_BOOT_DATA
. | . | | ] *
. | . | | | 4 x <0x00>
. | . | | | \--------------- binman/section/u-boot:pad-after
. | . | | * ----------------- binman/section/u-boot:size
. | . | | }
. | . | *
. | ] *
. | }
] *
}
This is like the above, just adds padding to u-boot. I have to visualize
the padding as something inside the entry block, since alignments and
entry.size is calculated for the padded data, not the raw U_BOOT_DATA.
It's a bit weird but understandable that len(entry.data) != entry.size.
== 179_skip_at_start_section_pad.dts ==
binman {
[ 0
. | section {
. | 0
. | | 8 x <0x00>
. | | \--------------------- binman/section:pad-before
. | [ *
. | . | 16 -------------------- binman/section:skip-at-start
. | . | | u-boot {
. | . | | [ 0
. | . | | . | U_BOOT_DATA
. | . | | ] *
. | . | | }
. | . | *
. | ] *
. | | 4 x <0x00>
. | | \--------------------- binman/section:pad-after
. | *
. | }
] *
}
I'm still having trouble with this. In the old code:
> base = self.pad_before + (entry.offset or 0) - self._skip_at_start
8 + 16 - 16
> pad = base - len(section_data) + (entry.pad_before or 0)
8 - 0 + 0
> if pad > 0:
8
> section_data += tools.GetBytes(self._pad_byte, pad)
8
So, why was it prepending 16 bytes? The only way I see is if u-boot
entry.offset is 24, but that's explicitly checked to be 16 in the test.
However, it's clear to me now that the fragment I sent wouldn't result
in different padding between two versions, because there entry.offset =
section.skip_at_start so the negative padding never happens.
Then, what does an entry.offset < section.skip-at-start mean? That's
what was missing for the actual thing I was trying to trigger:
section {
skip-at-start = <16>;
blob {
offset = <0>;
pad-before = <16>;
filename = "foo";
};
};
== 180_section_pad.dts ==
binman {
[ 0
. | section at 0 {
. | 0
. | | 3 x <0x26> -------------- binman:pad-byte
. | | \----------------------- binman/section at 0:pad-before
. | [ *
. | . | u-boot {
. | . | 0
. | . | | 5 x <0x21> ---------- binman/section at 0:pad-byte
. | . | | \------------------- binman/section at 0/u-boot:pad-before
. | . | [ *
. | . | . | U_BOOT_DATA
. | . | ] *
. | . | | 1 x <0x21> ---------- binman/section at 0:pad-byte
. | . | * \------------------- binman/section at 0/u-boot:pad-after
. | . | }
. | ] *
. | | 2 x <0x26> -------------- binman:pad-byte
. | | \----------------------- binman/section at 0:pad-after
. | *
. | }
] *
}
It looks like paddings are part of the entry:
- entry.offset and entry.image_pos point to pad-before padding
- entry.size includes both paddings
- pad-before, pad-after properties belong to the entry
- entry's parent aligns the entry with respect to the padded-data
But, also the opposite:
- entry.data doesn't include padding bytes
- it's entirely added by the entry's parent
- pad-byte property belongs to the parent
I have no idea which way things should go towards. I think padding could
be completely handled by the entries themselves. Section's
GetPaddedDataForEntry(entry, entry_data) could be moved to Entry as
GetPaddedData(pad_byte), which the parent section would use while
assembling itself. The pad_byte argument could be dropped by making the
entries find it by traversing upwards in the tree starting from the
entry itself (and not just checking the immediate parent).
== 181_section_align.dts ==
binman {
[ 0
. | fill {
. | [ 0
. | . | <0x00>
. | ] 1 ------------------------- binman/fill:size
. | }
. *
. | <0x26> ---------------------- binman:pad-byte
. 2*n --------------------------- binman/section at 1:align
. | section at 1 {
. | [ 0
. | . | fill {
. | . | [ 0
. | . | . | <0x00>
. | . | ] 1 --------------------- binman/section at 1/fill:size
. | . | }
. | . *
. | . | <0x21> ------------------ binman/section at 1:pad-byte
. | . 4*n ----------------------- binman/section at 1/u-boot:align
. | . | u-boot {
. | . | [ 0
. | . | . | U_BOOT_DATA
. | . | ] *
. | . | | <0x21> -------------- binman/section at 1:pad-byte
. | . | 8*n ------------------- binman/section at 1/u-boot:size
. | . | \----------------------binman/section at 1/u-boot:align-size
. | . | }
. | ] *
. | | <0x21> ------------------ binman/section at 1:pad-byte
. | 0x10*n -------------------- binman/section at 1:size
. | \------------------------ binman/section at 1:align-size
. | }
] *
}
The pad-byte values here surprise me a bit. I'd say they should be the
parent's pad-byte, since I think this in-section alignment padding is
the same kind of thing as the pad-before and pad-after, and those use
the parent's. However, like what I said above, the latter two could
instead be changed to use the entry's pad-byte like this one.
More information about the U-Boot
mailing list