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

[Ilias Apalodimas]

On Thu, 28 Dec 2023 at 15:37, Simon Glass <sjg at chromium.org> wrote:
>
> 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?
>

Sure I dont mind,

Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>


> 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