[PATCH v4 08/12] bloblist: Handle alignment with a void entry

[Ilias Apalodimas]

On Thu, 28 Dec 2023 at 18:04, Raymond Mao <raymond.mao at linaro.org> wrote:
>
> Hi Ilias,
>
> On Thu, 28 Dec 2023 at 06:32, Ilias Apalodimas <ilias.apalodimas at linaro.org> wrote:
>>
>> Hi Raymond,
>>
>> On Wed, 27 Dec 2023 at 23:08, Raymond Mao <raymond.mao at linaro.org> wrote:
>> >
>> > From: Simon Glass <sjg at chromium.org>
>> >
>> > Rather than setting the alignment using the header size, add an entirely
>> > new entry to cover the gap left by the alignment.
>> >
>> > Signed-off-by: Simon Glass <sjg at chromium.org>
>> > Co-developed-by: Raymond Mao <raymond.mao at linaro.org>
>> > Signed-off-by: Raymond Mao <raymond.mao at linaro.org>
>> > Reviewed-by: Simon Glass <sjg at chromium.org>
>> > ---
>> >  common/bloblist.c | 23 +++++++++++++++++++----
>> >  1 file changed, 19 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/common/bloblist.c b/common/bloblist.c
>> > index 705d9c6ae9..73dbbc01c0 100644
>> > --- a/common/bloblist.c
>> > +++ b/common/bloblist.c
>> > @@ -142,7 +142,7 @@ static int bloblist_addrec(uint tag, int size, int align_log2,
>> >  {
>> >         struct bloblist_hdr *hdr = gd->bloblist;
>> >         struct bloblist_rec *rec;
>> > -       int data_start, new_alloced;
>> > +       int data_start, aligned_start, new_alloced;
>> >
>> >         if (!align_log2)
>> >                 align_log2 = BLOBLIST_ALIGN_LOG2;
>> > @@ -151,10 +151,25 @@ static int bloblist_addrec(uint tag, int size, int align_log2,
>> >         data_start = map_to_sysmem(hdr) + hdr->alloced + sizeof(*rec);
>> >
>> >         /* Align the address and then calculate the offset from ->alloced */
>> > -       data_start = ALIGN(data_start, 1U << align_log2) - map_to_sysmem(hdr);
>> > +       aligned_start = ALIGN(data_start, 1U << align_log2) - data_start;
>> > +
>> > +       /* If we need to create a dummy record, create it */
>> > +       if (aligned_start) {
>> > +               int void_size = aligned_start - sizeof(*rec);
>> > +               struct bloblist_rec *vrec;
>> > +               int ret;
>> > +
>> > +               ret = bloblist_addrec(BLOBLISTT_VOID, void_size, 0, &vrec);
>>
>> I just noticed the 'BLOBLISTT'. Is that on purpose? or a typo?
>>
>> > +               if (ret)
>> > +                       return log_msg_ret("void", ret);
>> > +
>> > +               /* start the record after that */
>> > +               data_start = map_to_sysmem(hdr) + hdr->alloced + sizeof(*vrec);
>> > +       }
>> >
>> >         /* Calculate the new allocated total */
>> > -       new_alloced = data_start + ALIGN(size, 1U << align_log2);
>> > +       new_alloced = data_start - map_to_sysmem(hdr) +
>> > +               ALIGN(size, 1U << align_log2);
>>

Writing this as
new_alloced = hdr->alloced + sizeof(*rec) + ALIGN(size, 1U << align_log2);
is much more readable

>> So, wouldn't it make more sense to add the dummy record and align the
>> whole thing *after* we've added the entry?
>> Doing it like this might leave the last entry on an unaligned boundary, no?
>>
> The spec just cares about the TE *data* starts at an aligned address but not the TE header.
> Not sure if I fully understand your question but each TE *data* will start at an aligned address
> after calling `bloblist_addrec()`.
> And each TE is allowed to have its own alignment value, so we have to do the padding when
> a TE is being added but cannot predict the alignment value for the next TE - that means we
> cannot do the padding after each TE is added.
>

Ah thanks, this makes sense

> [...]
>
> Thanks and regards,
> Raymond

With the change above
Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>