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

[Ilias Apalodimas]

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);

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?

Thanks
/Ilias
>
>         if (new_alloced > hdr->size) {
>                 log_err("Failed to allocate %x bytes size=%x, need size=%x\n",
> @@ -164,7 +179,7 @@ static int bloblist_addrec(uint tag, int size, int align_log2,
>         rec = (void *)hdr + hdr->alloced;
>
>         rec->tag = tag;
> -       rec->hdr_size = data_start - hdr->alloced;
> +       rec->hdr_size = sizeof(struct bloblist_rec);
>         rec->size = size;
>
>         /* Zero the record data */
> --
> 2.25.1
>