On Thu, 28 Dec 2023 at 18:04, Raymond Mao <raymond....@linaro.org> wrote: > > Hi Ilias, > > On Thu, 28 Dec 2023 at 06:32, Ilias Apalodimas <ilias.apalodi...@linaro.org> > wrote: >> >> Hi Raymond, >> >> On Wed, 27 Dec 2023 at 23:08, Raymond Mao <raymond....@linaro.org> wrote: >> > >> > From: Simon Glass <s...@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 <s...@chromium.org> >> > Co-developed-by: Raymond Mao <raymond....@linaro.org> >> > Signed-off-by: Raymond Mao <raymond....@linaro.org> >> > Reviewed-by: Simon Glass <s...@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.apalodi...@linaro.org>