[PATCH v4 10/12] bloblist: Adjust the bloblist header

[Simon Glass]

Hi Ilias,

On Thu, Dec 28, 2023 at 7:37 AM Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Raymond,
>
> [...]
>
> >
> >  void bloblist_show_list(void)
> > @@ -463,7 +477,7 @@ void bloblist_reloc(void *to, uint to_size, void *from, uint from_size)
> >
> >         memcpy(to, from, from_size);
> >         hdr = to;
> > -       hdr->size = to_size;
> > +       hdr->total_size = to_size;
> >  }
> >
> >  int bloblist_init(void)
> > @@ -493,7 +507,7 @@ int bloblist_init(void)
> >                                     addr, ret);
> >                 } else {
> >                         /* Get the real size, if it is not what we expected */
> > -                       size = gd->bloblist->size;
> > +                       size = gd->bloblist->total_size;
> >                 }
> >         }
> >         if (ret) {
> > diff --git a/include/bloblist.h b/include/bloblist.h
> > index 7024d7bf9e..4ec4b3d449 100644
> > --- a/include/bloblist.h
> > +++ b/include/bloblist.h
> > @@ -166,32 +166,33 @@ enum bloblist_tag_t {
> >   * from the last.
> >   *
> >   * @magic: BLOBLIST_MAGIC
> > + * @chksum: checksum for the entire bloblist allocated area. Since any of the
> > + *     blobs can be altered after being created, this checksum is only valid
> > + *     when the bloblist is finalized before jumping to the next stage of boot.
> > + *     This is the value needed to make all checksummed bytes sum to 0
> >   * @version: BLOBLIST_VERSION
> >   * @hdr_size: Size of this header, normally sizeof(struct bloblist_hdr). The
> >   *     first bloblist_rec starts at this offset from the start of the header
> > - * @flags: Space for BLOBLISTF... flags (none yet)
> > - * @size: Total size of the bloblist (non-zero if valid) including this header.
> > - *     The bloblist extends for this many bytes from the start of this header.
> > - *     When adding new records, the bloblist can grow up to this size.
> > - * @alloced: Total size allocated so far for this bloblist. This starts out as
> > + * @align_log2: Power of two of the maximum alignment required by this list
> > + * @used_size: Size allocated so far for this bloblist. This starts out as
> >   *     sizeof(bloblist_hdr) since we need at least that much space to store a
> >   *     valid bloblist
> > + * @total_size: The number of total bytes that the bloblist can occupy.
> > + *     Any blob producer must check if there is sufficient space before adding
> > + *     a record to the bloblist.
> > + * @flags: Space for BLOBLISTF... flags (none yet)
> >   * @spare: Spare space (for future use)
> > - * @chksum: checksum for the entire bloblist allocated area. Since any of the
> > - *     blobs can be altered after being created, this checksum is only valid
> > - *     when the bloblist is finalised before jumping to the next stage of boot.
> > - *     This is the value needed to make all checksummed bytes sum to 0
> >   */
> >  struct bloblist_hdr {
> >         u32 magic;
> > -       u32 version;
> > -       u32 hdr_size;
> > +       u8 chksum;
> > +       u8 version;
> > +       u8 hdr_size;
> > +       u8 align_log2;
> > +       u32 used_size;
> > +       u32 total_size;
> >         u32 flags;
> > -
> > -       u32 size;
> > -       u32 alloced;
> >         u32 spare;
> > -       u32 chksum;
> >  };
>
> The patch generally looks ok, but while we are renaming things, can't
> we just copy what the spec says instead of slightly changing the
> names?
>
> With this applied we end up with
>   struct bloblist_hdr {
>          u32 magic;
>          u8 chksum;
>          u8 version;
>          u8 hdr_size;
>          u8 align_log2;
>          u32 used_size;
>          u32 total_size;
>          u32 flags;
>          u32 spare;
>   };
>
> Can you at least rename the ones you touch here properly?
> - Drop the patch that renames spare to _spare
> - used_size -> size
> - total_size -> max_size (which btw mean different things)
> - spare -> reserved

Yes that would be good, but since the spec only recently changed the
names, how about a 'rename' patch on top, if that is easier?

I was hoping that this series could be applied as is, or close to it.
It looks like a good base to me.

Regards,
Simon