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

Hi Raymond,

On Wed, 27 Dec 2023 at 23:08, Raymond Mao raymond.mao@linaro.org wrote:

From: Simon Glass sjg@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@chromium.org Co-developed-by: Raymond Mao raymond.mao@linaro.org Signed-off-by: Raymond Mao raymond.mao@linaro.org Reviewed-by: Simon Glass sjg@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