[PATCH 10/25] binman: Move section-building code into a function

[Alper Nebi Yasak <alpernebiyasak@gmail.com>]

On 03/11/2020 18:11, Simon Glass wrote:
> Hi Alper,
> 
> On Mon, 26 Oct 2020 at 17:20, Alper Nebi Yasak <alpernebiyasak@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.