On 03/11/2020 18:11, Simon Glass wrote: > Hi Alper, > > On Mon, 26 Oct 2020 at 17:20, Alper Nebi Yasak <alpernebiya...@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@0 { . | 0 . | | 3 x <0x26> -------------- binman:pad-byte . | | \----------------------- binman/section@0:pad-before . | [ * . | . | u-boot { . | . | 0 . | . | | 5 x <0x21> ---------- binman/section@0:pad-byte . | . | | \------------------- binman/section@0/u-boot:pad-before . | . | [ * . | . | . | U_BOOT_DATA . | . | ] * . | . | | 1 x <0x21> ---------- binman/section@0:pad-byte . | . | * \------------------- binman/section@0/u-boot:pad-after . | . | } . | ] * . | | 2 x <0x26> -------------- binman:pad-byte . | | \----------------------- binman/section@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@1:align . | section@1 { . | [ 0 . | . | fill { . | . | [ 0 . | . | . | <0x00> . | . | ] 1 --------------------- binman/section@1/fill:size . | . | } . | . * . | . | <0x21> ------------------ binman/section@1:pad-byte . | . 4*n ----------------------- binman/section@1/u-boot:align . | . | u-boot { . | . | [ 0 . | . | . | U_BOOT_DATA . | . | ] * . | . | | <0x21> -------------- binman/section@1:pad-byte . | . | 8*n ------------------- binman/section@1/u-boot:size . | . | \----------------------binman/section@1/u-boot:align-size . | . | } . | ] * . | | <0x21> ------------------ binman/section@1:pad-byte . | 0x10*n -------------------- binman/section@1:size . | \------------------------ binman/section@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.