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); > > 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. [...] Thanks and regards, Raymond